From: Philippe C. <la-...@or...> - 2013-05-14 18:20:07
|
Hi Jonathan, 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. I also tried to clarify the naming of channels. 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. 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). 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. Best regards, Phil -- Philippe Carriere <la-...@or...> |
From: Jonathan W. <jw...@ju...> - 2013-05-15 00:55:37
|
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? > 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. > 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. 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. 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. 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? > 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. 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. 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? 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. 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. 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. 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. Regards jonathan |
From: Philippe C. <la-...@or...> - 2013-05-15 17:19:21
|
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-...@or...> |
From: Philippe C. <la-...@or...> - 2013-05-15 17:21:55
|
Hi Jonathan, Le mercredi 15 mai 2013 à 09:49 +0930, Jonathan Woithe a écrit : > > 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. Enclosed are the corresponding patches. There was something not satisfying in the first patch, i.e. the router part would have to know about the mixer encoding. I corrected this in the second patch. Regards, Phil PS: there is also something not so straightforward when redirecting the signal (patch 3). If anybody knows how to do this more straightforwardly using python, you are welcome. -- Philippe Carriere <la-...@or...> -- Philippe Carriere <la-...@or...> |
From: Philippe C. <la-...@or...> - 2013-06-15 06:41:36
|
Hi Jonathan, Le vendredi 14 juin 2013 à 20:19 +0930, Jonathan Woithe a écrit : > There is what appears to be a mapping problem in the "columns_are_inputs" > mode used by the RME. The matrix view works as it always has, and the > per-output tabs are initialised to the correct values. However, there seems > to be a translation problem when a control is changed. If I alter input > (column) 2 in output (row) 1 in matrix view, the corresponding value shows > up in the input 1 slider on the out:2 tab. The same happens when changing > the tab too. It seems there's one location in the code somewhere which is > missing the column/row translation. > Should be fixed by patch 0004. > This translation issue also affects the single-row "output levels" matrix > used with the RME. Because of the missing translation, changes made in the > matrix are never mapped to the slider view (with the exception of the first > cell of course which, being row 1 column 1 doesn't need translation to > work). > > The patch does break the reloading of mixer settings from device flash: we > crash out in matrixmixer.py at line 591: AttributeError: 'MatrixMixer' > object has no attribute 'items'. I *think* this is due to the use of > "self.items" thoughout this function rather than "self.matrix.items" which > appears to be needed now - could you check this. Another issue which needs > to be resolved here is that refreshValues() will currently only refresh the > matrix view. Code needs to be added to also refresh the controls in the > per-output tabs. > Should be fixed by 0005 and 0006. However, I didn't really test refreshValues(); may be, for instance, that column/row are inverted, in which case it would require to exchange x for y in the adds-on part. > One little irritation: when a slider in an output tab is set to its minimum > value, it corresponds to -40 dB in the matrix view. Similarly, if setting a > matrix cell to "-inf" it is forced to -40 - it is necessary to then select > "-inf" again to truly turn the control to minimum. The slider's minimum > really needs to translate to -inf. Ideally the slider's gain range should > match that of the matrix cells. This is also affecting the numeric input in > the right-click dropdown menu in the matrix view: like the matrix cells, it > should also go all the way down to -inf. > > I expect the slider's minimum of -40 and the interaction between the > controls is the reason why setting "-inf" in matrix view goes via "-40". > Fixed in 0007. I also tried to the infinity symbol in the pop menu: it worked but lead to a further problem when translating the value. So I did not elaborate further at this point; it could be a further development. > Finally, here's just something cosmetic to think about. For the generic > names, how about using "Input" and "Output" rather than "Mixer/In" and > "Mixer/Out"? It's no longer and probably reads better for users. The > "Mixer/" part is redundant anyway - we can tell it's a mixer. :-) > Mixer/In and Mixer/Out are the names (coming from dice_eap.c) really appearing in the router part of EAP devices. So, for a better understanding of users, I prefer they are kept so. Users have whatever the possibility of reducing these names. And we will have to discuss together about such features for further improvements. > > If no rough bug is present for RME, I propose to introduce the > > modifications. ... > > I guess it depends on what you call a "rough bug". :-) > > 1. The translation bug is obviously fairly severe from a user perspective > since it will create considerable confusion. However, I doubt it will be > difficult to fix. > > 2. Having the sliders go to -inf is fairly important. In many respects I > would consider this more pressing than bug 1. > > 3. The bugfix and extension to refreshValues() is needed before this is > fully workable for the RME (recalling mixer settings from flash and/or > software is a fairly common activity). > These bugs are sufficiently severe so as to be overcome before merging. If they are solved by the enclosed patches, then I will merge modifications rapidly. > The rest of my points are cosmetic or usability suggestions which certainly > could wait until the code was merged if you want. > > These patches are generally in good shape. I'll leave it up to you whether > you want to address the three points above before merging. However, if you > do merge first I would like to see these addressed fairly quickly. > > Thanks for the effort you've put into this feature. I expect that a lot of > people will find it extremely useful. > > Regards > jonathan Regards, Phil -- Philippe Carriere <la-...@or...> |
From: Jonathan W. <jw...@ju...> - 2013-06-15 11:39:51
|
Hi Phil > > There is what appears to be a mapping problem in the "columns_are_inputs" > > mode used by the RME. The matrix view works as it always has, and the > > per-output tabs are initialised to the correct values. However, there seems > > to be a translation problem when a control is changed. If I alter input > > (column) 2 in output (row) 1 in matrix view, the corresponding value shows > > up in the input 1 slider on the out:2 tab. The same happens when changing > > the tab too. It seems there's one location in the code somewhere which is > > missing the column/row translation. > > Should be fixed by patch 0004. I can confirm that patch 0004 makes the correct slider change when a matrix cell is altered. As expected, the slider view of the single row "output levels" matrix is also fixed. > > The patch does break the reloading of mixer settings from device flash: we > > crash out in matrixmixer.py at line 591: AttributeError: 'MatrixMixer' > > object has no attribute 'items'. I *think* this is due to the use of > > "self.items" thoughout this function rather than "self.matrix.items" which > > appears to be needed now - could you check this. Another issue which needs > > to be resolved here is that refreshValues() will currently only refresh the > > matrix view. Code needs to be added to also refresh the controls in the > > per-output tabs. > > Should be fixed by 0005 and 0006. However, I didn't really test > refreshValues(); may be, for instance, that column/row are inverted, in > which case it would require to exchange x for y in the adds-on part. It seems that the correct sliders are being updated when this function is called. That is, after calling this function the sliders are set consistent with the state shown by the matrix. So I think this is working as expected. > > One little irritation: when a slider in an output tab is set to its minimum > > value, it corresponds to -40 dB in the matrix view. ... > > > > I expect the slider's minimum of -40 and the interaction between the > > controls is the reason why setting "-inf" in matrix view goes via "-40". > > Fixed in 0007. I can confirm this. Setting "-inf" in the matrix dropdown does indeed go straight to the minimum setting as it used to. > I also tried to the infinity symbol in the pop menu: it > worked but lead to a further problem when translating the value. So I > did not elaborate further at this point; it could be a further > development. It clearly falls into the "nice touch" category. We don't need to hold the merge up just for this. Keeping "-inf" in the popup menu is fine. Hunk 1 of the 0007 failed to patch against svn. The relevant hunk is: --- support/mixer-qt4/ffado/widgets/matrixmixer.py +++ support/mixer-qt4/ffado/widgets/matrixmixer.py @@ -187,7 +187,8 @@ #log.debug("new value is %g dB" % lv) text = "%.2g dB" % lv if v == 0: - text = "-??? dB" + symb_inf = u"\u221E" + text = "-" + symb_inf + " dB" p.drawText(rect, Qt.Qt.AlignCenter, QtCore.QString.fromUtf8(text)) if (self.inv_action!=None and self.inv_action.isChecked()): p.drawText(rect, Qt.Qt.AlignLeft|Qt.Qt.AlignTop, QtCore.QString.fromUtf8(" ??")) I expect this may be a leftover from your experiments with the popup menu infinity sign. This hunk is referring to the symbol used in the matrix cell, but the existing code already happily puts an infinity symbol in there (it uses unicode I believe). > > Finally, here's just something cosmetic to think about. For the generic > > names, how about using "Input" and "Output" rather than "Mixer/In" and > > "Mixer/Out"? It's no longer and probably reads better for users. The > > "Mixer/" part is redundant anyway - we can tell it's a mixer. :-) > > Mixer/In and Mixer/Out are the names (coming from dice_eap.c) really > appearing in the router part of EAP devices. So, for a better > understanding of users, I prefer they are kept so. Users have whatever > the possibility of reducing these names. And we will have to discuss > together about such features for further improvements. Sure - this can be discussed later after merging. Others may also have a view and chime in at the appropriate time. I'm puzzled though: if these names are coming in from dice_eap.c, why does getColName() hardcode them, as in: return "Mixer/In:%02d\n(%s)" % (i+1, name) for example. If I understand you correctly, "name" here contains the "Mixer/In" via dice_eap.c, so you'd end up getting this twice in the label. Ultimately I think we'll want the prefix ("Mixer/In" in this example) to be configurable somehow. For most devices, "Mixer/In" will just appear to the user to be an overly complicated way of saying "Input". However, at the end of the day it's a cosmetic issue and I don't want to hold the merge up over this. > These bugs are sufficiently severe so as to be overcome before merging. > If they are solved by the enclosed patches, then I will merge > modifications rapidly. Although my testing has not been exhaustive it does appear that the showstopper bugs for RME have now all been addressed. With the exception of that one chunk from 0007 which fails to apply I think we're good to merge this to trunk now. That will give it wider testing and make ongoing development of it easier. Once it's merged it might be worth posting to ffado-devel (and even ffado-user) to let people explicitly know that this "per output tab" feature is available in svn. I expect it will be of interest to quite a few people. I really appreciate the time you've spent in getting the patch set to this stage, and look forward to the future enhancements you've mentioned and we've discussed. Regards jonathan |
From: Philippe C. <la-...@or...> - 2013-08-08 17:12:05
|
Le jeudi 08 août 2013 à 22:02 +0930, Jonathan Woithe a écrit : > Hi Phil > > > I committed some works around our last discussions as revision 2358 to > > 2365. > > I've had a quick look at the result. These look good. > > > There is still some work to do, especially enabling the transpose view > > of the matrix mixer. Also, I have to re-read our previous email exchange > > to see what is possibly missing. I should have some time for other fixes > > before the end of next week. > > No problem - I look forward to the next steps. > > > In between, there is of course a need for testing these changes, > > especially for the RME device you use to own. > > Sure. I have tested this briefly. I like the display of the fader value > under the per-output faders - that looks good. > > The biggest issue I see at present is probably due to a round-off error in > the coversion to/from dB. In the matrix mixer, if I select "3 dB", what I > end up with in the matrix cell is 2.2 dB. I also notice that for the RME, > which has a maximum (linear) setting of 0x8000, the matrix cells and > per-output faders insist on calling this 5.7 dB. Along similar lines, "0 > dB" maps to 0 correctly, "-3 dB" becomes -2.9 and "-20 dB" becomes > (correctly) -20. I haven't had a chance to look at the dB mapping function > but I expect that there's a roundoff effect in there, given that the > mappings used to be fine and 6 dB was the maximum displayed in the matrix > view. This bug should be fixed in revision 2367. > I've had a really quick look at the code and can't see any obvious reason > for the change in behaviour. > This is related to the interaction between the matrix cell and the corresponding slider in the per output view. Any change in the first one must propagate to the latter (and reversely) thus leading to a Qt signal, then entering a recursive process which stops when the values are strictly identical. Since both matrix cell and slider work in the dB space, there is a succession of dB to linear and linear to dB conversion we must ensure that they leads to a unique linear value. There is possibly something more robust to do (that matrix cell and slider exchange reduces to dB values for instance), I will try to think about this later. > I also discovered that the more complex widget hierarchy now in use broke > the mute and invert functionality. This was because the MixerNode's parent > was no longer the top-level MatrixMixer object, so the corresponding dBus > interface object couldn't be found via parent(). I've introduced a separate > member to keep track of this now, so any future widget changes won't upset > it. See revision 2366. > Oh, sorry: but your new implementation looks like more robust and flexible, so it is a clear improvement and will make easier the further devel I planned. > Regards > jonathan Best regards, Phil -- Philippe Carriere <la-...@or...> |
From: Jonathan W. <jw...@ju...> - 2013-08-13 12:01:53
|
Hi Phil On Thu, Aug 08, 2013 at 07:11:52PM +0200, Philippe Carriere wrote: > > The biggest issue I see at present is probably due to a round-off error in > > the coversion to/from dB. In the matrix mixer, if I select "3 dB", what I > > end up with in the matrix cell is 2.2 dB. I also notice that for the RME, > > which has a maximum (linear) setting of 0x8000, the matrix cells and > > per-output faders insist on calling this 5.7 dB. Along similar lines, "0 > > dB" maps to 0 correctly, "-3 dB" becomes -2.9 and "-20 dB" becomes > > (correctly) -20. I haven't had a chance to look at the dB mapping function > > but I expect that there's a roundoff effect in there, given that the > > mappings used to be fine and 6 dB was the maximum displayed in the matrix > > view. > > This bug should be fixed in revision 2367. I can confirm that the issue is fixed for the RME as of this revision. Thanks for that. > This is related to the interaction between the matrix cell and the > corresponding slider in the per output view. Any change in the first one > must propagate to the latter (and reversely) thus leading to a Qt > signal, then entering a recursive process which stops when the values > are strictly identical. Since both matrix cell and slider work in the dB > space, there is a succession of dB to linear and linear to dB conversion > we must ensure that they leads to a unique linear value. > There is possibly something more robust to do (that matrix cell and > slider exchange reduces to dB values for instance), I will try to think > about this later. Sure. At present what we have works again so I think it's fine to leave it sit for a while. Although this wasn't part of the most recent change set, I wonder whether it's appropriate to clamp the matrix display to a minimum of -40 dB when the control can in fact go less than that. Again, this is a minor point and is probably related to the resolution of the fader widget. It's something to think about for later, when the more pressing things have been dealt with. Regards jonathan |
From: Jonathan W. <jw...@ju...> - 2013-05-23 11:43:29
|
Hi Philippe On Wed, May 15, 2013 at 07:21:47PM +0200, Philippe Carriere wrote: > > > 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. > > > Enclosed are the corresponding patches. > There was something not satisfying in the first patch, i.e. the router > part would have to know about the mixer encoding. I corrected this in > the second patch. I'm finally getting a chance to go over some of this stuff. I'm a little confused about the patchs you've sent through, the ordering (if relevant) and so on. Would you be able to resend a definitive patch set that I can use to test against the RME devices? Of primary importance is probably the matrixmixer changes; the others are smaller, simpler, and close to being ready for merging already I suspect. By way of feedback, the automatic routing changes patch for ffado-mixer looks reasonable at first glance. It does touch the matrixmixer widget but on devices which don't force routing changes in response to various things I expect these to be transparent. It is definitely a useful feature to have and it can probably be merged sooner rather than later. Apologies for the inconvenience - it's what happens when you can't get to these things for a week or so. :-( Regards jonathan |
From: Philippe C. <la-...@or...> - 2013-05-24 14:08:05
|
Le jeudi 23 mai 2013 à 21:13 +0930, Jonathan Woithe a écrit : > Hi Philippe > Hi Jonathan, > > On Wed, May 15, 2013 at 07:21:47PM +0200, Philippe Carriere wrote: > > > > 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. > > > > > > Enclosed are the corresponding patches. > > There was something not satisfying in the first patch, i.e. the router > > part would have to know about the mixer encoding. I corrected this in > > the second patch. > > I'm finally getting a chance to go over some of this stuff. I'm a little > confused about the patchs you've sent through, the ordering (if relevant) > and so on. Would you be able to resend a definitive patch set that I can > use to test against the RME devices? Of primary importance is probably the > matrixmixer changes; the others are smaller, simpler, and close to being > ready for merging already I suspect. > Enclosed are the patches in the relevant order. I tested them today against the last version of Adi git repos which seems to be almost up to date. Hope it will work. > By way of feedback, the automatic routing changes patch for ffado-mixer > looks reasonable at first glance. It does touch the matrixmixer widget but > on devices which don't force routing changes in response to various things I > expect these to be transparent. It is definitely a useful feature to have > and it can probably be merged sooner rather than later. > > Apologies for the inconvenience - it's what happens when you can't get to > these things for a week or so. :-( > > Regards > jonathan I am now at home and able to improve the modifications following your recommendations. Regards, Phil -- Philippe Carriere <la-...@or...> |
From: Philippe C. <la-...@or...> - 2013-06-23 12:46:36
|
Hi Jonathan, following our previous discussions to further improve the matrixmixer widget, I need we agree together on some general guidelines I give a first elaboration hereafter. Of course, feel free to modify/suppress/add anything you are wondering about. 1.a Fix the rule that columns are inputs and rows are output in libffado. ffado-mixer and further the user himself will decide how is represented the matrix in his own view. There is no need to change the name of the interface function: to make things clear, I will probably keep the indirect functions getInName and others in ffado-mixer, which will reduce to the call to interface.getColName and so on. - such a rule requires no modification for RME. The modifications required for DICE EAP devices are easy to implement. - it avoids unnecessary complications. - the argument Columns_are_inputs then just refer to the default view (for DICE EAP, it is preferable that the old convention is kept in apparence for the user). 1.b The full name for each row and column should be exclusively the one given by the libffado interface. - This requires, for DICE EAP devices, to complete the name sent by interface.getColName and getRowName. No complicated implementations required once this is decided. - The shortened naming In:.. or Out:.. will remain as a choice for the user. - As well hidden rows/columns remain with their simple numbering. 1.c The present font size button will be replaced by a more complete pop up menu (including the font size change or the use of shortened names. 2. An alternative for hiding "per output tab", as well as matrix view (in an exclusive OR framework) has to be implemented, including a default as an argument of MatrixMixer. 3. There is a need to unify the linear/decibel conversion for the different kind of sliders, the present implementation being rather a "hack" (the way round-off errors are taking into account). Deciding for a specific accuracy (typically +-0.1dB, as in ardour for instance - which look like the limit of human perception) would be of help. 4. There is a need to introduce something like interface.getMaxValue and interface.getMinValue (these ones being device dependent) to make MatrixMixer of more general applicability. Also, ultimately, libffado would have to provide informations about the linear/dB nature of the values as well as some reference when linear/dB is required. - This is possibly more complicated that appears at a glance. Probably post-poned at the very end of modifications. Best regards, Phil -- Philippe Carriere <la-...@or...> |
From: Jonathan W. <jw...@ju...> - 2013-06-23 13:33:57
|
Hi Phil This will probably be a quick response since it's getting late here. Not everything I write will necessarily make sense. :-) > 1.a Fix the rule that columns are inputs and rows are output in > libffado. ffado-mixer and further the user himself will decide how is > represented the matrix in his own view. There is no need to change the > name of the interface function: to make things clear, I will probably > keep the indirect functions getInName and others in ffado-mixer, which > will reduce to the call to interface.getColName and so on. > > - such a rule requires no modification for RME. The modifications > required for DICE EAP devices are easy to implement. To be honest I'm not sure that this is any different to what is currently the case. The underlying dbus matrixmixer control makes no assumptions at all about what its rows and columns represent. This is one of the reasons that getColName() and getRowName() exist. Ultimately there is embedded knowledge of what a row and column represents when a matrixmixer dbus control is put in place, and it's up to the ffado-mixer UI (or any other UI which interfaces to this) to work out the best way to represent the control visually. So to summarise I can't see what is being changed here: it's just restating what the present situation is. As far as I can tell libffado doesn't have a rule "that columns are inputs and rows are output". Rows and columns can already be whatever is required to suit a particular purpose. The main reason this has come up is that the "per output tab" implementation obviously requires that ffado-mixer have prior knowledge of whether rows or columns are considered outputs. There is really no way around this other than to have the mixer code explicitly tell the matrix mixer widget which case applies (that's the purpose which Columns_are_inputs etc serves). I guess an additional dbus control could be added so the driver itself communicates this information, but I don't thing the added complication is worth it in the long run. Given that as far as I can see the current situation already matches your idea, perhaps I've misunderstood something. Could you clarify? > - the argument Columns_are_inputs then just refer to the default view > (for DICE EAP, it is preferable that the old convention is kept in > apparence for the user). Again, isn't this pretty much the current sitation? > 1.b The full name for each row and column should be exclusively the one > given by the libffado interface. I have no problem with this. > - The shortened naming In:.. or Out:.. will remain as a choice for the > user. > - As well hidden rows/columns remain with their simple numbering. No problem. I can see that in certain situations users may well want to shorten the name as much as possible. > 1.c The present font size button will be replaced by a more complete pop > up menu (including the font size change or the use of shortened names. Agreed. Plus whatever other display options might turn up. > 2. An alternative for hiding "per output tab", as well as matrix view > (in an exclusive OR framework) has to be implemented, including a > default as an argument of MatrixMixer. Yes. There could well be uses of the matrix mixer for which the "per output tab" makes no sense. Being able to hide these tabs has obvious benefits. > 3. There is a need to unify the linear/decibel conversion for the > different kind of sliders, the present implementation being rather a > "hack" (the way round-off errors are taking into account). > Deciding for a specific accuracy (typically +-0.1dB, as in ardour for > instance - which look like the limit of human perception) would be of > help. What "different kind of sliders" are there at the moment? All present controls relate to volumes, making them all volume faders. Are you thinking of future requirements? This will require more thought. One fairly significant issue is that at the hardware level fader values are often expressed using a linear scale. This immediately means that the resolution of the faders is not constant when in dB space. If one implemented high-resolution faders (to 0.1 dB for example), most devices would appear very jumpy at low dB values because the hardware will only change volume at 3 dB intervals or there abouts. It turns out that the present implementation of the matrix cells is the closest match to what most hardware presently does. This is not to say that future hardware won't be different in this respect. Ultimately any change along these lines will have to change the external API used by the matrix mixer widget and the associated dbus control. Currently values of each cell are an integer between 0 and 65535 with 16384 (I think, from memory) corresponding to 0 dB. Upping the resolution in dB space will therefore require changes within ffado-mixer modules as well as each driver which provides a matrix mixer dbus control (since it's the driver which has to take the cell value reported via dbus and translate this into a number which makes sense for the hardware in question). While we ponder this issue, a useful addition to the sliders would be a pop-up text label which tells the user what dB value is currently set by the slider. Or maybe a text box under the fader which reports this (and which optionally permits direct entry of a dB value). > 4. There is a need to introduce something like interface.getMaxValue and > interface.getMinValue (these ones being device dependent) to make > MatrixMixer of more general applicability. This kind of relates to item 3 as it affects the external API. To make this a truly generic control, one would need to ditch the assumption that values should be displayed in dB and provide a method to display in some arbitary format. Implementing the transform between the value used in dbus and that which is displayed to the user but therefore be moved to a virtual function. This could be overridden in a descendent object type if (for example) dB was not applicable. Such an approach may do away with the need for getMaxValue() and getMinValue(); the range of values reported via dbus could be standardised and converting back to hardware-specific ranges would be solely the responsibility of the driver. However, there would still be a need for the matrixmixer widget to know the user-visible range applicable to a control, but I don't yet have a view as to whether this is something which should be communicated via dbus. This in turn is related to your final point. > Also, ultimately, libffado would have to provide informations about the > linear/dB nature of the values as well as some reference when linear/dB > is required. > - This is possibly more complicated that appears at a glance. Probably > post-poned at the very end of modifications. I think there's a real danger of making the matrixwidget too complex. The detail as to how dbus data values are mapped to user-visible values can be dependent on the underlying hardware and trying to second-guess every possibility is ultimately impossible. A better approach I think is to abstract these sorts of details into virtual methods, with the default methods implementing the most common case (the existing dB scale). Then if any devices come along which require something different the matrixmixer widget could simply be subclassed within the ffado-mixer module for that device and the relevant details provided in new virtual methods in the subclass. Regards jonathan |
From: Jonathan W. <jw...@ju...> - 2013-05-25 11:46:22
|
Hi Phil > > I'm finally getting a chance to go over some of this stuff. I'm a little > > confused about the patchs you've sent through, the ordering (if relevant) > > and so on. Would you be able to resend a definitive patch set that I can > > use to test against the RME devices? Of primary importance is probably the > > matrixmixer changes; the others are smaller, simpler, and close to being > > ready for merging already I suspect. > > Enclosed are the patches in the relevant order. I tested them today > against the last version of Adi git repos which seems to be almost up to > date. Hope it will work. Thanks for this. I tested the 0001 patch (enhanced matrix mixer) which patched head fine, although one chunk had a fuzz value of 2: patching file support/mixer-qt4/ffado/widgets/matrixmixer.py Hunk #5 succeeded at 398 with fuzz 2 I haven't looked into why. It's undoubtedly a minor thing. I've run the revised matrixmixer widget against the FF800 as a test. It worked better than I expected to be honest, and didn't crash even in the "single row" case. The existing scrollbars still work fine. Good stuff - it's a solid base to build on. The following are some comments to guide further development; many of these overlap what we have previously covered. 1) The font size used for the channel names in the matrix view is too small to read. The font resizing code will definitely need some revision. 2) Probably the most significant new issue is the realisation that the use of columns and rows for the RME is not the same as for the Saffire. The new code assumes that each column represents an output and each row an input. However, the RME druver uses the matrix mixer in a manner that's closer to a traditional monitor mixer board: each column is an input channel, and each row represents an output. This means that the present code's "output" tabs are actually showing faders for a particular input. To fix this, we really need a way to tell the matrix mixer what is represented by a row: an input or an output. The tab code could then adapt accordingly. 3) There is a small issue in the "single row" case used by the RME mixer to represent output master fader values. Contrary to the "input" and "playback" mixers, here each "column" is an output so each "tab" correctly includes a single fader. However, there's something funny going on. At the top of the "Out:01" tab for example, I have: Mixer/Out:01 (Line out 1) followed by an empty frame. This is in fact the label used for the first column in the matrix view. Under that, in a smaller font I have: Mixer/In:01 () Then comes another empty frame, and finally (under that) is the fader. Something's not quite right here. 4) For completeness, here's what I'm seeing for the 28x28 "input mixer", with reference to the "Out:01" tab. There's a blank space at the top of the tab, followed by an empty frame which spans the width of the tab area. Under that come the respective faders. These show: Mixer/In:01 (Line out 1) an empty frame the width of the channel strip, and then the fader itself. All in all, I'm not sure what the empty frames are meant to be. This example also highlights the switching of the meaning of rows and columns in the RME mixer. The "Out:01" tab shows the faders from input 1 to each output - so it's really a per-input view rather than a per-output view. 5) To deal with the single-row case, having the option to disable the per output tabs would be useful I think because in this case they don't provide anything that the standard matrix view doesn't. This is a low priority though: it could always be added after the initial patch is merged. I like the user channel naming ability this patch adds - it will require load/save functionality before it becomes truly useful but we definitely need this. All in all, this patch greatly enhances ffado-mixer for devices which make use of the matrixmixer widget. Once the rough edges have been dealt with I think it can go in. > I am now at home and able to improve the modifications following your > recommendations. Sounds good - I look forward to seeing the revision of this and the other two (which deal with the automatic mixer adjustments in response to routing changes on the relevant interfaces). Regards jonathan |
From: Philippe C. <la-...@or...> - 2013-05-25 17:02:17
|
Le samedi 25 mai 2013 à 21:16 +0930, Jonathan Woithe a écrit : > Hi Phil > Hi Jonathan, > Thanks for this. I tested the 0001 patch (enhanced matrix mixer) which > patched head fine, although one chunk had a fuzz value of 2: > > patching file support/mixer-qt4/ffado/widgets/matrixmixer.py > Hunk #5 succeeded at 398 with fuzz 2 > > I haven't looked into why. It's undoubtedly a minor thing. > Possibly the git repos is not exactly up to date. > I've run the revised matrixmixer widget against the FF800 as a test. It > worked better than I expected to be honest, and didn't crash even in the > "single row" case. The existing scrollbars still work fine. Good stuff - > it's a solid base to build on. The following are some comments to guide > further development; many of these overlap what we have previously covered. > So I restart the job from zero (with the help of yet existing patches, of course). Enclosed is the first patch: per Output view is not yet implemented here. I will do it over the present modifications. > 1) The font size used for the channel names in the matrix view is too small > to read. The font resizing code will definitely need some revision. > Corrected. Now the user can choose the font size, from his environment default one to smaller values. Also included a "0" font size which replace the channel name by its number; this is for people who would complain about too long names :-) > 2) Probably the most significant new issue is the realisation that the use > of columns and rows for the RME is not the same as for the Saffire. The > new code assumes that each column represents an output and each row an > input. However, the RME druver uses the matrix mixer in a manner that's > closer to a traditional monitor mixer board: each column is an input > channel, and each row represents an output. This means that the present > code's "output" tabs are actually showing faders for a particular input. > This is the point I tried to solve first. Effectively, for DICE EAP there is an implicit rule which was present largely before I began to modify the code. > To fix this, we really need a way to tell the matrix mixer what is > represented by a row: an input or an output. The tab code could then > adapt accordingly. So I included an argument to MatrixMixer which represents the rule: inputs to outputs may be West to North or North to West depending on the developer choice (note I referred to PyQT by using West and North rather than Left and Top; it could be modified if you preferred. With this patch applied, you should (if I did not mistake somewhere, of course) be able to see the usual matrix view, with In and Out specified, and the ability of changing font size (or almost hide the channel names). It works for the Saffire, does it for the RME (note I modified rme.py with the assumption you use a North to West rule). I am pursuing the work for pet Output view, but at that point I am wondering about something confusing in the present implementation of ffado. ffado-mixer asks the interface for column and row names (as well as for column and row counts and so on): if we plan to implement a choice for the user (West to North versus North to West) there will be much confusion with this. I think libffado should only know about In and Out names/counts/... and locating them as rows or columns should only depend on ffado-mixer. The only required rule should be the one for coefficient access (in versus out number as the first argument for instance). Of course this requires some changes inside libffado, but since only DICE EAP and RME devices are presently involved, it could be easier to do now than in some future where possibly additional devices could use matrixmixer.py. What is your opinion about this ? Best regards, Phil -- Philippe Carriere <la-...@or...> |
From: Philippe C. <la-...@or...> - 2013-06-23 15:32:11
|
Hi Jonathan, Le dimanche 23 juin 2013 à 23:03 +0930, Jonathan Woithe a écrit : > Hi Phil > > This will probably be a quick response since it's getting late here. Not > everything I write will necessarily make sense. :-) > > > 1.a Fix the rule that columns are inputs and rows are output in > > libffado. ffado-mixer and further the user himself will decide how is > > represented the matrix in his own view. There is no need to change the > > name of the interface function: to make things clear, I will probably > > keep the indirect functions getInName and others in ffado-mixer, which > > will reduce to the call to interface.getColName and so on. > > > > - such a rule requires no modification for RME. The modifications > > required for DICE EAP devices are easy to implement. > > To be honest I'm not sure that this is any different to what is currently > the case. Presently, RME returns input informations for any call to getCol..., DICE EAP return output informations. Let me recall I had to correct dice_eap.cpp some months ago because there were bugs related to this kind of confusion, so this is clearly a source of confusion. In the present revision, this yet induced complexification of the implementation in ffado-mixer. Trying to add a choice of the user will lead to additional complexity and probably much confusion for any further devel. If DICE EAP implementation is modified for conforming the same rule than RME, this leads to many simplifications. > The underlying dbus matrixmixer control makes no assumptions at > all about what its rows and columns represent. This is one of the reasons > that getColName() and getRowName() exist. dbus is not involved here. How getColName() and getRowName() (among others) are implemented in libffado is the question. What is the present usefulness that RME and DICE EAP implement opposite rules while sharing the same UI except adding complexity, and much more complexity for a user choice to be provided ? > Ultimately there is embedded > knowledge of what a row and column represents when a matrixmixer dbus > control is put in place, and it's up to the ffado-mixer UI (or any other UI > which interfaces to this) to work out the best way to represent the control > visually. > > So to summarise I can't see what is being changed here: it's just restating > what the present situation is. As far as I can tell libffado doesn't have a > rule "that columns are inputs and rows are output". Rows and columns can > already be whatever is required to suit a particular purpose. > The main reason this has come up is that the "per output tab" implementation > obviously requires that ffado-mixer have prior knowledge of whether rows or > columns are considered outputs. There is really no way around this other > than to have the mixer code explicitly tell the matrix mixer widget which > case applies (that's the purpose which Columns_are_inputs etc serves). I > guess an additional dbus control could be added so the driver itself > communicates this information, but I don't thing the added complication is > worth it in the long run. > > Given that as far as I can see the current situation already matches your > idea, perhaps I've misunderstood something. Could you clarify? > DSP mixer have inputs and outputs which are what libffado is aware of; in other words, "rows" and "columns" make no sense for libffado which should only be able to interface something like getInCount(), getInName(), etc ... "Rows" and "columns" just refer to a particular graph the ffado-mixer (or any interface) developer choose to represent the different controls. Now, for some historical reasons, the interface functions are named getRowCount(), getRowName(), etc ..., so ffado-mixer does not know which ones are the inputs and which ones are the outputs since they are presently implemented differently depending on the device. And there is no usefulness for such different implementations which additionally lead to confusion (and the past experience shows that it is really confusing) and make the ffado-mixer implementation yet more complex. In view of implementing a choice for the user, I find that simplifying at first is largely desirable and lead to no functionality loss. > > - the argument Columns_are_inputs then just refer to the default view > > (for DICE EAP, it is preferable that the old convention is kept in > > apparence for the user). > > Again, isn't this pretty much the current sitation? > No, presently "Columns_are_inputs" must match what is implemented in the libffado interface functions: typically, using Columns_are_inputs for DICE EAP devices would lead to the wrong behaviour. > > 1.b The full name for each row and column should be exclusively the one > > given by the libffado interface. > > I have no problem with this. > > > - The shortened naming In:.. or Out:.. will remain as a choice for the > > user. > > - As well hidden rows/columns remain with their simple numbering. > > No problem. I can see that in certain situations users may well want to > shorten the name as much as possible. > > > 1.c The present font size button will be replaced by a more complete pop > > up menu (including the font size change or the use of shortened names. > > Agreed. Plus whatever other display options might turn up. > > > 2. An alternative for hiding "per output tab", as well as matrix view > > (in an exclusive OR framework) has to be implemented, including a > > default as an argument of MatrixMixer. > > Yes. There could well be uses of the matrix mixer for which the "per output > tab" makes no sense. Being able to hide these tabs has obvious benefits. > > > 3. There is a need to unify the linear/decibel conversion for the > > different kind of sliders, the present implementation being rather a > > "hack" (the way round-off errors are taking into account). > > Deciding for a specific accuracy (typically +-0.1dB, as in ardour for > > instance - which look like the limit of human perception) would be of > > help. > > What "different kind of sliders" are there at the moment? All present > controls relate to volumes, making them all volume faders. Are you thinking > of future requirements? > Presently, three different linear/dB conversion are implemented in MatrixMixer at different locations, which round-off differently: this is indeed a problem because the "sliders" interact the one with the other. There is whatever a need to change the situation: at least, each conversion should be a call to the same conversion function to avoid any undesirable recursion. > This will require more thought. One fairly significant issue is that at the > hardware level fader values are often expressed using a linear scale. This > immediately means that the resolution of the faders is not constant when in > dB space. If one implemented high-resolution faders (to 0.1 dB for > example), most devices would appear very jumpy at low dB values because the > hardware will only change volume at 3 dB intervals or there abouts. It > turns out that the present implementation of the matrix cells is the closest > match to what most hardware presently does. Presently, the faders of matrix view apparently have a 0.01 dB resolution (in some cases, for a reason I don't presently know, even larger resolution can be encountered: typically a number like 0.00134 in the spinbox !). The faders in per output tab have only a 1dB resolution: so I was rather asking for a more limited resolution for faders ! > Regards, Phil -- Philippe Carriere <la-...@or...> |
From: Jonathan W. <jw...@ju...> - 2013-06-24 00:04:52
|
Hi Phil > > > 1.a Fix the rule that columns are inputs and rows are output in > > > libffado. ffado-mixer and further the user himself will decide how is > > > represented the matrix in his own view. There is no need to change the > > > name of the interface function: to make things clear, I will probably > > > keep the indirect functions getInName and others in ffado-mixer, which > > > will reduce to the call to interface.getColName and so on. > > > > > > - such a rule requires no modification for RME. The modifications > > > required for DICE EAP devices are easy to implement. > > > > To be honest I'm not sure that this is any different to what is currently > > the case. > > Presently, RME returns input informations for any call to getCol..., > DICE EAP return output informations. That's the choice of the respective drivers. Libffado does not enforce any particular interpretation as to what a "row" or "column" represents. In many respects the physical meaning of the rows and columns is dictated by the names returned via getColName() and getRowName(). > Let me recall I had to correct dice_eap.cpp some months ago because > there were bugs related to this kind of confusion, so this is clearly a > source of confusion. I apologise but I can't recall the specific "correction" you're referring to here. Could you give me an svn revision number so I can refresh my memory? > If DICE EAP implementation is modified for conforming the same rule than > RME, this leads to many simplifications. I don't believe the DICE EAP driver (or any other) should be forced to adopt a particular convention as to the meaning of rows and columns within the dbus data structure. > > The underlying dbus matrixmixer control makes no assumptions at > > all about what its rows and columns represent. This is one of the reasons > > that getColName() and getRowName() exist. > > dbus is not involved here. How getColName() and getRowName() (among > others) are implemented in libffado is the question. dbus is the interface that ffado-mixer interfaces to. ffado-mixer does not link against libffado or use it directly. Every interaction with the underlying device driver is via dbus. interface.getColName() and friends translate into dbus calls which are served by ffado-dbus-server. > What is the present usefulness that RME and DICE EAP implement opposite > rules while sharing the same UI except adding complexity, and much more > complexity for a user choice to be provided ? Again, I've lost track of a few things I think. Which "user choice" are you referring to here - the user of ffado-mixer, or the choice of the person implementing ffado-mixer modules? > DSP mixer have inputs and outputs which are what libffado is aware of; > in other words, "rows" and "columns" make no sense for libffado which > should only be able to interface something like getInCount(), > getInName(), etc ... > "Rows" and "columns" just refer to a particular graph the ffado-mixer > (or any interface) developer choose to represent the different controls. > Now, for some historical reasons, the interface functions are named > getRowCount(), getRowName(), etc ..., so ffado-mixer does not know which > ones are the inputs and which ones are the outputs since they are > presently implemented differently depending on the device. And there is > no usefulness for such different implementations which additionally lead > to confusion (and the past experience shows that it is really > confusing) and make the ffado-mixer implementation yet more complex. Ok, I think I see what the problem is. The implementation of the basic matrix mixer control does not necessarily have to be used for an m-by-n mixer. Instead, it was architected in a way that it could be used for any group of controls which were best represented as either a 1D or 2D array. There could be use cases where the rows and columns have nothing to do with inputs or outputs. As a result, the underlying implementation adopts the generic "row" and "column" naming convention. If the foundation layer switches to an input/output convention then any use of the matrix mixer for controls which do not conform to the input-output model will be confusing; code will refer to "inputs" and "outputs" while the underlying physical interpretation will be something quite different. By way of example, consider a device which includes gain controls across 8 analog channels. A matrix mixer could be used to implement this. In this case, columns could well be inputs, but the "row" is definitely not an output. In the above you wrote: > ... so ffado-mixer does not know which ones are the inputs and which ones > are the outputs ... It's more generic than that: ffado-mixer at the lowest level doesn't know whether the input/output model is even applicable for a given matrix mixer. Knowledge such as this is currently embedded within the ffado-mixer module which creates the matrixmixer widget. I reiterate that all of this was of academic interest until the "per output" tab thing came into being. Up to that point ffado-mixer did not need to know what rows and columns corresponded to because it relied on the get{Row,Col}Name() functions to return appropriate labels. Obviously with the per-output tabs you now need this specific knowledge within ffado-mixer. However, I think that forcing the input/output model onto the lower level of the matrix mixer interface would be a mistake. > In view of implementing a choice for the user, I find that simplifying > at first is largely desirable and lead to no functionality loss. Making the underlying matrixmixer objects specific to a particular use case, while not necessarily being a loss of functionality, will contribute to confusion down the track. > > > - the argument Columns_are_inputs then just refer to the default view > > > (for DICE EAP, it is preferable that the old convention is kept in > > > apparence for the user). > > > > Again, isn't this pretty much the current sitation? > > No, presently "Columns_are_inputs" must match what is implemented in the > libffado interface functions: typically, using Columns_are_inputs for > DICE EAP devices would lead to the wrong behaviour. Ok, I see what you mean here when you say "default view". Note that having Columns_are_inputs matching the convention exported by libffado via dbus is exactly the sort of embedded knowledge I was referring to earlier. Anyway, I agree that this issue needs a neater solution, but defining the matrix dimensions to suit only one specific use case (an input/output mixer) is the wrong approach. The concept of rows/columns should remain because this makes the code more naturally reusable in cases which don't conform to the input/output model. Instead there may need to be a refactoring of code. At the libffado end, very little would change at the interface level. In ffado-mixer, the widget currently known as MatrixMixer would revert to functionality similar to what it had before: a generic matrix panel which required no knowledge of what the rows/columns represent. We would then subclass this to a new widget (InOutMatrixMixer for example) which added the per output tabs functionality (and whatever else is useful for an in/out mixer). Some functionality that's hard coded in MatrixMixer would need to be separated out into virtual functions which can then be overridden by descendents or called in a different context (the drawing of the cells would be one such candidate). With this in place there would be no problem in adopting a convention that an InOutMatrixMixer expects rows to be inputs (for example; the convention could just as easily require that columns be inputs if anyone cared). When a matrix mixer was used as an in/out mixer the only change on the libffado side would be to ensure the applicable matrix dimension was taken to be inputs, but this is trivial and easily seen to. So long as the InOutMatrixMixer provided for both views this should please everyone. This approach preserves the generic nature of the matrix mixer interface while still adding some conventions to make particular specific implementations cleaner. Note that the above is written assuming that python, being an object orientated language, allows the subclassing and virtual methods discussed above. If it doesn't then the MatrixMixer class would need to have the concept of a mode: generic, in/out, plus whatever else we come up with in due course. This is a very messy approach though and the object-based approach would be far better. It's just that right at this point in time my python knowledge doesn't stretch far enough to know what's possible. > > > 3. There is a need to unify the linear/decibel conversion for the > > > different kind of sliders, the present implementation being rather a > > > "hack" (the way round-off errors are taking into account). > > > Deciding for a specific accuracy (typically +-0.1dB, as in ardour for > > > instance - which look like the limit of human perception) would be of > > > help. > > > > What "different kind of sliders" are there at the moment? All present > > controls relate to volumes, making them all volume faders. Are you thinking > > of future requirements? > > Presently, three different linear/dB conversion are implemented in > MatrixMixer at different locations, which round-off differently: this is > indeed a problem because the "sliders" interact the one with the other. > There is whatever a need to change the situation: at least, each > conversion should be a call to the same conversion function to avoid any > undesirable recursion. Oh I see. In that case I agree: there should be one linear-dB conversion function and one dB-linear conversion function, with all conversion sites using one of these two. > > This will require more thought. One fairly significant issue is that at the > > hardware level fader values are often expressed using a linear scale. This > > immediately means that the resolution of the faders is not constant when in > > dB space. If one implemented high-resolution faders (to 0.1 dB for > > example), most devices would appear very jumpy at low dB values because the > > hardware will only change volume at 3 dB intervals or there abouts. It > > turns out that the present implementation of the matrix cells is the closest > > match to what most hardware presently does. > > Presently, the faders of matrix view apparently have a 0.01 dB > resolution (in some cases, for a reason I don't presently know, even > larger resolution can be encountered: typically a number like 0.00134 in > the spinbox !). As I understand it, this would take into account the effective resolution towards the high end of the control for those devices which use a linear scale for their faders. As mentioned, for all devices currently using the matrix mixer for faders, the possible values range from 0 to 65535 with the 0 dB point set at 16384 In any case, on this dB scale a linear value of 65000 corresponds to 11.9699 dB while 65001 gives 11.9700. To allow access to each physical fader step of such controls the resolution of the GUI control must be correspondingly small. This doesn't necessarily make it right - it just explains why the control resolution is seemingly overly small. We could of course redefine the matrix mixer control value to be related to dB rather than being linear when it's used in an InOutMatrixMixer widget (of course this would require changes in the libffado code which interprets the value coming back from the mixer). I'm not sure what the best approach might be here, but it may be something to deal with after the other issues have been sorted. > The faders in per output tab have only a 1dB resolution: so I was rather > asking for a more limited resolution for faders ! Ah, right, I see. The reason for the seemingly high resolution of the matrix cell controls is outlined above. Deciding on the most appropriate way to deal with this will require more consideration since it could be influenced by a number of other factors currently being discussed. Regards jonathan |
From: Jonathan W. <jw...@ju...> - 2013-07-12 11:04:53
|
Hi Phil Have you had any further thoughts regarding the future work needed on the matrix mixer we discussed a few weeks ago? I ask because I am wondering about the potential timing of a FFADO version 2.2. It would be ideal if a few of the loose edges at least were tidied up for that, but obviously the feasibility of this depends on how much time you have over the next little while. I am conscious of the fact that the fixes we were discussing will involve a fair amount of work. To reiterate, I think the matrix mixer at the lowest level needs to retain the notion of rows and columns so it can be used for more than just input-output matrices. A derived widget class could contain the per-output functionality, and could impose a particular convention for whether rows or columns represented inputs - so long as the widget gained the ability to flick the matrix view around between columns-as-input and rows-as-input modes, which I understand was your intention [1]. This would enable the code simplifications that you spoke about while retaining the generic nature in the underlying structure. Please let me know what you think at your convenience. Regards jonathan |
From: Jonathan W. <jw...@ju...> - 2013-05-26 12:15:08
|
Hi Phil > > 1) The font size used for the channel names in the matrix view is too small > > to read. The font resizing code will definitely need some revision. > > Corrected. Now the user can choose the font size, from his environment > default one to smaller values. > Also included a "0" font size which replace the channel name by its > number; this is for people who would complain about too long names :-) The font size displayed for me is much better now: it's readable. It appears to be only marginally smaller than that which was used originally. I don't have a problem with that. The "0" font size works as you described. In the dropdown I get a choice of 5 other sizes: 5 through 9, with 9 being the default. Selecting any size other than 0 does not affect the visual appearance of the labels. That is, the size used does not seem to change regardless of what size is selected. Only size 0 behaves differently. Ultimately we'll need to come up with a way to indicate what that dropdown does. I'm not sure how this can be done without messing up the layout: there's really not enough space for a label of any kind. The behaviour of "size 0" seems ok - for those who know what they want and don't care for long channel names it works a treat. My only thought was whether we should add "in" and "out" to the appropriate labels so the user can see immediately what the rows and colums correspond to. This wouldn't add any width to the controls since the matrix cells are already wider than what is needed to accommodate the extra characters. > > 2) Probably the most significant new issue is the realisation that the use > > of columns and rows for the RME is not the same as for the Saffire. The > > new code assumes that each column represents an output and each row an > > input. However, the RME druver uses the matrix mixer in a manner that's > > closer to a traditional monitor mixer board: each column is an input > > channel, and each row represents an output. This means that the present > > code's "output" tabs are actually showing faders for a particular input. > > This is the point I tried to solve first. Effectively, for DICE EAP > there is an implicit rule which was present largely before I began to > modify the code. > > > To fix this, we really need a way to tell the matrix mixer what is > > represented by a row: an input or an output. The tab code could then > > adapt accordingly. > > So I included an argument to MatrixMixer which represents the rule: > inputs to outputs may be West to North or North to West depending on the > developer choice (note I referred to PyQT by using West and North rather > than Left and Top; it could be modified if you preferred. I think the introduction of the north/west type nomenclature is a little confusing - it took me a little time to work out what it was referring to. In the present situation it is probably not needed: the new parameter could simply be a boolean called something like "columns_are_outputs". If set to "true" the columns are treated as outputs (as per the Saffire case); otherwise they're treated as inputs (for the RME). For clarity it could be stringified I guess (as you've done in the present patch). > With this patch applied, you should (if I did not mistake somewhere, of > course) be able to see the usual matrix view, with In and Out specified, > and the ability of changing font size (or almost hide the channel > names). It works for the Saffire, does it for the RME (note I modified > rme.py with the assumption you use a North to West rule). The patch as supplied does appear to work as intended with the RME. I'm seeing "Mixer/In" labels across the top, and "Mixer/Out" labels down the side. > I am pursuing the work for pet Output view, but at that point I am > wondering about something confusing in the present implementation of > ffado. > ffado-mixer asks the interface for column and row names (as well as for > column and row counts and so on): if we plan to implement a choice for > the user (West to North versus North to West) there will be much > confusion with this. > I think libffado should only know about In and Out names/counts/... and > locating them as rows or columns should only depend on ffado-mixer. The > only required rule should be the one for coefficient access (in versus > out number as the first argument for instance). > Of course this requires some changes inside libffado, but since only > DICE EAP and RME devices are presently involved, it could be easier to > do now than in some future where possibly additional devices could use > matrixmixer.py. What is your opinion about this ? I don't think there's major confusion, but things could certainly be made more flexible with a suggestion like you've described. In essence it redefines the dimensions of the matrix mixer to be "inputs" and "outputs" rather than "rows" and "columns". Which dimension was "inputs" becomes arbitary as the layout choice becomes an issue for ffado-mixer. The benefit I see here is that the choice between "columns as inputs" or "columns as outputs" could be selected by the user with a widget. That way the preference of the programmer isn't forced on the user. Those used to monitor boards can have columns as inputs, which those for whom "columns as outputs" makes more sense are also accommodated. This may also lead to a solution to the problem identified earlier about the font size control. We could have a small section at the top of the mixer containing layout type options. "Label size" could be one control, the input/output layout could be another. Alternatively, these options could be put into a popup menu that's activated with a button that's put where you've currently got the font size dropdown (it could be called "layout options" or something similar). Having written that, I prefer the button-activated popup menu. So on the whole, moving to the abstract input/output thing seems to make sense. There's only one issue that I can see: the underlying matrixmixer dbus control is not always used in conjunction with matrixmixer widgets in ffado-mixer. Sometimes it is used to group related controls together to cut down on the number of dbus controls needed. The MOTU driver makes extensive use of this usage pattern, for example. In these contexts, the concept of "input" and "output" doesn't actually make sense - the more generic "row" and "column" are better. It was for this reason that the matrixmixer dbus control was implemented with dimensions labelled row/column: it keeps the control generic. This leaves us in a bit of a bind. I would rather not alter the row/column concept of the dbus control object because doing so will loose the generic nature of that control type. However, the implications of the sort of change you suggested are attractive for the reasons given above. A potential way forward might be to decouple the row/column concept of the dbus matrixmixer control from the row/column arrangement used by ffado-mixer. In other words, arrange things so that what ffado-mixer displays as rows/columns is not necessarily the same as what the dbus control manipulates as rows/columns. This could be done like this: 1) Decree that when a matrixmixer dbus control is used by a matrixmixer widget, the columns/rows will be assumed to be inputs/outputs (or the other way around - it's a fairly arbitary choice in the end). 2) The matrixmixer widget can then operate on the dbus control with knowledge of the row/column convention. 3) This can only really work if the matrixmixer widget has the "display columns are inputs" and "display columns are outputs" layout modes, since both modes are useful in certain circumstances. An alternative approach to option 1 would be to include another parameter in the matrixmixer dbus control alongside the row/column counts (and the like): a control which indicates whether the dbus control columns are inputs or outputs. This would still permit ffado-mixer to operate as discussed above since it would know what to treat as outputs. It's also more explicit: option 1 requires that you "just know" the convention. However, the resulting code may be more complex than if option 1 were pursued. I would be interested in your further thoughts on this subject in light of the above. Apologies for the length. :-) Regards jonathan |
From: Philippe C. <la-...@or...> - 2013-05-27 07:01:30
|
Le dimanche 26 mai 2013 à 21:44 +0930, Jonathan Woithe a écrit : > Hi Phil Hi Jonathan, > > > > 1) The font size used for the channel names in the matrix view is too small > > > to read. The font resizing code will definitely need some revision. > > > > Corrected. Now the user can choose the font size, from his environment > > default one to smaller values. > > Also included a "0" font size which replace the channel name by its > > number; this is for people who would complain about too long names :-) > > The font size displayed for me is much better now: it's readable. It > appears to be only marginally smaller than that which was used originally. > I don't have a problem with that. > > The "0" font size works as you described. > OK. > In the dropdown I get a choice of 5 other sizes: 5 through 9, with 9 being > the default. Selecting any size other than 0 does not affect the visual > appearance of the labels. That is, the size used does not seem to change > regardless of what size is selected. Only size 0 behaves differently. Strange: here it works well. I imagine you obtain no message from ffado-mixer: is the behaviour environment dependent ? I mean 9 being your default font size, does your environment contains smaller font size for the font in use ? > Ultimately we'll need to come up with a way to indicate what that dropdown > does. I'm not sure how this can be done without messing up the layout: > there's really not enough space for a label of any kind. > Presently, because free space is too small, information is given as a tooltip. > The behaviour of "size 0" seems ok - for those who know what they want and > don't care for long channel names it works a treat. My only thought was > whether we should add "in" and "out" to the appropriate labels so the user > can see immediately what the rows and colums correspond to. This wouldn't > add any width to the controls since the matrix cells are already wider than > what is needed to accommodate the extra characters. Done in the enclosed patch. > > > > 2) Probably the most significant new issue is the realisation that the use > > > of columns and rows for the RME is not the same as for the Saffire. The > > > new code assumes that each column represents an output and each row an > > > input. However, the RME druver uses the matrix mixer in a manner that's > > > closer to a traditional monitor mixer board: each column is an input > > > channel, and each row represents an output. This means that the present > > > code's "output" tabs are actually showing faders for a particular input. > > > > This is the point I tried to solve first. Effectively, for DICE EAP > > there is an implicit rule which was present largely before I began to > > modify the code. > > > > > To fix this, we really need a way to tell the matrix mixer what is > > > represented by a row: an input or an output. The tab code could then > > > adapt accordingly. > > > > So I included an argument to MatrixMixer which represents the rule: > > inputs to outputs may be West to North or North to West depending on the > > developer choice (note I referred to PyQT by using West and North rather > > than Left and Top; it could be modified if you preferred. > > I think the introduction of the north/west type nomenclature is a little > confusing - it took me a little time to work out what it was referring to. > In the present situation it is probably not needed: the new parameter could > simply be a boolean called something like "columns_are_outputs". If set to > "true" the columns are treated as outputs (as per the Saffire case); > otherwise they're treated as inputs (for the RME). For clarity it could be > stringified I guess (as you've done in the present patch). > Corrected in th enclosed patch. > > With this patch applied, you should (if I did not mistake somewhere, of > > course) be able to see the usual matrix view, with In and Out specified, > > and the ability of changing font size (or almost hide the channel > > names). It works for the Saffire, does it for the RME (note I modified > > rme.py with the assumption you use a North to West rule). > > The patch as supplied does appear to work as intended with the RME. I'm > seeing "Mixer/In" labels across the top, and "Mixer/Out" labels down the > side. > OK. > > I am pursuing the work for pet Output view, but at that point I am > > wondering about something confusing in the present implementation of > > ffado. > > ffado-mixer asks the interface for column and row names (as well as for > > column and row counts and so on): if we plan to implement a choice for > > the user (West to North versus North to West) there will be much > > confusion with this. > > I think libffado should only know about In and Out names/counts/... and > > locating them as rows or columns should only depend on ffado-mixer. The > > only required rule should be the one for coefficient access (in versus > > out number as the first argument for instance). > > Of course this requires some changes inside libffado, but since only > > DICE EAP and RME devices are presently involved, it could be easier to > > do now than in some future where possibly additional devices could use > > matrixmixer.py. What is your opinion about this ? > > I don't think there's major confusion, but things could certainly be made > more flexible with a suggestion like you've described. In essence it > redefines the dimensions of the matrix mixer to be "inputs" and "outputs" > rather than "rows" and "columns". Which dimension was "inputs" becomes > arbitary as the layout choice becomes an issue for ffado-mixer. > > The benefit I see here is that the choice between "columns as inputs" or > "columns as outputs" could be selected by the user with a widget. That way > the preference of the programmer isn't forced on the user. Those used to > monitor boards can have columns as inputs, which those for whom "columns as > outputs" makes more sense are also accommodated. > > This may also lead to a solution to the problem identified earlier about the > font size control. We could have a small section at the top of the mixer > containing layout type options. "Label size" could be one control, the > input/output layout could be another. Alternatively, these options could be > put into a popup menu that's activated with a button that's put where you've > currently got the font size dropdown (it could be called "layout options" or > something similar). Having written that, I prefer the button-activated > popup menu. > > So on the whole, moving to the abstract input/output thing seems to make > sense. There's only one issue that I can see: the underlying matrixmixer > dbus control is not always used in conjunction with matrixmixer widgets in > ffado-mixer. Sometimes it is used to group related controls together to cut > down on the number of dbus controls needed. The MOTU driver makes extensive > use of this usage pattern, for example. In these contexts, the concept of > "input" and "output" doesn't actually make sense - the more generic "row" > and "column" are better. It was for this reason that the matrixmixer dbus > control was implemented with dimensions labelled row/column: it keeps the > control generic. > > This leaves us in a bit of a bind. I would rather not alter the row/column > concept of the dbus control object because doing so will loose the generic > nature of that control type. However, the implications of the sort of > change you suggested are attractive for the reasons given above. A > potential way forward might be to decouple the row/column concept of the > dbus matrixmixer control from the row/column arrangement used by > ffado-mixer. In other words, arrange things so that what ffado-mixer > displays as rows/columns is not necessarily the same as what the dbus > control manipulates as rows/columns. This could be done like this: > > 1) Decree that when a matrixmixer dbus control is used by a matrixmixer > widget, the columns/rows will be assumed to be inputs/outputs (or the > other way around - it's a fairly arbitary choice in the end). > > 2) The matrixmixer widget can then operate on the dbus control with > knowledge of the row/column convention. > > 3) This can only really work if the matrixmixer widget has the "display > columns are inputs" and "display columns are outputs" layout modes, > since both modes are useful in certain circumstances. > > An alternative approach to option 1 would be to include another parameter in > the matrixmixer dbus control alongside the row/column counts (and the like): > a control which indicates whether the dbus control columns are inputs or > outputs. This would still permit ffado-mixer to operate as discussed above > since it would know what to treat as outputs. It's also more explicit: > option 1 requires that you "just know" the convention. However, the > resulting code may be more complex than if option 1 were pursued. > > I would be interested in your further thoughts on this subject in light of > the above. Apologies for the length. :-) > I'll send you an answer later (now, it is time for job). > Regards > jonathan Regards, Phil -- Philippe Carriere <la-...@or...> |
From: Philippe C. <la-...@or...> - 2013-06-12 19:52:42
|
Hi Jonathan, enclosed three patches I would like you to test on your RME device (note the first one is the compilation of the two I sent you some days ago; in other words, these patches apply to the present state of the svn version). Except bugs, it should work for you except probably the font size change which works fine here: without being able to reproduce the bug, I can not do much more. Also, there is possibly something concerning your "hack" for one of the control widget for RME. Note also that I did not yet introduce the editable labels you have seen in previous patches. I await for developing file saving for mixer/router/monitoring settings. If no rough bug is present for RME, I propose to introduce the modifications. There will be a lot of improvement further but, having tested them in a real situation (8 simultaneous voices recording) as they stand, I can confirm they may be very useful under some circumstances. Regards, Phil Le lundi 27 mai 2013 à 09:01 +0200, Philippe Carriere a écrit : > Le dimanche 26 mai 2013 à 21:44 +0930, Jonathan Woithe a écrit : > > > Hi Phil > > Hi Jonathan, > > > > > > > > 1) The font size used for the channel names in the matrix view is too small > > > > to read. The font resizing code will definitely need some revision. > > > > > > Corrected. Now the user can choose the font size, from his environment > > > default one to smaller values. > > > Also included a "0" font size which replace the channel name by its > > > number; this is for people who would complain about too long names :-) > > > > The font size displayed for me is much better now: it's readable. It > > appears to be only marginally smaller than that which was used originally. > > I don't have a problem with that. > > > > The "0" font size works as you described. > > > > OK. > > > > In the dropdown I get a choice of 5 other sizes: 5 through 9, with 9 being > > the default. Selecting any size other than 0 does not affect the visual > > appearance of the labels. That is, the size used does not seem to change > > regardless of what size is selected. Only size 0 behaves differently. > > > Strange: here it works well. I imagine you obtain no message from > ffado-mixer: is the behaviour environment dependent ? I mean 9 being > your default font size, does your environment contains smaller font > size for the font in use ? > > > > Ultimately we'll need to come up with a way to indicate what that dropdown > > does. I'm not sure how this can be done without messing up the layout: > > there's really not enough space for a label of any kind. > > > > Presently, because free space is too small, information is given as a > tooltip. > > > > The behaviour of "size 0" seems ok - for those who know what they want and > > don't care for long channel names it works a treat. My only thought was > > whether we should add "in" and "out" to the appropriate labels so the user > > can see immediately what the rows and colums correspond to. This wouldn't > > add any width to the controls since the matrix cells are already wider than > > what is needed to accommodate the extra characters. > > Done in the enclosed patch. > > > > > > > > 2) Probably the most significant new issue is the realisation that the use > > > > of columns and rows for the RME is not the same as for the Saffire. The > > > > new code assumes that each column represents an output and each row an > > > > input. However, the RME druver uses the matrix mixer in a manner that's > > > > closer to a traditional monitor mixer board: each column is an input > > > > channel, and each row represents an output. This means that the present > > > > code's "output" tabs are actually showing faders for a particular input. > > > > > > This is the point I tried to solve first. Effectively, for DICE EAP > > > there is an implicit rule which was present largely before I began to > > > modify the code. > > > > > > > To fix this, we really need a way to tell the matrix mixer what is > > > > represented by a row: an input or an output. The tab code could then > > > > adapt accordingly. > > > > > > So I included an argument to MatrixMixer which represents the rule: > > > inputs to outputs may be West to North or North to West depending on the > > > developer choice (note I referred to PyQT by using West and North rather > > > than Left and Top; it could be modified if you preferred. > > > > I think the introduction of the north/west type nomenclature is a little > > confusing - it took me a little time to work out what it was referring to. > > In the present situation it is probably not needed: the new parameter could > > simply be a boolean called something like "columns_are_outputs". If set to > > "true" the columns are treated as outputs (as per the Saffire case); > > otherwise they're treated as inputs (for the RME). For clarity it could be > > stringified I guess (as you've done in the present patch). > > > > Corrected in th enclosed patch. > > > > > With this patch applied, you should (if I did not mistake somewhere, of > > > course) be able to see the usual matrix view, with In and Out specified, > > > and the ability of changing font size (or almost hide the channel > > > names). It works for the Saffire, does it for the RME (note I modified > > > rme.py with the assumption you use a North to West rule). > > > > The patch as supplied does appear to work as intended with the RME. I'm > > seeing "Mixer/In" labels across the top, and "Mixer/Out" labels down the > > side. > > > > OK. > > > > > I am pursuing the work for pet Output view, but at that point I am > > > wondering about something confusing in the present implementation of > > > ffado. > > > ffado-mixer asks the interface for column and row names (as well as for > > > column and row counts and so on): if we plan to implement a choice for > > > the user (West to North versus North to West) there will be much > > > confusion with this. > > > I think libffado should only know about In and Out names/counts/... and > > > locating them as rows or columns should only depend on ffado-mixer. The > > > only required rule should be the one for coefficient access (in versus > > > out number as the first argument for instance). > > > Of course this requires some changes inside libffado, but since only > > > DICE EAP and RME devices are presently involved, it could be easier to > > > do now than in some future where possibly additional devices could use > > > matrixmixer.py. What is your opinion about this ? > > > > I don't think there's major confusion, but things could certainly be made > > more flexible with a suggestion like you've described. In essence it > > redefines the dimensions of the matrix mixer to be "inputs" and "outputs" > > rather than "rows" and "columns". Which dimension was "inputs" becomes > > arbitary as the layout choice becomes an issue for ffado-mixer. > > > > The benefit I see here is that the choice between "columns as inputs" or > > "columns as outputs" could be selected by the user with a widget. That way > > the preference of the programmer isn't forced on the user. Those used to > > monitor boards can have columns as inputs, which those for whom "columns as > > outputs" makes more sense are also accommodated. > > > > This may also lead to a solution to the problem identified earlier about the > > font size control. We could have a small section at the top of the mixer > > containing layout type options. "Label size" could be one control, the > > input/output layout could be another. Alternatively, these options could be > > put into a popup menu that's activated with a button that's put where you've > > currently got the font size dropdown (it could be called "layout options" or > > something similar). Having written that, I prefer the button-activated > > popup menu. > > > > So on the whole, moving to the abstract input/output thing seems to make > > sense. There's only one issue that I can see: the underlying matrixmixer > > dbus control is not always used in conjunction with matrixmixer widgets in > > ffado-mixer. Sometimes it is used to group related controls together to cut > > down on the number of dbus controls needed. The MOTU driver makes extensive > > use of this usage pattern, for example. In these contexts, the concept of > > "input" and "output" doesn't actually make sense - the more generic "row" > > and "column" are better. It was for this reason that the matrixmixer dbus > > control was implemented with dimensions labelled row/column: it keeps the > > control generic. > > > > This leaves us in a bit of a bind. I would rather not alter the row/column > > concept of the dbus control object because doing so will loose the generic > > nature of that control type. However, the implications of the sort of > > change you suggested are attractive for the reasons given above. A > > potential way forward might be to decouple the row/column concept of the > > dbus matrixmixer control from the row/column arrangement used by > > ffado-mixer. In other words, arrange things so that what ffado-mixer > > displays as rows/columns is not necessarily the same as what the dbus > > control manipulates as rows/columns. This could be done like this: > > > > 1) Decree that when a matrixmixer dbus control is used by a matrixmixer > > widget, the columns/rows will be assumed to be inputs/outputs (or the > > other way around - it's a fairly arbitary choice in the end). > > > > 2) The matrixmixer widget can then operate on the dbus control with > > knowledge of the row/column convention. > > > > 3) This can only really work if the matrixmixer widget has the "display > > columns are inputs" and "display columns are outputs" layout modes, > > since both modes are useful in certain circumstances. > > > > An alternative approach to option 1 would be to include another parameter in > > the matrixmixer dbus control alongside the row/column counts (and the like): > > a control which indicates whether the dbus control columns are inputs or > > outputs. This would still permit ffado-mixer to operate as discussed above > > since it would know what to treat as outputs. It's also more explicit: > > option 1 requires that you "just know" the convention. However, the > > resulting code may be more complex than if option 1 were pursued. > > > > I would be interested in your further thoughts on this subject in light of > > the above. Apologies for the length. :-) > > > > I'll send you an answer later (now, it is time for job). > > > > Regards > > jonathan > > Regards -- Philippe Carriere <la-...@or...> |
From: Philippe C. <la-...@or...> - 2013-06-12 19:53:54
|
-- Philippe Carriere <la-...@or...> |
From: Philippe C. <la-...@or...> - 2013-06-12 19:54:50
|
-- Philippe Carriere <la-...@or...> |
From: Philippe C. <la-...@or...> - 2013-07-17 18:17:43
|
Hi Jonathan, sorry for delay, I was on vacations last two weeks with no easy web connexions, and life became instantaneously busy right after :-) Le vendredi 12 juillet 2013 à 20:34 +0930, Jonathan Woithe a écrit : > Hi Phil > > Have you had any further thoughts regarding the future work needed on the > matrix mixer we discussed a few weeks ago? > > I ask because I am wondering about the potential timing of a FFADO version > 2.2. It would be ideal if a few of the loose edges at least were tidied up > for that, but obviously the feasibility of this depends on how much time you > have over the next little while. I am conscious of the fact that the fixes > we were discussing will involve a fair amount of work. > OK, version 2.2 is a good idea since a lot of new features and support has been provided this year. So, I will focus on the few points which require some fix, implementing on the existing with no major changes. More fundamental changes, if required, will be postponed to the next 2.3 (in 2014 ...:-). I should have more times during the first three weeks of August, and hopefully be able to do so. > To reiterate, I think the matrix mixer at the lowest level needs to retain > the notion of rows and columns so it can be used for more than just > input-output matrices. A derived widget class could contain the per-output > functionality, and could impose a particular convention for whether rows or > columns represented inputs - so long as the widget gained the ability to > flick the matrix view around between columns-as-input and rows-as-input > modes, which I understand was your intention [1]. This would enable the code > simplifications that you spoke about while retaining the generic nature in > the underlying structure. Yes, I understood that you need something which more generally could be called a "matrix controls" widget, with maximal degrees of flexibility for the developer. Ideally, "matrix mixer" itself would be a call to this "matrix controls" with just its own specific settings. However, this will require much works and tests, so, as I said previously, I will probably postpone them for version 2.3. > > Please let me know what you think at your convenience. > > Regards > jonathan Best regards, Phil -- Philippe Carriere <la-...@or...> |
From: Jonathan W. <jw...@ju...> - 2013-07-26 11:25:27
|
Hi Phil > sorry for delay, I was on vacations last two weeks with no easy web > connexions, and life became instantaneously busy right after :-) I understand. I hope you had a good break. > > I ask because I am wondering about the potential timing of a FFADO version > > 2.2. It would be ideal if a few of the loose edges at least were tidied up > > for that, but obviously the feasibility of this depends on how much time you > > have over the next little while. I am conscious of the fact that the fixes > > we were discussing will involve a fair amount of work. > > OK, version 2.2 is a good idea since a lot of new features and support has > been provided this year. So, I will focus on the few points which require > some fix, implementing on the existing with no major changes. > : > I should have more times during the first three weeks of August, and > hopefully be able to do so. That sounds like a good plan. That also gives me time to see if I can coax some audio out of a MOTU 828 (the original) - I've been loaned one for a while but it's proving to be surprisingly difficult to crack in this regard (curiously recording works fine). > More fundamental changes, if required, will be postponed to the next 2.3 > (in 2014 ...:-). I would sincerely hope you don't have to wait until 2014 to do them. If we push out a 2.2 I would not want to delay it too long. Once the first round of your matrix fixes are in place I'd be keen to get 2.2 released so we can move onto more invasive changes sooner rather than later. > Yes, I understood that you need something which more generally could be > called a "matrix controls" widget, with maximal degrees of flexibility > for the developer. Ideally, "matrix mixer" itself would be a call to > this "matrix controls" with just its own specific settings. However, > this will require much works and tests, so, as I said previously, I will > probably postpone them for version 2.3. Great - sounds good. Regards jonathan |
From: Jonathan W. <jw...@ju...> - 2013-06-13 13:24:10
|
Hi Phil > enclosed three patches I would like you to test on your RME device (note > the first one is the compilation of the two I sent you some days ago; in > other words, these patches apply to the present state of the svn > version). No problem. I'll see what I can do in the next few days or over the weekend. > Except bugs, it should work for you except probably the font size change > which works fine here: without being able to reproduce the bug, I can > not do much more. I'll check this and let you know what I see. > Also, there is possibly something concerning your "hack" for one of the > control widget for RME. I assume you're referring to the size issue on the outputmixer control. Ultimately I would love to drop the hack. However, I have already spent several hours trying to get Qt to do what I want and it steadfastly refuses to do so. The general problem is to ensure that rows of a matrix mixer don't grow beyond a certain size because tall rows look ridiculous. This can happen if there are a low number of rows. We'll see what the new code produces and take things from there. > Note also that I did not yet introduce the editable labels you have seen > in previous patches. I await for developing file saving for > mixer/router/monitoring settings. Sure, that makes sense. Those two features are clearly related. > If no rough bug is present for RME, I propose to introduce the > modifications. I have no problem with your plan. I will review the patches and test them with the RME as soon as possible. > ... I can confirm they may be very useful under some circumstances. Indeed - I am very happy to see these features become available. Regards jonathan |