Hi Jonathan,

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.

Thanks.

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 device).
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).

Sure.

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
here.

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 sources/destinations names).

This font size change would be a good candidate for splitting out and
treating as a separate patch.

Of course.

> +        # 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
think.

>          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 it).
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 ?)

Regards
  jonathan

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.

Best regards,

Phil

--
Philippe Carriere <la-page-web-of-phil.contact@orange.fr>