From: Julie S <msj...@ya...> - 2010-08-20 02:58:48
|
Hello All, I believe I fixed Will's audio problem he was having in Committed revision 12024. I reactivated the old FirstUnusedID code and deactivated the new code. The new code used a static variable and started the ID count at 0. Unfortunately, this causes a big issue (duplicate ID). When loading a file Audio ID's are taken from the rg file itself and the Last ID counter is not incremented in this process. Therefore, when RG is exited and restarted, the first audio file added always gets ID 0 even if it was already used. There are other problems with the "new" code. The question is what do we want to do about it? * We could make the new code more robust and update the unused counter accordingly after file load and before the user is able to add new files. * We could remove the new code and just use the old code (basically what I have done in Committed revision 12024). * We could improve the old code to be more efficient. The current code in O(n) with n = the number of audio files in the composition. Let's talk. If I don't get much feed back on this...I will eventually just erase the deactivated code and call it good. Sincerely, Julie S. |
From: D. M. M. <mic...@ro...> - 2010-08-20 06:43:27
|
On Thursday, August 19, 2010, Julie S wrote: > Let's talk. If I don't get much feed back on this...I will eventually just > erase the deactivated code and call it good. Chris probably did the new stuff that was supposed to have been an improvement. He's been absent for a good half dozen or more things that normally would have piqued his interest, and I don't expect him to show up anytime soon. I was just a few minutes ago musing on how in spite of the fact that I am utterly and indescribably bored in my spare time these days, I simply have no desire to work on Rosegarden or anything else. You have the conn Ms. Swango, and whatever you decide is fine by me. There are no wrong answers. -- D. Michael McIntyre |
From: Chris C. <ca...@al...> - 2010-08-21 18:25:33
|
On Fri, Aug 20, 2010 at 7:43 AM, D. Michael McIntyre <mic...@ro...> wrote: > On Thursday, August 19, 2010, Julie S wrote: > >> Let's talk. If I don't get much feed back on this...I will eventually just >> erase the deactivated code and call it good. > > Chris probably did the new stuff that was supposed to have been an > improvement. Hm, I don't recognise it (from the diff). But that isn't necessarily conclusive... let's check the records. (goes to do that) Chris |
From: Chris C. <ca...@al...> - 2010-08-21 18:29:05
|
On Sat, Aug 21, 2010 at 7:25 PM, Chris Cannam <ca...@al...> wrote: > On Fri, Aug 20, 2010 at 7:43 AM, D. Michael McIntyre > <mic...@ro...> wrote: >> On Thursday, August 19, 2010, Julie S wrote: >> >>> Let's talk. If I don't get much feed back on this...I will eventually just >>> erase the deactivated code and call it good. >> >> Chris probably did the new stuff that was supposed to have been an >> improvement. > > Hm, I don't recognise it (from the diff). svn annotate: 11070 emrum I'll make an earnest attempt to work out what's what with this code during this week. In the meantime, if anyone has any thoughts on (a) what was wrong with the old code that inspired Emanuel to introduce this or (b) what he was aiming at -- or indeed if someone else has already worked out a clean fix that they can properly justify -- please speak up. Chris |
From: Julie S <msj...@ya...> - 2010-08-21 18:48:32
|
Hello Chris, Concerning: > svn annotate: > > 11070 emrum > I don't think there was anything wrong with the old code that I reactivated. My guess is that "emrum" thought he could knock out an easy O(1) update to an O(n) algorithm. I know the O(1) algorithm to be flawed in several ways. I think we could improve the O(n) algorithm to an an O(1) amortized algorithm without out too much overhead. But we have these kinds of O(n) functions all over RG, so I'm not too inclined to start converting them unless we want to scrutinize the code set as a whole, or for some reason this particular piece of code is proving to create significant degradation of functionality. That is my argument. Sincerely, Julie S. |
From: Chris C. <ca...@al...> - 2010-08-21 18:56:04
|
On Sat, Aug 21, 2010 at 7:48 PM, Julie S <msj...@ya...> wrote: > My guess is that "emrum" thought he could knock out an easy O(1) update to an O(n) algorithm. Hm. Yes, well -- in this case O(n) is fine. It has never been a bottleneck and it probably never will be. I _think_ the original code was written by Richard, who used to be quite aggressive about that kind of principle -- if an easy O(n) algorithm was good enough in practice, he'd use it every time. I would probably have tried to be more clever, and in this case the result would probably have been less appropriate. Chris |
From: Chris C. <ca...@al...> - 2010-08-21 20:58:13
|
On Sat, Aug 21, 2010 at 7:55 PM, Chris Cannam <ca...@al...> wrote: > On Sat, Aug 21, 2010 at 7:48 PM, Julie S <msj...@ya...> wrote: >> My guess is that "emrum" thought he could knock out an easy O(1) update to an O(n) algorithm. > > Hm. > > Yes, well -- in this case O(n) is fine. It has never been a > bottleneck and it probably never will be. Hang on, we're doing him a disservice here. The commit log said: "bug fixes you can drop multi selections (of files/URIs) on the AudioFileList now ProgessDialog fixed for drop events insertion without dragging is functional" I at first read the commit log but didn't read the whole diff for the commit, and I had assumed that this particular change was additional to the stuff listed in the log. But in fact, this change is the only thing in the diff. It's possible that the log was in error (e.g. written for the wrong commit) but the likelihood is that this change did fix something. And thinking about "you can drop multi selections", that makes sense. If you drop many things at once, the insertion code will (presumably -- I'm not reading the code here) want to get new audio file IDs for all of them at once. But the way getFirstUnusedID works, it will return the first ID that is not of an audio file that has been inserted into the composition -- which means it will return the same ID for each new file, since none of them has been inserted yet. Of course, the fix is itself broken because its IDs don't persist across file save/load (as we saw). What we need it seems is for a getNewAudioFileID function that returns an ID guaranteed not to be in the current composition and also guaranteed not to have been returned by any previous call to getNewAudioFileID during this run of the program. Also, is this function called by any undoable commands? If so, do they assume that the file can be added, removed, and added again using the same ID as it had before? (In which case we need to also ensure that "new" IDs handed out elsewhere between undo and redo differ from any IDs that were in the composition when it was loaded but may have been removed from it since?) Or do they allocate a new ID for the audio file on each redo? (Which would likely be broken in many other ways, since segments owned by other commands in the stack are likely to have references to the audio file ID.) Chris |
From: Julie S <msj...@ya...> - 2010-08-21 21:49:43
|
Hello Chris, > > Yes, well -- in this case O(n) is fine. It has never > been a > > bottleneck and it probably never will be. > > Hang on, we're doing him a disservice here. Didn't intend to...just looking at it with a narrow len is all. I didn't research why the correction was made. I see that the audio manager only lets the user select one item to be dragged from the dialog....so that wasn't what you are talking about. So I checked the out adding multiple files from the add file dialog...and they all took on the same id This was also true when I dragged and dropped from my file browser into rg So that was what Emmanuel's code was attempting to do, was track these multiple calls before insertion. So now we have two partial solutions...hmmm ... I see a couple options * Use Emmanuel's idea of a static variable but add a setter type function, that when files are loaded the audio ID is sent setter function. This setter function will determine if the ID passed is greater than the Id it is tracking. If so then update the ID to the next available ID (passed ID + 1). * One file load we could just track Audio ID tags as we read them keeping the maximum. Then we could set the next Available to max + 1 (variation on a theme). * We could use Emmanuel's code during file load and try to remap all audio segment ID to the values obtained from Emmanuel's code. This has the advantage of letting us keep ID number low. I can think of more, but they are pretty obvious variations on keeping a list or max value kinds of things. Sincerely, Julie S. |
From: Chris C. <ca...@al...> - 2010-08-22 08:08:39
|
On Sat, Aug 21, 2010 at 10:49 PM, Julie S <msj...@ya...> wrote: > I see a couple options > * Use Emmanuel's idea of a static variable but add a setter type function, that when files are loaded the audio ID is sent setter function. This setter function will determine if the ID passed is greater than the Id it is tracking. If so then update the ID to the next available ID (passed ID + 1). That would seem reasonable. > * We could use Emmanuel's code during file load and try to remap all audio segment ID to the values obtained from Emmanuel's code. > > This has the advantage of letting us keep ID number low. Is there any reason why we should prefer ID numbers to be low (or contiguous)? A traditional alternative, if we really don't care about what the numbers actually are, would be to make them random, or hashed on some basis. Chris |
From: Julie S <msj...@ya...> - 2010-08-22 12:50:42
|
Hello Chris, Concerning: > Is there any reason why we should prefer ID numbers to be > low (or contiguous)? No not really. > A traditional alternative, if we really don't care about > what the > numbers actually are, would be to make them random, or > hashed on some > basis. We'll we still have to check the hash against collisions, therefore we'd still need to access a list or set. ... I'm taking a first stab at combining the two halves of the solution and see what you think. Sincerely, Julie S. |
From: Julie S <msj...@ya...> - 2010-08-22 13:14:01
|
Hello Chris, I just put a solution in the repository. Committed revision 12026. See what you think. Drag and Drop of multiple files still does not work when the destination is the RG Main Canvas. I did not look at the code for that. I'm guessing this is a different issue. Out of time today for this though. Drag and drop to the Audio Manager is fine. Sincerely, Julie S. |
From: Chris C. <ca...@al...> - 2010-08-23 14:42:42
|
On Sun, Aug 22, 2010 at 2:13 PM, Julie S <msj...@ya...> wrote: > I just put a solution in the repository. Committed revision 12026. > > See what you think. Looks about right. Thinking about it, the static variable should probably be a non-static member of the AudioFileManager class. Since it depends on the m_audioFiles members of the AudioFileManager alone, that would avoid any problems which might happen in principle if we ever ended up with more than one AudioFileManager object. That also suggests, in turn, that we could just update the last audio file ID member from within the manager object each time the m_audioFiles list is modified (i.e. if the new file's ID is greater than the last audio file ID, set the last audio file ID to it). Then there would be no need to call resetAudioFileID at all. I should probably have thought of that sooner. Does it make sense, though? Chris |
From: Julie S <msj...@ya...> - 2010-08-23 14:55:02
|
Hello Chris, Concerning: > Thinking about it, the static variable should probably be a > non-static > member of the AudioFileManager class. Since it > depends on the > m_audioFiles members of the AudioFileManager alone, that > would avoid > any problems which might happen in principle if we ever > ended up with > more than one AudioFileManager object. I already updated the code to non-static as of my last commit. So we were thinking alike. The static reference was from the 'erumf' code. But I agree. It's done it is now stored in m_lastAudioFileID Concerning > That also suggests, in turn, that we could just update the > last audio > file ID member from within the manager object each time > the > m_audioFiles list is modified (i.e. if the new file's ID is > greater > than the last audio file ID, set the last audio file ID to > it). Then > there would be no need to call resetAudioFileID at all. > > I should probably have thought of that sooner. Does > it make sense, though? Yes, that is dead simple. I will investigate. And commit a change if able. Sincerely, Julie S. |
From: Chris C. <ca...@al...> - 2010-08-23 20:21:12
|
On Mon, Aug 23, 2010 at 3:54 PM, Julie S <msj...@ya...> wrote: > Yes, that is dead simple. I will investigate. And commit a change if able. I saw your change, and Will's comment -- looks good! thank you for following this through. Chris |