Le mercredi 15 mai 2013 à 09:49 +0930, Jonathan Woithe a écrit :
> Hi Philippe
> Thanks for the preliminary look at this work. It looks very interesting.
> On Tue, May 14, 2013 at 08:19:56PM +0200, Philippe Carriere wrote:
> > I worked last week in order to provide additional "per output setting"
> > tabs in MatrixMixer, something which might make things easier when large
> > number of Mixer inputs/outputs are involved.
> My understanding of this is that you present a series of tabs - one for each
> output - which are populated with sliders representing the send level from
> each input to that output. If this is correct then it sounds like a
> worthwhile addition.
> > I also tried to clarify the naming of channels.
> In what way?
Until now, Mixer channel names were "Ch 1", "Ch 2", ... whatever they
are inputs or outputs.
Here "Mixer/In:01", ... and "Mixer/Out:01", ... are used: first it makes
clear what are inputs and outputs; second, these ones are the names used
in the router section, it makes easier, for the user, the correlation
between the mixer in and out and the corresponding routing.
> > Now I know RME devices also use MatrixMixer and I am not able to test
> > for them, only know it works fine for Saffire Pro.
> A number of devices use the MatrixMixer object. Those I can spot
> immediately are all the Saffires (not just the "pro"), all the RMEs and the
> generic DICE mixer.
Oh, sorry, I should have use the "generic dice_eap", not the Saffire Pro
(which rather refer to older Saffire Pro). So I mean I was just able to
test for the generic DICE EAP case (with Saffire Pro 40 as the testing
I checked before sending you my last email that, apparently, only RMEs
are effectively also involved. Previous (not DICE EAP ones) Saffire
models have apparently their own mixers, but possibly I miss something.
> > Could you have a test with this patch ? Probably, it would at least
> > require some modification in rme.py as in generic_dice_eap.py
> > (essentially eliminating the scrollarea at this level, scrollarea being
> > created further).
> Note that when making API changes like this the usual convention in the FOSS
> world is to update all the code which uses the thing you're changing as part
> of the patch. Obviously you can't necessarily test the changes, but in most
> cases the alternations needed are very mechanical in nature. In situations
> where there are fundamental changes in the behaviour, it is sometimes
> appropriate to retain the old behaviour as an option or activate the new
> with an option.
It's true that I could have made the corresponding changes ...
> Of course at this early stage, there would be no reason to do any of this in
> your personal repo because the changes needed may change as you further
> develop the patch. But once the way forward is known you should at least
> propose the changes for the other users of the MatrixMixer widget to give
> the maintainers of that code something to evaluate. But see below for
> further thoughts in relation to this.
> > I will have some further modifications after these ones for automatically
> > refreshing the routing in matrixmixer when needed, but first I await for
> > your answer.
> I will only be able to test this later in the week. In the meantime, it
> would be helpful to me if you can clarify what it is that you're adding
I sent you additional patches via another email.
> A high-level comment: when you come to merge this work later it would be
> good to split the patch up into logically separated patches. It seems that
> there are a number of changes in this patch which could quite easily be
> considered separately. For example, the ability to name the I/Os does not
> depend on the "per output setting" code as such. This produces a clearer
> history in the repo. It also means that reasonably trivial changes (such as
> the editable labels) can go in quickly, making it easier to evaluate the
> more complex issues.
Yes, of course; these patches are only draft ones extracted from my own
git repo. Before submitting, I usually re-operate to produce more
patches each focusing on a specific modification.
> Some general comments about the patch follow.
> > diff --git a/libffado/support/mixer-qt4/ffado/widgets/matrixmixer.py b/libffado/support/mixer-qt4/ffado/widgets/matrixmixer.py
> > index 9c06f4b..1bed732 100644
> > --- a/libffado/support/mixer-qt4/ffado/widgets/matrixmixer.py
> > +++ b/libffado/support/mixer-qt4/ffado/widgets/matrixmixer.py
> > @@ -1,6 +1,7 @@
> > # coding=utf8
> > #
> > # Copyright (C) 2009 by Arnold Krille
> > +# Copyright (C) 2013 by Philippe Carriere
> > #
> > # This file is part of FFADO
> > # FFADO = Free Firewire (pro-)audio drivers for linux
> > @@ -215,34 +216,96 @@ class MixerChannel(QtGui.QWidget):
> > QtGui.QWidget.__init__(self, parent)
> > layout = QtGui.QGridLayout(self)
> > self.number = number
> > - if name != "":
> > - name = "\n(%s)" % name
> Any idea why the CR was being added here in the first place? What is
> achieved by removing it?
I have no possibilty for testing right now: it looks like I omitted to
erase the CR ...
> > self.name = name
> > self.lbl = QtGui.QLabel(self)
> > self.lbl.setAlignment(Qt.Qt.AlignCenter)
> > +
> > + # Names may be quite long
> > + font = self.lbl.font()
> > + font.setPointSize(font.pointSize()/1.5)
> > + self.lbl.setFont(font)
> > +
> > + # Might be shortened more
> > if (smallFont):
> > font = self.lbl.font()
> > - font.setPointSize(font.pointSize()/1.5)
> > + font.setPointSize(font.pointSize()/2.0)
> > self.lbl.setFont(font)
> > - layout.addWidget(self.lbl, 0, 0, 1, 2)
> > + layout.addWidget(self.lbl, 0, 0, 1, 1)
> > +
> I gather here that you're reducing the font size to account for longer
> names. In theory this isn't a bad idea, but I'm concerned that the
> resulting labels might be too small for many people. I'll see what the end
> result is and comment further on this later.
Yes, downsizing so much is probably not a good idea.
> Rather than statically setting the font size like this I wonder whether it
> might be better to downscale the font size only if necessary. That is, if
> the name is longer than a certain size then select a smaller font.
Or let the user choose the downsizing (like he can choose to hide the
> This font size change would be a good candidate for splitting out and
> treating as a separate patch.
> > + # User Out/In naming utility
> > + self.edit = QtGui.QLineEdit(self)
> > + self.edit.setAlignment(Qt.Qt.AlignCenter)
> > + layout.addWidget(self.edit, 1, 0, 1, 1)
> > + self.connect(self.edit, QtCore.SIGNAL("textChanged(const QString &)"), self.editChannelName)
> > +
> This is an interesting addition and could be quite useful. However, it's
> really only worth doing if there's a way to save the user labels. Otherwise
> you have to go through and label them all every time. Do you have any
> thoughts about that?
Well, here is a point: as far as file saving of mixer/router/monitoring
setting is not yet developed, the user will have to label each time a
new session is started.
I hope I will have some time further to develop for file saving.
> This is another change which could be split out and considered separately I
> > self.hideChannel(False)
> > self.setContextMenuPolicy(Qt.Qt.ActionsContextMenu)
> > + # Hide almost completely the channel
> > action = QtGui.QAction("Make this channel small", self)
> > action.setCheckable(True)
> > self.connect(action, QtCore.SIGNAL("triggered(bool)"), self.hideChannel)
> > self.addAction(action)
> The added comment is unnecessary. The code makes the intent clear.
> > + # Hide label and edit line for this channel and replace it by the channel number or vice versa
> > + # Then emit a signal for further use
> I have a slight preference that comments be wrapped at 80 characters, but
> it's a minor thing in the scheme of things.
OK, I will correct this.
> The rest of the patch deals almost exclusively with the new "per output" tab
> thing which I'll look at in more detail when I'm testing it. In general I
> think this is a very worthwhile feature and I'm keen to see this refined and
> ultimately merged.
> I have a few other things to bring up for your to ponder. The first is that
> there are some use cases where the matrix mixer is used with N inputs and a
> single "output" as an easy way to construct an array of controls (the RME
> does this for example). When this is done, the option of having sliders is
> very attractive. I mention this so you can ensure that the new code behaves
> sensibly even when only one output is specified.
Reading your comment, I thought that there is still some adjustments to
do (most probably rather concerning the additional patches I sent you)
for the single output case: it is intended to work, but possibly python
will complain at some points (typically, changing the value of something
which does not exist).
> Related to this, I wonder how complex it might be to make the the "per
> output" tabs optional, selected via a parameter to the constructor? The
> code seems reasonably clean so this could be easy to do (however I'll admit
> that there still may be some problems lurking under the surface which may
> prevent this from being easy). The advantage here is that all existing
> users of the matrixmixer widget would retain their existing behaviour
> without any changes required within them. The per output tabs could then be
> enabled as people have the opportunity to test and modify as required. This
> then would remove the need for you to worry about updating mixers other than
> the one you are specifically working on (the Saffire Pro). This was the
> approach I took when I added the mute and phase inversion control extensions
> some time ago: by default nothing changed, with the new features only
> visible and useable when explicitly requested by the caller.
Well, as the per output sections are only additional ones, nothing is
intended to be changed for the ones who would exclusively use the matrix
view; the only change is about scrollarea but I am even not sure that
the modification is really imperatively required (I will test again for
this, but, if I remember well, it worked for a while before I modified
Making the per output tabs really optional (I mean not only not visible
at first, since otherwise the problem would remain) might require a lot
of additional work, whatever much more than simply adapting the
scrollarea encoding. So I'm not sure the result is worth the effort (I
admit I googled to find such an expression; hope it is relevant :-)
> The matrixmixer widget has some other optional functionality in addition to
> the faders: if suitable dbus paths are supplied it can also implement
> channel muting and phase inversion. At this stage I suspect the RME is the
> only device to make use of these features, and as a result it will probably
> fall to me to implement them in the per-output tabs if deemed important
> enough. However, I was wondering whether you had any thoughts as to how the
> inclusion of suitable switch controls might be done in the long run. To
> facilitate this sort of thing, I wonder whether it might be worth creating a
> top-level channel strip object, and into that place the fader that you're
> adding now. In time the "channel strip" container could then be extended
> with additional controls as required without the need to fundamentally
> change the structure.
Yes, I saw these additional features: presently no modification are
performed. Most probably they will fall to you (recalling that the
matrix view whatever remains; so is it really imperative, at least at
first, to implement these feature inside the per output view ?)
I will be traveling until next Thursday (I mean the 23th) and not be
able to apply anything in the meantime. But as soon, as I will come
home, I will begin to improve the patches.
Philippe Carriere <la-page-web-of-phil.contact@...>