From: Tim M. <or...@le...> - 2013-02-10 15:21:35
Attachments:
fix_submaster_count_patch
|
> On Sun, 20 Jan 2013, Tim Munro wrote: > >> Holger Marzen wrote: >> >>> By trying some tricks with grouping I noticed that I can't set the audio >>> mixer to 8 submasters. Rosegarden crashes immediately. >> Hi Holger. >> >> There is a special trick involved here. It isn't a fix, but it works for me, >> as I've been too lazy to fix it right. >> >> When you open the audio mixer and try to set the number of submasters, the >> control will initially indicate "No Submasters," regardless of how many >> submasters are actually present. >> >> Click on "No Submasters" even though the control appears to indicate this >> already. The existing submasters will disappear. >> >> Now when you reopen the Settings menu, you should be able to set eight >> submasters. > Indeed. Works for me. Thanks for the hint. > > But it would be cool if someone could fix this UI-type bug. OK. I've taken another look at this issue, and here's what I've found. Changing the number of submasters involves requesting copies of existing plugins at src/gui/studio/AudioMixerWindow.cpp, line 1421: dups.push_back(new Buss(*busses[i + 1])); The resulting copies have, however, been defective, resulting in a crash at src/document/RosegardenDocument.cpp, line 1038: plugin->getIdentifier().c_str()); The program was puking when it sucked up garbage instead of the expected string. I've dealt with the issue by cobbling together an ugly, complicated copy constructor for the PluginContainer class. The resulting code appears to work, but I have no idea what sort of memory leaks or other future train wrecks I might have introduced while beating it into submission. I've also added some minor tweaks to ensure the "Number of Stereo Inputs" and "Number of Submasters" menu checkboxes correctly display the initial state of a file. Tim Munro |
From: Ted F. <te...@te...> - 2013-02-10 19:29:17
|
On 02/10/2013 10:21 AM, Tim Munro wrote: > I've dealt with the issue by cobbling together an ugly, complicated copy > constructor for the PluginContainer class. > > The resulting code appears to work, but I have no idea what sort of memory > leaks or other future train wrecks I might have introduced while beating > it into submission. It's a good idea to avoid the need for a custom copy ctor if at all possible. Copy ctors are tricky to get right. If I had time, I'd take a look at this situation and see if there are alternatives. Instead, I'll just pontificate on copy ctors for a bit. Ignore if you've heard it all before.... The first alternative that I usually try for is to remove the need for a copy ctor altogether. This can be achieved at least two ways: don't copy the object, or make everything in the object copy itself. Why is this plugin container being copied in the first place? It sounds like that would be a less than desirable thing to do (potentially expensive in terms of CPU and memory). Is there a way to change the design so that no copy is necessary? You can find every place where a copy occurs by making your copy ctor private. (Unless, of course, this is so botched up that the object is copying itself. Yikes! In that case, leaving it declared but undefined will cause the linker to complain.) Once these copies have been identified, an attempt can be made to remove them. Making everything in the object copy itself can be achieved in many ways. One way is to make everything an object instead of having pointers. Another is to use smart pointers (like shared_ptr) which provide a reference counting mechanism. In the end, though, a copy ctor might indeed be the right thing to do (and yours might be exactly what is needed). In that case make sure that the copy ctor copies *everything* in a way that makes sense. It must create an object that is identical to the original in every way (read through the regular ctors and the member objects to create a checklist of things you need to do in your copy ctor). Also, make sure you declare an operator=(). You can make it private to prevent anyone from using it. You can also leave it undefined so you'll get a linker error in case the class itself uses it. Having a copy ctor and no operator=() is almost always an error. (Actually implementing an operator= is a bit tricky as it must deal properly with self-assignment.) For more, read up on the Rule of Three: https://en.wikipedia.org/wiki/Rule_of_three_%28C%2B%2B_programming%29 (Which in C++11 is now the Rule of Five what with the new move ctor and move assignment.) Another great source is Lippman's C++ Primer 5th ed. (2013) Chapter 13. The crash symptoms you describe sound like a case of a class that has pointer members (directly or indirectly), does a delete in the dtor, but has no copy ctor or op= defined. Then somebody does a copy (or assignment). This is the perfect recipe for a crash. The Rule of Three should never be taken lightly like this. Ted. |
From: Tom B. (Tehom) <te...@pa...> - 2013-02-10 22:34:57
|
Ted: Good comments on copy ctors. > Having a copy ctor and no > operator=() is almost always an error. (Actually implementing an > operator= is a bit tricky as it must deal properly with self-assignment.) We've had a few bugs because copy ctor and operator=() didn't agree, usually because one was updated but the other wasn't. I don't think we have any situations where they shouldn't completely agree. One way to make them always agree is to have operator=() use explicit dtor and placement new. Then any change to the copy ctor is automatically shared. This recipe only requires replacing Classname: Classname& operator=(const Classname& other) { if (this != &other) { this->~Classname(); new(this) Classname(other); } return *this; } Caveat: It's not meant for base classes. Tom Breton (Tehom) |
From: Tim M. <or...@le...> - 2013-02-10 23:53:18
|
Ted: Ted Felix wrote: > I'll just pontificate on copy ctors for a bit. Ignore if you've heard > it all before.... I haven't and I very much appreciate the feedback. > The first alternative that I usually try for is to remove the need > for a copy ctor altogether. I will try to do this. The implicit copy ctor wasn't up to the task, so simply dropping in an explicit one seemed the path of least resistance. However, since both Buss class and Instrument class inherit from PluginContainer class, any mistakes there could generate all sorts of nasty side effects. I will see if I can find some other way around the problem. > Also, make sure you declare an operator=(). You can make it private to > prevent anyone from using it. You can also leave it undefined so you'll > get a linker error in case the class itself uses it. For what it's worth, I did declare a private, undefined operator=(), and the program compiled without a hitch. Clearly the patch I submitted should not be used as is. I look forward to getting this thing resolved, so I can get back to actually writing some music. Tim Munro |
From: Ted F. <te...@te...> - 2013-02-11 00:37:14
|
On 02/10/2013 06:53 PM, Tim Munro wrote: >> Also, make sure you declare an operator=(). You can make it private to >> prevent anyone from using it. You can also leave it undefined so you'll >> get a linker error in case the class itself uses it. > > For what it's worth, I did declare a private, undefined operator=(), and the > program compiled without a hitch. That's a real good sign. That means the code isn't too far gone. > Clearly the patch I submitted should not be used as is. Really appreciate the effort, though. We need the help. You might be really close, depending on the situation. > I look forward to > getting this thing resolved, so I can get back to actually writing some music. I hear ya. Seems to always be the case with rg. Ted. |
From: D. M. M. <ros...@gm...> - 2013-02-11 02:00:46
|
On 02/10/2013 07:37 PM, Ted Felix wrote: >> Clearly the patch I submitted should not be used as is. > > Really appreciate the effort, though. We need the help. You might > be really close, depending on the situation. Probably won't get anywhere, but I have a minute, and I'm poking around to see if I see anything. Will report. -- D. Michael McIntyre |
From: Tim M. <or...@le...> - 2013-02-11 20:06:50
Attachments:
fix_submaster_count_patch
|
D. Michael McIntyre wrote: > On 02/10/2013 07:37 PM, Ted Felix wrote: > >>> >> Clearly the patch I submitted should not be used as is. >> > >> > Really appreciate the effort, though. We need the help. You might >> > be really close, depending on the situation. > Probably won't get anywhere, but I have a minute, and I'm poking around > to see if I see anything. Will report. > -- D. Michael McIntyre I think I've got it this time. I was almost there anyway. I found a way to add and remove submaster busses without resorting to copying. The Studio class already contained an addBuss() function, and now it also contains a removeBuss() function. Tim Munro |
From: D. M. M. <ros...@gm...> - 2013-02-12 01:21:20
|
On 02/11/2013 03:06 PM, Tim Munro wrote: > and remove submaster busses without resorting to copying. > > The Studio class already contained an addBuss() function, and now it also > contains a removeBuss() function. That looks like it should be a safer way of removing the old bus. Just looking at this and having only a very vague understanding of what's going on, I wonder if this latest version doesn't wipe out whatever was on the original set of busses when you've added or subtracted one. Isn't it just wiping the slate clean and producing a new slate of whatever the desired size is? If so, and if that gets the job done, I don't have a real problem with that, but I feel there should probably be some kind of warning. Or maybe I'm reading it wrong. I just woke up, depressingly enough. 8:00 PM and I just woke up. I guess I won't be doing anything with THIS day off. OTOH I guess I needed sleep, huh? Apparently so! -- D. Michael McIntyre |
From: D. M. M. <ros...@gm...> - 2013-02-12 02:04:44
|
> If so, and if that gets the job done, I don't have a real problem with > that, but I feel there should probably be some kind of warning. So in actual practice hands-on, I don't see the problem I vaguely expected to see based on glancing at and partially understanding the code. I didn't test this extensively at all, but I don't see any concerns. If you did test this extensively yourself, and you're satisfied with this version, I'll clean up some nitpicky things and commit just in time for the 13.02 release. -- D. Michael McIntyre |
From: D. M. M. <ros...@gm...> - 2013-02-12 07:36:16
|
> The Studio class already contained an addBuss() function, and now it also > contains a removeBuss() function. So after fooling around a bit more, I ended up committing it without changing anything after all. Some of the picky stuff I was going to go massage, you were just copying the spirit and style of the surrounding code. Seems OK to me, and nobody else has any more time than I do to review and really dig, so let's go with it and see what blows up in the future! It's called QUALITY CONTROL, and it's what we EXCEL at here at Rosegarden. -- D. Michael McIntyre |
From: Tim M. <or...@le...> - 2013-02-12 12:32:27
|
D. Michael McIntyre wrote: > Some of the picky stuff I was going to go massage, you were just copying > the spirit and style of the surrounding code. Yeah, that was my intent. I figured that way at least I wouldn't be degrading anything. Tim Munro |
From: Tim M. <or...@le...> - 2013-02-12 12:25:26
|
D. Michael McIntyre wrote: > On 02/11/2013 03:06 PM, Tim Munro wrote: > >> > and remove submaster busses without resorting to copying. >> > >> > The Studio class already contained an addBuss() function, and now it also >> > contains a removeBuss() function. > That looks like it should be a safer way of removing the old bus. > > Just looking at this and having only a very vague understanding of > what's going on, I wonder if this latest version doesn't wipe out > whatever was on the original set of busses when you've added or > subtracted one. Isn't it just wiping the slate clean and producing a > new slate of whatever the desired size is? No, my priority was to preserve the content of the original busses wherever possible. That was also the intent of the original code. Expanding the number of submasters now involves padding m_busses (in Studio) with empty busses until the desired size is reached. Shrinking the number of submasters involves removing existing busses from the end of m_busses until the desired size is reached. The original content of m_busses is left undisturbed wherever possible. The original code would take a snapshot of m_busses, blow away the contents of m_busses, and then repack it with new busses. These new busses would be either empties or duplicates of the originals, as needed. Where the original code got into trouble was when it tried to duplicate existing busses containing plugins. My first plan involved simply upgrading the copy ctor so that the code could proceed as intended. But Ted wisely pointed out that this approach was fraught with danger, so chose a different method. Tim Munro |
From: Holger M. <ho...@ma...> - 2013-02-12 12:50:59
|
On Tue, 12 Feb 2013, Tim Munro wrote: > D. Michael McIntyre wrote: > > On 02/11/2013 03:06 PM, Tim Munro wrote: > > > >> > and remove submaster busses without resorting to copying. > >> > > >> > The Studio class already contained an addBuss() function, and now it also > >> > contains a removeBuss() function. > > That looks like it should be a safer way of removing the old bus. > > > > Just looking at this and having only a very vague understanding of > > what's going on, I wonder if this latest version doesn't wipe out > > whatever was on the original set of busses when you've added or > > subtracted one. Isn't it just wiping the slate clean and producing a > > new slate of whatever the desired size is? > > No, my priority was to preserve the content of the original busses wherever > possible. That was also the intent of the original code. I wouldn't see it as a bug if expanding/shrinking the number of submasters would clear the plugins before, provided there was a warning before. If it can be done while preserving the plugins and their settings then it would be perfect. |
From: D. M. M. <ros...@gm...> - 2013-02-12 12:59:25
|
> No, my priority was to preserve the content of the original busses wherever > possible. That was also the intent of the original code. I get it now. I like the thinking behind that approach. I committed the patch because it seemed to work, but I feel much more relaxed about it now. I'm especially grateful that you worked out all the QAction nonsense and got that to work too. I'm noticing the trend that Ted, Tom and Tim are a tenacious trio of technological techniques. Rosegarden owes its good fortune to the letter T and the number 3. Well then, I'll see about releasing this in the next week or so. Your timing was perfect. -- D. Michael McIntyre |
From: D. M. M. <ros...@gm...> - 2013-02-12 13:41:28
|
On 02/12/2013 07:50 AM, Holger Marzen wrote: > If it can be done while preserving the plugins and their settings then > it would be perfect. Tim Munro is your new best friend then. Try it yourself. Seemed to work quite well to me. -- D. Michael McIntyre |
From: Holger M. <ho...@ma...> - 2013-02-12 17:21:17
|
On Tue, 12 Feb 2013, D. Michael McIntyre wrote: > On 02/12/2013 07:50 AM, Holger Marzen wrote: > > > If it can be done while preserving the plugins and their settings then > > it would be perfect. > > Tim Munro is your new best friend then. Try it yourself. Seemed to > work quite well to me. Works fine. Good work! Regards Holger |
From: Daren B. <dtb...@gm...> - 2013-02-12 15:18:51
|
> > Seems OK to me, and nobody else has any more time than I do to review > and really dig, so let's go with it and see what blows up in the future! > > It's called QUALITY CONTROL, and it's what we EXCEL at here at Rosegarden. > > I'm noticing the trend that Ted, Tom and Tim are a tenacious trio of > technological techniques. Rosegarden owes its good fortune to the letter T > and the number 3. > Can I submit these as quotes of the week on LWN? They're just too good not to share! |
From: D. M. M. <ros...@gm...> - 2013-02-12 20:17:23
|
On 02/12/2013 10:18 AM, Daren Beattie wrote: > Can I submit these as quotes of the week on LWN? They're just too good > not to share! Knock yourself out. -- D. Michael McIntyre |