Re: [Audacity-devel] problems with recent MIDI code
A free multi-track audio editor and recorder
Brought to you by:
aosiniao
From: Roger D. <rb...@cs...> - 2011-02-17 20:03:14
|
> On 17/02/2011 15:08, Roger Dannenberg wrote: >> Just to be clear, there are USE_MIDI, EXPERIMENTAL_MIDI_OUT, and >> EXPERIMENTAL_SCORE_ALIGN. I mention this because I believe the short >> term plan (1.3.13) is USE_MIDI on and EXPERIMENTAL_MIDI_OUT off. > I am confused then. > > I'd thought USE_MIDI was off in SVN HEAD and assumed it will be off in > 1.3.13 - since I do not get the option to add a note track in the menu. > However I see config_win.h has '#define USE_MIDI 1. > > So we include it so that people can import a MIDI track.??? OK. > > Maybe I am less confused now. > > I see "gswillia on Jul 17, 2008" enabled USE_MIDI. USE_MIDI means "implement NoteTracks". I think the plan is to USE_MIDI in 1.3.13. This switch is not part of Experimental.h, but instead gets set by configuration files included by Audacity.h. > > >>> What I've understood is that >>> >>> * There are a few actual very minor regressions. The most >>> significant being that we do a full repaint of TrackPanel when >>> we previously did not do so. >>> >> >> I believe there are also many changes and fixes to the score >> representation code (allegro, aka portsmf). These would affect >> NoteTracks (enabled by USE_MIDI). > > Please read my comment about regressions again. I can see your patch > does a great deal of good for MIDI. I am saying (my understanding is) > that it introduces a regression in the repainting of TrackPanel. We > now repaint more area than we need to. I think I was confused by the term "regression" and because the repaint issue, as far as I know, is orthogonal to the rest of this discussion. You can avoid some repainting by reverting to the old behavior where there was less repainting and where the redisplay was not completely correct, e.g. the yellow "selected" box was damaged by repainting the animated playback position. I'd be happy to be corrected, but I think the choices (independent of my patch) are: (1) re-introduce the "yellow selection box gets damaged bug" in return for a bit less redisplay work, (2) properly repaint the yellow selection box in return for a bit more redisplay work, (3) significant change to redisplay, which has a lot of room for improvement -- i.e. we could fix the bugs AND do much less repainting at the cost of a lot of code changes. > > If that is only in the EXPERIMENTAL_MIDI_OUT case then at this stage > it just needs a comment in the code. If it's going to affect 1.3.13 > it needs fixing. The fix could be (1), (2), or (3) (see above). Or I could be wrong and someone might see an easier, better way to fix the redisplay. >>> >>> * There are some infelicities in the source code which we would >>> like to clean up at some time. Cut and paste code and the >>> like. This is particularly true of how the #ifdeffed out MIDI >>> affects MixerBoard. >>> >> I don't understand this comment. If there's no EXPERIMENTAL_MIDI_OUT, >> there are no volume controls for NoteTracks, and NoteTracks do not >> appear in MixerBoard, which I think has always been the case. > I was trying to say that (I understood that) the source code after > applying the patch has bad style - cut and paste code and the like. > Functionality is not changed for the no EXPERIMENTAL_MIDI_OUT case, > but the source code is now more messy. This is something we would like > at some time to clean up. Oh, I see. I'm happy to clean up all the style issues after applying the patch. I think it's important to do these in sequence because if cleaning up the code introduces any bugs, we'll have an "uncleaned" reference to go back to. While the patched code will be more messy (temporarily), it is very faithful to pre-r10680. > > I was basing my assumptions here on the dialogue between you and > Vaughan, not examination of > http://www.cs.cmu.edu/~music/audacity/r10680/final-diffs.txt . In > the detailed discussion between you and Vaughan there were points you > and Vaughan agreed on, on ways the source code could be better. One > example was subclassing of MixerTrackCluster for NoteTracks. It's > these infelicities I'd like //TODO notes on. [Can be added after the > patch is applied] Certainly. Beyond that, we should decide whether to do this reorganization pre-or-post 1.3.13. > >>> If I have got this right, my preference is that you fix the minor >>> regressions (or back out your patch) and that you add //TODO >>> comments to the most serious infelicities in the main source code >>> introduced as part of MIDI work. I believe nothing actually stops >>> us sorting out those infelicities before 1.3.13, but it is not >>> required that we do. >> I spent a lot of time constructing the patch that I am now proposing. > Yes. That is why I prefer that the patch goes-in/stays-in. (I'd > misunderstood and thought the > http://www.cs.cmu.edu/~music/audacity/r10680/final-diffs.txt patch had > already been applied to HEAD). > > If the patch is introducing new regressions, such as excess repainting > mentioned earlier, and these will affect 1.3.13, then really we must > fix them, (meaning you must fix them). At the risk of being too redundant, if "you must fix them" means go back to the pre-r10680 repainting style, that's easily done, but re-introduces bug 255. (Now I'm having some doubts -- I see bug 255 is relatively recent, but I think this has been around for a long time and determined it is unrelated to the changes I made. I can look at r10679 or earlier to confirm this.) > >> The patch does two things: (1) it fixes r10680 so that those changes >> are properly within #ifdef's -- if you apply the patch to r10680 and >> turn off EXPERIMENTAL_MIDI_OUT and _SCOREALIGN, you get just bug >> fixes to r10679 under USE_MIDI; (2) Since the patch does not actually >> remove r10680, all the bug fixes that have been made with r10680 in >> place (by everyone) should still be valid. Of course, there were some >> cases of problems being fixed twice, but all cases I know of showed >> up as SVN conflicts as expected and have been resolved. > That sounds very safe. > >> I think this is better than your preferences because "fix the minor >> regressions" requires identifying them, but it's a big task to filter >> through and understand every code change. > Again we're at cross purposes on which regressions we are talking > about. I am talking about any regressions introduced by the patch > that users of 1.3.13 will see, not past regressions that it fixes (all > MIDI features on). > >> My patch was also a very big task, but I was able to use diff and the >> C preprocessor to provide some automatic correctness checking. "Back >> out your patch (assuming you mean r10680)" is possible, but then >> subsequent changes that were made and tested on top of r10680 could >> break -- I don't have a good feel for the risk here and maybe it's my >> imagination, but it doesn't seem like a good idea to me. >>> If there are major or many regressions, or if the infelicities are >>> actually substantial in code outside the hidden-in-#ifdeffed-bits, >>> then my preferences change. >> Maybe "infelicities" is too diplomatic. I'm not sure if you mean >> bugs, bad style, or missing features or all of these. I think the >> main issue is bad style driven by a desire to not change tested code >> close to a release. > Infelicities = bad style (and I understand the cause). It just needs > //TODOs in the code, in my opinion. The //TODO comments can be added > after applying the patch. Thanks. > >> I'm certainly in favor of a decision -- I think we had one in >> November which resulted in the current patch. I did recently respond >> to all of Vaughan's questions below -- I'm not sure if you saw it, >> but I can send the long message to audacity-devel or any individuals. > I did see it, and that is what prompted my message. > It could be helpful to post it here too. Will do. > > I am well aware there is a lot of work in the patch. That's why I > prefer it to go in. It will create a lot more work to delay it. > > If it goes in and we have USE_MIDI #undeffed, it is still better, in > my opinion, than not having it in SVN. > > --James. > > Overall, I think the biggest question is about the redisplay. If it would help, I can double-check that the problem is unrelated to the patch, and also try to get some idea of the performance impact. |