GetLinked()

classic Classic list List threaded Threaded
48 messages Options
123
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

GetLinked()

James Crook
Paul, are we going to banish GetLinked() for 2.2.0 or wait until a later
release?

--James.


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: GetLinked()

David Bailes-3
On Sat, Jun 17, 2017 at 10:11 AM, James Crook <[hidden email]> wrote:
Paul, are we going to banish GetLinked() for 2.2.0 or wait until a later release?

James, is this a change to how stereo tracks are handled? What is the new design going to be?

I've been looking at why Audacity is crashing when Jaws is also running on the Windows 10 creators update.
When Jaws is running there appear to be more paint messages being sent to Audacity, though it's unclear to me what is causing this.
For example when deleting a stereo track, a paint message can occur after one of the tracks has been removed, and this causes audacity to  crash. Regardless of whether these extra paint message should be occurring, the code for removing stereo tracks sends update events after each of the two tracks is removed from the track list, rather than a single update event after both tracks have be removed. This doesn't seem right.

Hence my interest in any changes to the design of stereo tracks.

David.
 

--James.


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Jaws Crashes (was: GetLinked())

James Crook
David,

I'm relatively new to the party in terms of looking at actual screen
reader behaviour.  I notice that Audacity with Narrator on leaks memory
like a sieve.  Paul's cleaning up of naked news means non-narrator use
does not memory leak, so it makes these leaks more obvious.  That leads
me to think that wxWidgets with any screen reader is code that is less
well exercised and tested, i.e. expect many bugs relating to screen
readers in wxWidgets. Probably we can find workarounds, staying on the
parts that do work, rather than complete fixes.  But we may only be able
to ameliorate the problems rather than fix them.

I talked with Leland about screen readers at AU14 conference, and we
both were keen to provide access to Audacity algorithms at more of a
command line like level to get better accessibility.  Long term this is
still in my view the best way to improve accessibility of Audacity -
i.e. have an alternative front end for the same audio engine.  Have the
normal GUI which makes some assumptions about a user using a mouse, and
a more command line GUI that still has menus for access, but that is
geared for automation and keyboard use - with explicit binding of mouse
wheel and x-y to controlled values rather than implicit binding by click
and drag.


IF frequent repainting is causing problems for Jaws, we can / should
ameliorate by removing redundant repaintings.  That will be better for
all.  Audacity has a somewhat Jekyll and Hyde attitude to repainting.  
It has event based repainting and periodic painting off a timer.  At
some future time we should clean that up too.

The Windows 10 creators update has done some strange things at start
up.  The splash screen now appears sooner, and so it appears before it
has been positioned.  We have some strange code with the splash screen
to work around a problem with the possibility of having splash screen
and error dialog at the same time, before the app has fully started.  
There should not be a need for our own event loop (which is what we
have).  I think our workarounds maybe should be untangled.  As it is we
show and hide our splash screen rather quickly now, so it does not serve
the role of a splash screen, that reassures you that the app is starting
up.

This doesn't directly answer the question of Jaws crashes, but may be
useful none the less.

--James.


On 6/17/2017 10:52 AM, David Bailes wrote:

> On Sat, Jun 17, 2017 at 10:11 AM, James Crook <[hidden email]> wrote:
>
>> Paul, are we going to banish GetLinked() for 2.2.0 or wait until a later
>> release?
>>
> James, is this a change to how stereo tracks are handled? What is the new
> design going to be?
>
> I've been looking at why Audacity is crashing when Jaws is also running on
> the Windows 10 creators update.
> When Jaws is running there appear to be more paint messages being sent to
> Audacity, though it's unclear to me what is causing this.
> For example when deleting a stereo track, a paint message can occur after
> one of the tracks has been removed, and this causes audacity to  crash.
> Regardless of whether these extra paint message should be occurring, the
> code for removing stereo tracks sends update events after each of the two
> tracks is removed from the track list, rather than a single update event
> after both tracks have be removed. This doesn't seem right.
>
> Hence my interest in any changes to the design of stereo tracks.
>
> David.
>
>
>> --James.


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: GetLinked()

James Crook
In reply to this post by David Bailes-3
GetLinked() in Audacity is overly special case code.  It treats stereo
tracks as a special case embellishment of mono tracks.

A more logical design would not care whether we had none, one, two or
twenty channels in a 'track'.
Our code would just iterate through all channels, rather than as now do
the first channel and then possibly do a second channel as a special
case.  That special case code is often mostly a reduplication of the
'first channel' code.  It is not causing serious problems (we think) but
it is messy having 85 or so mini-duplications of code that are
completely avoidable.

My expectation is that we will move in two steps.  In the first step we
make the number of channels in a track arbitrary and clean up
GetLinked().  In the second step, we put channels, views and different
kinds of track on the same footing, so we can have a track that has
other views, channels and tracks in it, so we can have spectrogram and
wave view of the same track, with a label track overlay on one of them,
for example.  We will then need to be clearer what we are iterating
over, e.g. iterate over all views of the data in a track, or over all
data in a track, or over all audio data in a track.

In the course of such changes, we would likely need to reexamine what
events happen when.  E.g refreshes and flushes inside loops or outside.  
So very indirectly this would have a bearing on the repeated update
events you are talking about.

Jaws not working with the creators update is a much more pressing
problem than our code for GetLinked() being messy, so I wouldn't expect
us to proceed down the cleaning up GetLinked() route in order to
indirectly clear or ameliorate the Jaws issue.

--James.




On 6/17/2017 10:52 AM, David Bailes wrote:

> On Sat, Jun 17, 2017 at 10:11 AM, James Crook <[hidden email]> wrote:
>
>> Paul, are we going to banish GetLinked() for 2.2.0 or wait until a later
>> release?
>>
> James, is this a change to how stereo tracks are handled? What is the new
> design going to be?
>
> I've been looking at why Audacity is crashing when Jaws is also running on
> the Windows 10 creators update.
> When Jaws is running there appear to be more paint messages being sent to
> Audacity, though it's unclear to me what is causing this.
> For example when deleting a stereo track, a paint message can occur after
> one of the tracks has been removed, and this causes audacity to  crash.
> Regardless of whether these extra paint message should be occurring, the
> code for removing stereo tracks sends update events after each of the two
> tracks is removed from the track list, rather than a single update event
> after both tracks have be removed. This doesn't seem right.
>
> Hence my interest in any changes to the design of stereo tracks.
>
> David.
>
>
>> --James.
>>


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Jaws Crashes (was: GetLinked())

David Bailes-3
In reply to this post by James Crook
On Sat, Jun 17, 2017 at 11:42 AM, James Crook <[hidden email]> wrote:
David,

I'm relatively new to the party in terms of looking at actual screen reader behaviour.  I notice that Audacity with Narrator on leaks memory like a sieve.  Paul's cleaning up of naked news means non-narrator use does not memory leak, so it makes these leaks more obvious.  That leads me to think that wxWidgets with any screen reader is code that is less well exercised and tested, i.e. expect many bugs relating to screen readers in wxWidgets. Probably we can find workarounds, staying on the parts that do work, rather than complete fixes.  But we may only be able to ameliorate the problems rather than fix them.

I talked with Leland about screen readers at AU14 conference, and we both were keen to provide access to Audacity algorithms at more of a command line like level to get better accessibility.  Long term this is still in my view the best way to improve accessibility of Audacity - i.e. have an alternative front end for the same audio engine.  Have the normal GUI which makes some assumptions about a user using a mouse, and a more command line GUI that still has menus for access, but that is geared for automation and keyboard use - with explicit binding of mouse wheel and x-y to controlled values rather than implicit binding by click and drag.

I'm unsure about this alternative front end, but it's a discussion for another day. 


IF frequent repainting is causing problems for Jaws, we can / should ameliorate by removing redundant repaintings.  That will be better for all.  Audacity has a somewhat Jekyll and Hyde attitude to repainting.  It has event based repainting and periodic painting off a timer.

Is the periodic painting off a timer "on" all the time? Where is the code?
 
  At some future time we should clean that up too.

The Windows 10 creators update has done some strange things at start up.  The splash screen now appears sooner, and so it appears before it has been positioned.  We have some strange code with the splash screen to work around a problem with the possibility of having splash screen and error dialog at the same time, before the app has fully started.  There should not be a need for our own event loop (which is what we have).

Is there more than one event loop? Where is the code for our own event loop?
 
  I think our workarounds maybe should be untangled.  As it is we show and hide our splash screen rather quickly now, so it does not serve the role of a splash screen, that reassures you that the app is starting up.

This doesn't directly answer the question of Jaws crashes, but may be useful none the less.

very useful, thanks.


--James.


On 6/17/2017 10:52 AM, David Bailes wrote:
On Sat, Jun 17, 2017 at 10:11 AM, James Crook <[hidden email]> wrote:

Paul, are we going to banish GetLinked() for 2.2.0 or wait until a later
release?

James, is this a change to how stereo tracks are handled? What is the new
design going to be?

I've been looking at why Audacity is crashing when Jaws is also running on
the Windows 10 creators update.
When Jaws is running there appear to be more paint messages being sent to
Audacity, though it's unclear to me what is causing this.
For example when deleting a stereo track, a paint message can occur after
one of the tracks has been removed, and this causes audacity to  crash.
Regardless of whether these extra paint message should be occurring, the
code for removing stereo tracks sends update events after each of the two
tracks is removed from the track list, rather than a single update event
after both tracks have be removed. This doesn't seem right.

Hence my interest in any changes to the design of stereo tracks.

David.


--James.


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Jaws Crashes

James Crook
On 6/17/2017 12:28 PM, David Bailes wrote:
On Sat, Jun 17, 2017 at 11:42 AM, James Crook [hidden email] wrote:

David,

I'm relatively new to the party in terms of looking at actual screen
reader behaviour.  I notice that Audacity with Narrator on leaks memory
like a sieve.  Paul's cleaning up of naked news means non-narrator use does
not memory leak, so it makes these leaks more obvious.  That leads me to
think that wxWidgets with any screen reader is code that is less well
exercised and tested, i.e. expect many bugs relating to screen readers in
wxWidgets. Probably we can find workarounds, staying on the parts that do
work, rather than complete fixes.  But we may only be able to ameliorate
the problems rather than fix them.

I talked with Leland about screen readers at AU14 conference, and we both
were keen to provide access to Audacity algorithms at more of a command
line like level to get better accessibility.  Long term this is still in my
view the best way to improve accessibility of Audacity - i.e. have an
alternative front end for the same audio engine.  Have the normal GUI which
makes some assumptions about a user using a mouse, and a more command line
GUI that still has menus for access, but that is geared for automation and
keyboard use - with explicit binding of mouse wheel and x-y to controlled
values rather than implicit binding by click and drag.

I'm unsure about this alternative front end, but it's a discussion for
another day.


IF frequent repainting is causing problems for Jaws, we can / should
ameliorate by removing redundant repaintings.  That will be better for
all.  Audacity has a somewhat Jekyll and Hyde attitude to repainting.  It
has event based repainting and periodic painting off a timer.

Is the periodic painting off a timer "on" all the time? Where is the code?
You'll need to investigate..

      mTimer.Start(kTimerInterval, FALSE);

kTimerInterval is 50ms, which is 20 fps, which applies to the overlays.

void TrackPanel::OnTimer(wxTimerEvent& )

Which calls
   DrawOverlays(false);
   mRuler->DrawOverlays(false);

And every fifth call does:

    mRefreshBacking = true;
    Refresh( false );

so 4fps for complete repaints.



  At some future time we should clean that up too.

The Windows 10 creators update has done some strange things at start up.
The splash screen now appears sooner, and so it appears before it has been
positioned.  We have some strange code with the splash screen to work
around a problem with the possibility of having splash screen and error
dialog at the same time, before the app has fully started.  There should
not be a need for our own event loop (which is what we have).

Is there more than one event loop? Where is the code for our own event loop?
At start up we have:
   wxEventLoopGuarantor eventLoop;

created in
bool AudacityApp::OnInit()

I (probably mistakenly) thought this created a temporary event loop.

--James.


      
  I think our workarounds maybe should be untangled.  As it is we show and
hide our splash screen rather quickly now, so it does not serve the role of
a splash screen, that reassures you that the app is starting up.

This doesn't directly answer the question of Jaws crashes, but may be
useful none the less.

very useful, thanks.


--James.


On 6/17/2017 10:52 AM, David Bailes wrote:

On Sat, Jun 17, 2017 at 10:11 AM, James Crook [hidden email] wrote:

Paul, are we going to banish GetLinked() for 2.2.0 or wait until a later
release?

James, is this a change to how stereo tracks are handled? What is the new
design going to be?

I've been looking at why Audacity is crashing when Jaws is also running on
the Windows 10 creators update.
When Jaws is running there appear to be more paint messages being sent to
Audacity, though it's unclear to me what is causing this.
For example when deleting a stereo track, a paint message can occur after
one of the tracks has been removed, and this causes audacity to  crash.
Regardless of whether these extra paint message should be occurring, the
code for removing stereo tracks sends update events after each of the two
tracks is removed from the track list, rather than a single update event
after both tracks have be removed. This doesn't seem right.

Hence my interest in any changes to the design of stereo tracks.

David.


--James.

          

        
------------------------------------------------------------
------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel


      

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot


_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Jaws Crashes

Paul Licameli


On Sat, Jun 17, 2017 at 9:04 AM, James Crook <[hidden email]> wrote:
On 6/17/2017 12:28 PM, David Bailes wrote:
On Sat, Jun 17, 2017 at 11:42 AM, James Crook [hidden email] wrote:

David,

I'm relatively new to the party in terms of looking at actual screen
reader behaviour.  I notice that Audacity with Narrator on leaks memory
like a sieve.  Paul's cleaning up of naked news means non-narrator use does
not memory leak, so it makes these leaks more obvious.  That leads me to
think that wxWidgets with any screen reader is code that is less well
exercised and tested, i.e. expect many bugs relating to screen readers in
wxWidgets. Probably we can find workarounds, staying on the parts that do
work, rather than complete fixes.  But we may only be able to ameliorate
the problems rather than fix them.

Maybe I need to work my way onto the wxWidget team too and do for their memory management what I did for ours.
 
I talked with Leland about screen readers at AU14 conference, and we both
were keen to provide access to Audacity algorithms at more of a command
line like level to get better accessibility.  Long term this is still in my
view the best way to improve accessibility of Audacity - i.e. have an
alternative front end for the same audio engine.  Have the normal GUI which
makes some assumptions about a user using a mouse, and a more command line
GUI that still has menus for access, but that is geared for automation and
keyboard use - with explicit binding of mouse wheel and x-y to controlled
values rather than implicit binding by click and drag.

I'm unsure about this alternative front end, but it's a discussion for
another day.

      
IF frequent repainting is causing problems for Jaws, we can / should
ameliorate by removing redundant repaintings.  That will be better for
all.  Audacity has a somewhat Jekyll and Hyde attitude to repainting.  It
has event based repainting and periodic painting off a timer.
Is the periodic painting off a timer "on" all the time? Where is the code?
You'll need to investigate..

      mTimer.Start(kTimerInterval, FALSE);

kTimerInterval is 50ms, which is 20 fps, which applies to the overlays.

void TrackPanel::OnTimer(wxTimerEvent& )

Which calls
   DrawOverlays(false);
   mRuler->DrawOverlays(false);

And every fifth call does:

    mRefreshBacking = true;
    Refresh( false );

so 4fps for complete repaints.

I believe that timer driven repaint occurs only during recording and playback.  If you only play back, with an unpinned play head, then I believe it is doing only a cheap update of the green line, by repainting a saved rectangle over its old position, saving the pixels at the new position, and then overpainting those.

As you can imagine, more complete repaints are needed if the playhead is pinned, or if you record and lay down new audio.

In 2.1.3 Track preferences we had a checkbox for "Update display when Recording/Playback head unpinned" which might help you here.  But that has been removed from 2.2.0.  I recall there was some discussion whether faster computers these days make this choice less valuable to anybody.  Maybe you want to lobby to put that preference back?

PRL

 



  At some future time we should clean that up too.

The Windows 10 creators update has done some strange things at start up.
The splash screen now appears sooner, and so it appears before it has been
positioned.  We have some strange code with the splash screen to work
around a problem with the possibility of having splash screen and error
dialog at the same time, before the app has fully started.  There should
not be a need for our own event loop (which is what we have).
Is there more than one event loop? Where is the code for our own event loop?
At start up we have:
   wxEventLoopGuarantor eventLoop;

created in
bool AudacityApp::OnInit()

I (probably mistakenly) thought this created a temporary event loop.

--James.


      
  I think our workarounds maybe should be untangled.  As it is we show and
hide our splash screen rather quickly now, so it does not serve the role of
a splash screen, that reassures you that the app is starting up.

This doesn't directly answer the question of Jaws crashes, but may be
useful none the less.

very useful, thanks.


--James.


On 6/17/2017 10:52 AM, David Bailes wrote:

On Sat, Jun 17, 2017 at 10:11 AM, James Crook [hidden email] wrote:

Paul, are we going to banish GetLinked() for 2.2.0 or wait until a later
release?

James, is this a change to how stereo tracks are handled? What is the new
design going to be?

I've been looking at why Audacity is crashing when Jaws is also running on
the Windows 10 creators update.
When Jaws is running there appear to be more paint messages being sent to
Audacity, though it's unclear to me what is causing this.
For example when deleting a stereo track, a paint message can occur after
one of the tracks has been removed, and this causes audacity to  crash.
Regardless of whether these extra paint message should be occurring, the
code for removing stereo tracks sends update events after each of the two
tracks is removed from the track list, rather than a single update event
after both tracks have be removed. This doesn't seem right.

Hence my interest in any changes to the design of stereo tracks.

David.


--James.

          

        
------------------------------------------------------------
------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel


      

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot


_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: GetLinked()

Paul Licameli
In reply to this post by James Crook


On Sat, Jun 17, 2017 at 7:02 AM, James Crook <[hidden email]> wrote:
GetLinked() in Audacity is overly special case code.  It treats stereo tracks as a special case embellishment of mono tracks.

A more logical design would not care whether we had none, one, two or twenty channels in a 'track'.
Our code would just iterate through all channels, rather than as now do the first channel and then possibly do a second channel as a special case.  That special case code is often mostly a reduplication of the 'first channel' code.  It is not causing serious problems (we think) but it is messy having 85 or so mini-duplications of code that are completely avoidable.

My expectation is that we will move in two steps.  In the first step we make the number of channels in a track arbitrary and clean up GetLinked().  In the second step, we put channels, views and different kinds of track on the same footing, so we can have a track that has other views, channels and tracks in it, so we can have spectrogram and wave view of the same track, with a label track overlay on one of them, for example.  We will then need to be clearer what we are iterating over, e.g. iterate over all views of the data in a track, or over all data in a track, or over all audio data in a track.

In the course of such changes, we would likely need to reexamine what events happen when.  E.g refreshes and flushes inside loops or outside.  So very indirectly this would have a bearing on the repeated update events you are talking about.

My understanding is that wxWindows does not send one refresh event for each Refresh() call to a window.  Rather it only sends one message to each window that has had at least one call to Refresh between one event loop pass and the next.  Am I wrong?
PRL

 

Jaws not working with the creators update is a much more pressing problem than our code for GetLinked() being messy, so I wouldn't expect us to proceed down the cleaning up GetLinked() route in order to indirectly clear or ameliorate the Jaws issue.

--James.





On 6/17/2017 10:52 AM, David Bailes wrote:
On Sat, Jun 17, 2017 at 10:11 AM, James Crook <[hidden email]> wrote:

Paul, are we going to banish GetLinked() for 2.2.0 or wait until a later
release?

James, is this a change to how stereo tracks are handled? What is the new
design going to be?

I've been looking at why Audacity is crashing when Jaws is also running on
the Windows 10 creators update.
When Jaws is running there appear to be more paint messages being sent to
Audacity, though it's unclear to me what is causing this.
For example when deleting a stereo track, a paint message can occur after
one of the tracks has been removed, and this causes audacity to  crash.
Regardless of whether these extra paint message should be occurring, the
code for removing stereo tracks sends update events after each of the two
tracks is removed from the track list, rather than a single update event
after both tracks have be removed. This doesn't seem right.

Hence my interest in any changes to the design of stereo tracks.

David.


--James.



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: GetLinked()

Paul Licameli
In reply to this post by James Crook


On Sat, Jun 17, 2017 at 5:11 AM, James Crook <[hidden email]> wrote:
Paul, are we going to banish GetLinked() for 2.2.0 or wait until a later release?

My present thinking is no, this would be an overreach for 2.2.0.  I would rather like to get it ready as a big-bang for the start of 2.2.1.  Which could be stabilized sooner than otherwise, without these changes.

Why do you ask now?  Do you want to persuade me otherwise?  Do you want to rebase some of your pending work onto it?

Should I go for broke pushing all my pervasive disruptions into one longer delayed version for one kick-the-tyres?  (I don't think so.)

What I have done so far is in a branch called track-iters2.  Not only are there far fewer calls to GetLink(), but also every iteration over tracks was rewritten in an idiom that is less verbose and also type- and const-correct through some use of C++ templates.  I didn't like seeing C-style casts like ((WaveTrack*)t) and those are all gone.  Instead you can say something like:  for (auto t : trackList->Selected<const WaveTrack>) { ... }

I did bring that branch up to date with early 2.2.0 developments and test it to the extent of putting a breakpoint at every changed place and hitting it.

But much work would remain, between that and TrackPanel refactoring, to rebase one onto the other and resolve conflicts.  I decided that the TrackPanel refactor was the more valuable thing that I ought to press on with first.  So I knuckled down to the task of reviewing all my forgotten two year old code, and that hurdle is thankfully past.

You mention iterations over things in another email.  As I see it, our code doesn't yet do a good enough job of decoupling the persistent entities in the data (tracks and their contents) from the view entities in the display (so, as you say, we ought to be freer to make many-to-one correspondences).

So better abstracted iteration over both kinds of entity would be valuable.  And for the latter kind, display, we are making progress right now.

Please look at TrackPanelCellIterator in head.  I made changes to this class only yesterday.  The purpose of this class is iteration over the disjoint rectangular areas of TrackPanel that have different policies for drawing themselves and for dispatching events when the pointer is in them.

I want to concentrate the knowledge of just what the screen-partitioning policy is, into one class, that you might replace in order to define a different policy.  This is an advancement toward that.

PRL




--James.



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Jaws Crashes

Gale
Administrator
In reply to this post by Paul Licameli
On 17 June 2017 at 15:52, Paul Licameli <[hidden email]> wrote:

>
>
> On Sat, Jun 17, 2017 at 9:04 AM, James Crook <[hidden email]> wrote:
>>
>> On 6/17/2017 12:28 PM, David Bailes wrote:
>>
>> On Sat, Jun 17, 2017 at 11:42 AM, James Crook <[hidden email]> wrote:
>>
>> David,
>>
>> I'm relatively new to the party in terms of looking at actual screen
>> reader behaviour.  I notice that Audacity with Narrator on leaks memory
>> like a sieve.  Paul's cleaning up of naked news means non-narrator use
>> does
>> not memory leak, so it makes these leaks more obvious.  That leads me to
>> think that wxWidgets with any screen reader is code that is less well
>> exercised and tested, i.e. expect many bugs relating to screen readers in
>> wxWidgets. Probably we can find workarounds, staying on the parts that do
>> work, rather than complete fixes.  But we may only be able to ameliorate
>> the problems rather than fix them.
>
>
> Maybe I need to work my way onto the wxWidget team too and do for their
> memory management what I did for ours.
>
>>
>> I talked with Leland about screen readers at AU14 conference, and we both
>> were keen to provide access to Audacity algorithms at more of a command
>> line like level to get better accessibility.  Long term this is still in
>> my
>> view the best way to improve accessibility of Audacity - i.e. have an
>> alternative front end for the same audio engine.  Have the normal GUI
>> which
>> makes some assumptions about a user using a mouse, and a more command line
>> GUI that still has menus for access, but that is geared for automation and
>> keyboard use - with explicit binding of mouse wheel and x-y to controlled
>> values rather than implicit binding by click and drag.
>>
>> I'm unsure about this alternative front end, but it's a discussion for
>> another day.
>>
>> IF frequent repainting is causing problems for Jaws, we can / should
>> ameliorate by removing redundant repaintings.  That will be better for
>> all.  Audacity has a somewhat Jekyll and Hyde attitude to repainting.  It
>> has event based repainting and periodic painting off a timer.
>>
>> Is the periodic painting off a timer "on" all the time? Where is the code?
>>
>> You'll need to investigate..
>>
>>       mTimer.Start(kTimerInterval, FALSE);
>>
>> kTimerInterval is 50ms, which is 20 fps, which applies to the overlays.
>>
>> void TrackPanel::OnTimer(wxTimerEvent& )
>>
>> Which calls
>>    DrawOverlays(false);
>>    mRuler->DrawOverlays(false);
>>
>> And every fifth call does:
>>
>>     mRefreshBacking = true;
>>     Refresh( false );
>>
>> so 4fps for complete repaints.
>
>
> I believe that timer driven repaint occurs only during recording and
> playback.  If you only play back, with an unpinned play head, then I believe
> it is doing only a cheap update of the green line, by repainting a saved
> rectangle over its old position, saving the pixels at the new position, and
> then overpainting those.
>
> As you can imagine, more complete repaints are needed if the playhead is
> pinned, or if you record and lay down new audio.
>
> In 2.1.3 Track preferences we had a checkbox for "Update display when
> Recording/Playback head unpinned" which might help you here.  But that has
> been removed from 2.2.0.  I recall there was some discussion whether faster
> computers these days make this choice less valuable to anybody.  Maybe you
> want to lobby to put that preference back?

The preference is still there, now shortened to "Auto-scroll if head unpinned".


Gale


>>   At some future time we should clean that up too.
>>
>> The Windows 10 creators update has done some strange things at start up.
>> The splash screen now appears sooner, and so it appears before it has been
>> positioned.  We have some strange code with the splash screen to work
>> around a problem with the possibility of having splash screen and error
>> dialog at the same time, before the app has fully started.  There should
>> not be a need for our own event loop (which is what we have).
>>
>> Is there more than one event loop? Where is the code for our own event
>> loop?
>>
>> At start up we have:
>>    wxEventLoopGuarantor eventLoop;
>>
>> created in
>> bool AudacityApp::OnInit()
>>
>> I (probably mistakenly) thought this created a temporary event loop.
>>
>> --James.
>>
>>   I think our workarounds maybe should be untangled.  As it is we show and
>> hide our splash screen rather quickly now, so it does not serve the role
>> of
>> a splash screen, that reassures you that the app is starting up.
>>
>> This doesn't directly answer the question of Jaws crashes, but may be
>> useful none the less.
>>
>> very useful, thanks.
>>
>>
>> --James.
>>
>>
>> On 6/17/2017 10:52 AM, David Bailes wrote:
>>
>> On Sat, Jun 17, 2017 at 10:11 AM, James Crook <[hidden email]> wrote:
>>
>> Paul, are we going to banish GetLinked() for 2.2.0 or wait until a later
>>
>> release?
>>
>> James, is this a change to how stereo tracks are handled? What is the new
>>
>> design going to be?
>>
>> I've been looking at why Audacity is crashing when Jaws is also running on
>> the Windows 10 creators update.
>> When Jaws is running there appear to be more paint messages being sent to
>> Audacity, though it's unclear to me what is causing this.
>> For example when deleting a stereo track, a paint message can occur after
>> one of the tracks has been removed, and this causes audacity to  crash.
>> Regardless of whether these extra paint message should be occurring, the
>> code for removing stereo tracks sends update events after each of the two
>> tracks is removed from the track list, rather than a single update event
>> after both tracks have be removed. This doesn't seem right.
>>
>> Hence my interest in any changes to the design of stereo tracks.
>>
>> David.
>>
>>
>> --James.
>>
>> ------------------------------------------------------------
>> ------------------
>> Check out the vibrant tech community on one of the world's most
>> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>> _______________________________________________
>> audacity-devel mailing list
>> [hidden email]
>> https://lists.sourceforge.net/lists/listinfo/audacity-devel
>>
>>
>>
>>
>> ------------------------------------------------------------------------------
>> Check out the vibrant tech community on one of the world's most
>> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>>
>>
>>
>> _______________________________________________
>> audacity-devel mailing list
>> [hidden email]
>> https://lists.sourceforge.net/lists/listinfo/audacity-devel
>>
>>
>>
>>
>> ------------------------------------------------------------------------------
>> Check out the vibrant tech community on one of the world's most
>> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
>> _______________________________________________
>> audacity-devel mailing list
>> [hidden email]
>> https://lists.sourceforge.net/lists/listinfo/audacity-devel
>>
>
>
> ------------------------------------------------------------------------------
> Check out the vibrant tech community on one of the world's most
> engaging tech sites, Slashdot.org! http://sdm.link/slashdot
> _______________________________________________
> audacity-devel mailing list
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/audacity-devel
>

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Jaws Crashes (was: GetLinked())

Paul Licameli
In reply to this post by James Crook


On Sat, Jun 17, 2017 at 6:42 AM, James Crook <[hidden email]> wrote:
David,

I'm relatively new to the party in terms of looking at actual screen reader behaviour.  I notice that Audacity with Narrator on leaks memory like a sieve.  Paul's cleaning up of naked news means non-narrator use does not memory leak, so it makes these leaks more obvious.  That leads me to think that wxWidgets with any screen reader is code that is less well exercised and tested, i.e. expect many bugs relating to screen readers in wxWidgets. Probably we can find workarounds, staying on the parts that do work, rather than complete fixes.  But we may only be able to ameliorate the problems rather than fix them.

I talked with Leland about screen readers at AU14 conference, and we both were keen to provide access to Audacity algorithms at more of a command line like level to get better accessibility.  Long term this is still in my view the best way to improve accessibility of Audacity - i.e. have an alternative front end for the same audio engine.  Have the normal GUI which makes some assumptions about a user using a mouse, and a more command line GUI that still has menus for access, but that is geared for automation and keyboard use - with explicit binding of mouse wheel and x-y to controlled values rather than implicit binding by click and drag.


IF frequent repainting is causing problems for Jaws, we can / should ameliorate by removing redundant repaintings.  That will be better for all.  Audacity has a somewhat Jekyll and Hyde attitude to repainting.  It has event based repainting and periodic painting off a timer.  At some future time we should clean that up too.

The Windows 10 creators update has done some strange things at start up.  The splash screen now appears sooner, and so it appears before it has been positioned.  We have some strange code with the splash screen to work around a problem with the possibility of having splash screen and error dialog at the same time, before the app has fully started.  There should not be a need for our own event loop (which is what we have).  I think our workarounds maybe should be untangled.  As it is we show and hide our splash screen rather quickly now, so it does not serve the role of a splash screen, that reassures you that the app is starting up.

This doesn't directly answer the question of Jaws crashes, but may be useful none the less.

--James.


On 6/17/2017 10:52 AM, David Bailes wrote:
On Sat, Jun 17, 2017 at 10:11 AM, James Crook <[hidden email]> wrote:

Paul, are we going to banish GetLinked() for 2.2.0 or wait until a later
release?

James, is this a change to how stereo tracks are handled? What is the new
design going to be?

I've been looking at why Audacity is crashing when Jaws is also running on
the Windows 10 creators update.

How reliably can you make these crashes happen?

I notice this comment in TrackPanel.cpp, which dates to Jan 2010 or earlier -- I can't discover who wrote it first in the confusing git repository for the older commits.  I think it was new in version 1.3.9.

   // The check for a null linked track is necessary because there's

   // a possible race condition between the time the 2 linked tracks

   // are added and when wxAccessible methods are called.  This is

   // most evident when using Jaws.


So this suggests multi-threading was involved.  This makes me suspect that the bare pointer, Track *TrackPanelAx::mFocusedTrack, is the problematic dangling pointer here.

And this makes me suspect that a solution would involve changing our management of Track objects everywhere to use std::shared_ptr rather that std::unique_ptr.  That might not be as hard as it sounds -- it could be only a matter of changint some "using" type aliases.

Then, TrackPanelAx should point to its track by a shared_ptr.

This would not be sufficient:  uses of that shared_ptr member of TrackPanelAx should then themselves be guarded by a mutex.  Standard share_ptr gives you thread safety for free when threads each use different shared_ptrs to the shared object, but not when threads contend for the same shared_ptr.  That is explained here:  http://en.cppreference.com/w/cpp/memory/shared_ptr

Is this a sufficient description, and do you understand C++11 smart pointers enough, that you might attempt this rewrite yourself?

PRL


 
When Jaws is running there appear to be more paint messages being sent to
Audacity, though it's unclear to me what is causing this.
For example when deleting a stereo track, a paint message can occur after
one of the tracks has been removed, and this causes audacity to  crash.
Regardless of whether these extra paint message should be occurring, the
code for removing stereo tracks sends update events after each of the two
tracks is removed from the track list, rather than a single update event
after both tracks have be removed. This doesn't seem right.

Hence my interest in any changes to the design of stereo tracks.

David.


--James.


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: GetLinked()

James Crook
In reply to this post by Paul Licameli
On 6/17/2017 4:23 PM, Paul Licameli wrote:

> On Sat, Jun 17, 2017 at 5:11 AM, James Crook <[hidden email]> wrote:
>
>> Paul, are we going to banish GetLinked() for 2.2.0 or wait until a later
>> release?
>
> My present thinking is no, this would be an overreach for 2.2.0.  I would
> rather like to get it ready as a big-bang for the start of 2.2.1.  Which
> could be stabilized sooner than otherwise, without these changes.
>
> Why do you ask now?  Do you want to persuade me otherwise?  Do you want to
> rebase some of your pending work onto it?

I asked because of noticing work related to GetLinked() in
VisibleTrackIterator:: and wondering what your plan was.
I'd like to persuade you to do it early on in 2.2.1.

There is a possible case for doing it sooner, as current HEAD, despite
your best efforts in the refactoring, seems rather buggy, and so will
need a lot of testing.  There is a possible case that the GetLinked code
refactoring is dangerous too, and so possibly a case to get one lot of
testing covering both refactorings.  Counter arguments include that more
changes means harder bug hunting, and overall it would assuredly affect
schedule.  I think it is sufficiently risky/big not to do it now.

Nope, there's not pending work of mine that would benefit from rebasing
on it.  Main concern is that you don't have to keep rebasing it (with
conflicts) to carry it forward.



> Should I go for broke pushing all my pervasive disruptions into one longer
> delayed version for one kick-the-tyres?  (I don't think so.)
I don't think so either.



> What I have done so far is in a branch called track-iters2.  Not only are
> there far fewer calls to GetLink(), but also every iteration over tracks
> was rewritten in an idiom that is less verbose and also type- and
> const-correct through some use of C++ templates.  I didn't like seeing
> C-style casts like ((WaveTrack*)t) and those are all gone.  Instead you can
> say something like:  for (auto t : trackList->Selected<const WaveTrack>) {
> ... }
>
> I did bring that branch up to date with early 2.2.0 developments and test
> it to the extent of putting a breakpoint at every changed place and hitting
> it.
>
> But much work would remain, between that and TrackPanel refactoring, to
> rebase one onto the other and resolve conflicts.  I decided that the
> TrackPanel refactor was the more valuable thing that I ought to press on
> with first.  So I knuckled down to the task of reviewing all my forgotten
> two year old code, and that hurdle is thankfully past.
I'm glad too, as you should not be carrying that refactoring forward
endlessly.


>
> You mention iterations over things in another email.  As I see it, our code
> doesn't yet do a good enough job of decoupling the persistent entities in
> the data (tracks and their contents) from the view entities in the display
+1
> (so, as you say, we ought to be freer to make many-to-one correspondences).
>
> So better abstracted iteration over both kinds of entity would be
> valuable.  And for the latter kind, display, we are making progress right
> now.
>
> Please look at TrackPanelCellIterator in head.  I made changes to this
> class only yesterday.
Yes.  That's partly what prompted my question.
>   The purpose of this class is iteration over the
> disjoint rectangular areas of TrackPanel that have different policies for
> drawing themselves and for dispatching events when the pointer is in them.
>
> I want to concentrate the knowledge of just what the screen-partitioning
> policy is, into one class, that you might replace in order to define a
> different policy.  This is an advancement toward that.
Cool.


> PRL
>
>> --James.
>>


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: GetLinked()

Paul Licameli


On Sat, Jun 17, 2017 at 1:34 PM, James Crook <[hidden email]> wrote:
On 6/17/2017 4:23 PM, Paul Licameli wrote:
On Sat, Jun 17, 2017 at 5:11 AM, James Crook <[hidden email]> wrote:

Paul, are we going to banish GetLinked() for 2.2.0 or wait until a later
release?

My present thinking is no, this would be an overreach for 2.2.0.  I would
rather like to get it ready as a big-bang for the start of 2.2.1.  Which
could be stabilized sooner than otherwise, without these changes.

Why do you ask now?  Do you want to persuade me otherwise?  Do you want to
rebase some of your pending work onto it?

I asked because of noticing work related to GetLinked() in VisibleTrackIterator:: and wondering what your plan was.
I'd like to persuade you to do it early on in 2.2.1.

There is a possible case for doing it sooner, as current HEAD, despite your best efforts in the refactoring, seems rather buggy,

What are you referring to?

Broken keystroke dispatching has been fixed.

Peter has reported crashes on Windows for menu items, still to investigate.

Still, these are "gross" problems that have obvious solutions.  Question is whether there is much both gross and subtle.

You see how I want to allow time to shake bugs out of things like this.  To get the other iteration work even to the point of testing by Gale and Peter is going to require nontrivial effort of conflict resolution on my part.  So again I'm inclined to say no, let this go until 2.2.1.

PRL

 
and so will need a lot of testing.  There is a possible case that the GetLinked code refactoring is dangerous too, and so possibly a case to get one lot of testing covering both refactorings.  Counter arguments include that more changes means harder bug hunting, and overall it would assuredly affect schedule.  I think it is sufficiently risky/big not to do it now.

Nope, there's not pending work of mine that would benefit from rebasing on it.  Main concern is that you don't have to keep rebasing it (with conflicts) to carry it forward.



Should I go for broke pushing all my pervasive disruptions into one longer
delayed version for one kick-the-tyres?  (I don't think so.)
I don't think so either.



What I have done so far is in a branch called track-iters2.  Not only are
there far fewer calls to GetLink(), but also every iteration over tracks
was rewritten in an idiom that is less verbose and also type- and
const-correct through some use of C++ templates.  I didn't like seeing
C-style casts like ((WaveTrack*)t) and those are all gone.  Instead you can
say something like:  for (auto t : trackList->Selected<const WaveTrack>) {
... }

I did bring that branch up to date with early 2.2.0 developments and test
it to the extent of putting a breakpoint at every changed place and hitting
it.

But much work would remain, between that and TrackPanel refactoring, to
rebase one onto the other and resolve conflicts.  I decided that the
TrackPanel refactor was the more valuable thing that I ought to press on
with first.  So I knuckled down to the task of reviewing all my forgotten
two year old code, and that hurdle is thankfully past.
I'm glad too, as you should not be carrying that refactoring forward endlessly.



You mention iterations over things in another email.  As I see it, our code
doesn't yet do a good enough job of decoupling the persistent entities in
the data (tracks and their contents) from the view entities in the display
+1
(so, as you say, we ought to be freer to make many-to-one correspondences).

So better abstracted iteration over both kinds of entity would be
valuable.  And for the latter kind, display, we are making progress right
now.

Please look at TrackPanelCellIterator in head.  I made changes to this
class only yesterday.
Yes.  That's partly what prompted my question.
  The purpose of this class is iteration over the
disjoint rectangular areas of TrackPanel that have different policies for
drawing themselves and for dispatching events when the pointer is in them.

I want to concentrate the knowledge of just what the screen-partitioning
policy is, into one class, that you might replace in order to define a
different policy.  This is an advancement toward that.
Cool.


PRL

--James.



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: GetLinked()

James Crook
On 6/17/2017 6:47 PM, Paul Licameli wrote:

> On Sat, Jun 17, 2017 at 1:34 PM, James Crook <[hidden email]> wrote:
>
>> On 6/17/2017 4:23 PM, Paul Licameli wrote:
>>
>>> On Sat, Jun 17, 2017 at 5:11 AM, James Crook <[hidden email]> wrote:
>>>
>>> Paul, are we going to banish GetLinked() for 2.2.0 or wait until a later
>>>> release?
>>>>
>>> My present thinking is no, this would be an overreach for 2.2.0.  I would
>>> rather like to get it ready as a big-bang for the start of 2.2.1.  Which
>>> could be stabilized sooner than otherwise, without these changes.
>>>
>>> Why do you ask now?  Do you want to persuade me otherwise?  Do you want to
>>> rebase some of your pending work onto it?
>>>
>> I asked because of noticing work related to GetLinked() in
>> VisibleTrackIterator:: and wondering what your plan was.
>> I'd like to persuade you to do it early on in 2.2.1.
>>
>> There is a possible case for doing it sooner, as current HEAD, despite
>> your best efforts in the refactoring, seems rather buggy,
>
> What are you referring to?
>
> Broken keystroke dispatching has been fixed.
>
> Peter has reported crashes on Windows for menu items, still to investigate.
I hit that one too, for TCP menu.
I also hit a 'sequence error' exception when doing a cut, which
unfortunately I could not make repeatable.  I can't find the text
'sequence error' in the code, so I'm guessing I mis rememebered what the
error message dialog said.
I also hit bug http://bugzilla.audacityteam.org/show_bug.cgi?id=1663 
which it turns out is actually a bug in 2.1.3, so not new..

So that is 3 significant distinct bugs in a relatively short spell of
use, suggesting (at the time) that the rearrangement has introduced half
a dozen or so....  As 1663 predates the refactoring, I was mistaken in
counting it.


> Still, these are "gross" problems that have obvious solutions.  Question is
> whether there is much both gross and subtle.
>
> You see how I want to allow time to shake bugs out of things like this.
Yes.
> To get the other iteration work even to the point of testing by Gale and Peter
> is going to require nontrivial effort of conflict resolution on my part.
> So again I'm inclined to say no, let this go until 2.2.1.
Sounds sensible to me too.
--James

>
> PRL
>
>
>
>> and so will need a lot of testing.  There is a possible case that the
>> GetLinked code refactoring is dangerous too, and so possibly a case to get
>> one lot of testing covering both refactorings.  Counter arguments include
>> that more changes means harder bug hunting, and overall it would assuredly
>> affect schedule.  I think it is sufficiently risky/big not to do it now.
>>
>> Nope, there's not pending work of mine that would benefit from rebasing on
>> it.  Main concern is that you don't have to keep rebasing it (with
>> conflicts) to carry it forward.
>>
>>
>>
>> Should I go for broke pushing all my pervasive disruptions into one longer
>>> delayed version for one kick-the-tyres?  (I don't think so.)
>>>
>> I don't think so either.
>>
>>
>>
>> What I have done so far is in a branch called track-iters2.  Not only are
>>> there far fewer calls to GetLink(), but also every iteration over tracks
>>> was rewritten in an idiom that is less verbose and also type- and
>>> const-correct through some use of C++ templates.  I didn't like seeing
>>> C-style casts like ((WaveTrack*)t) and those are all gone.  Instead you
>>> can
>>> say something like:  for (auto t : trackList->Selected<const WaveTrack>) {
>>> ... }
>>>
>>> I did bring that branch up to date with early 2.2.0 developments and test
>>> it to the extent of putting a breakpoint at every changed place and
>>> hitting
>>> it.
>>>
>>> But much work would remain, between that and TrackPanel refactoring, to
>>> rebase one onto the other and resolve conflicts.  I decided that the
>>> TrackPanel refactor was the more valuable thing that I ought to press on
>>> with first.  So I knuckled down to the task of reviewing all my forgotten
>>> two year old code, and that hurdle is thankfully past.
>>>
>> I'm glad too, as you should not be carrying that refactoring forward
>> endlessly.
>>
>>
>>
>>> You mention iterations over things in another email.  As I see it, our
>>> code
>>> doesn't yet do a good enough job of decoupling the persistent entities in
>>> the data (tracks and their contents) from the view entities in the display
>>>
>> +1
>>
>>> (so, as you say, we ought to be freer to make many-to-one
>>> correspondences).
>>>
>>> So better abstracted iteration over both kinds of entity would be
>>> valuable.  And for the latter kind, display, we are making progress right
>>> now.
>>>
>>> Please look at TrackPanelCellIterator in head.  I made changes to this
>>> class only yesterday.
>>>
>> Yes.  That's partly what prompted my question.
>>
>>>    The purpose of this class is iteration over the
>>> disjoint rectangular areas of TrackPanel that have different policies for
>>> drawing themselves and for dispatching events when the pointer is in them.
>>>
>>> I want to concentrate the knowledge of just what the screen-partitioning
>>> policy is, into one class, that you might replace in order to define a
>>> different policy.  This is an advancement toward that.
>>>
>> Cool.
>>
>>
>> PRL
>>> --James.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: GetLinked()

Paul Licameli


On Sat, Jun 17, 2017 at 2:29 PM, James Crook <[hidden email]> wrote:
On 6/17/2017 6:47 PM, Paul Licameli wrote:
On Sat, Jun 17, 2017 at 1:34 PM, James Crook <[hidden email]> wrote:

On 6/17/2017 4:23 PM, Paul Licameli wrote:

On Sat, Jun 17, 2017 at 5:11 AM, James Crook <[hidden email]> wrote:

Paul, are we going to banish GetLinked() for 2.2.0 or wait until a later
release?

My present thinking is no, this would be an overreach for 2.2.0.  I would
rather like to get it ready as a big-bang for the start of 2.2.1.  Which
could be stabilized sooner than otherwise, without these changes.

Why do you ask now?  Do you want to persuade me otherwise?  Do you want to
rebase some of your pending work onto it?

I asked because of noticing work related to GetLinked() in
VisibleTrackIterator:: and wondering what your plan was.
I'd like to persuade you to do it early on in 2.2.1.

There is a possible case for doing it sooner, as current HEAD, despite
your best efforts in the refactoring, seems rather buggy,

What are you referring to?

Broken keystroke dispatching has been fixed.

Peter has reported crashes on Windows for menu items, still to investigate.
I hit that one too, for TCP menu.
I also hit a 'sequence error' exception when doing a cut, which unfortunately I could not make repeatable.  I can't find the text 'sequence error' in the code, so I'm guessing I mis rememebered what the error message dialog said.

You may have hit on a reproduction of http://bugzilla.audacityteam.org/show_bug.cgi?id=1655

which seems not a consequence of the track panel refactor, though it is one of my earlier 2.2.0 work.

So again, it seems there are loose ends enough to tidy up without taking on the track iteration project too.

PRL

 
I also hit bug http://bugzilla.audacityteam.org/show_bug.cgi?id=1663 which it turns out is actually a bug in 2.1.3, so not new..

So that is 3 significant distinct bugs in a relatively short spell of use, suggesting (at the time) that the rearrangement has introduced half a dozen or so....  As 1663 predates the refactoring, I was mistaken in counting it.


Still, these are "gross" problems that have obvious solutions.  Question is
whether there is much both gross and subtle.

You see how I want to allow time to shake bugs out of things like this.
Yes.
To get the other iteration work even to the point of testing by Gale and Peter
is going to require nontrivial effort of conflict resolution on my part.
So again I'm inclined to say no, let this go until 2.2.1.
Sounds sensible to me too.
--James



PRL



and so will need a lot of testing.  There is a possible case that the
GetLinked code refactoring is dangerous too, and so possibly a case to get
one lot of testing covering both refactorings.  Counter arguments include
that more changes means harder bug hunting, and overall it would assuredly
affect schedule.  I think it is sufficiently risky/big not to do it now.

Nope, there's not pending work of mine that would benefit from rebasing on
it.  Main concern is that you don't have to keep rebasing it (with
conflicts) to carry it forward.



Should I go for broke pushing all my pervasive disruptions into one longer
delayed version for one kick-the-tyres?  (I don't think so.)

I don't think so either.



What I have done so far is in a branch called track-iters2.  Not only are
there far fewer calls to GetLink(), but also every iteration over tracks
was rewritten in an idiom that is less verbose and also type- and
const-correct through some use of C++ templates.  I didn't like seeing
C-style casts like ((WaveTrack*)t) and those are all gone.  Instead you
can
say something like:  for (auto t : trackList->Selected<const WaveTrack>) {
... }

I did bring that branch up to date with early 2.2.0 developments and test
it to the extent of putting a breakpoint at every changed place and
hitting
it.

But much work would remain, between that and TrackPanel refactoring, to
rebase one onto the other and resolve conflicts.  I decided that the
TrackPanel refactor was the more valuable thing that I ought to press on
with first.  So I knuckled down to the task of reviewing all my forgotten
two year old code, and that hurdle is thankfully past.

I'm glad too, as you should not be carrying that refactoring forward
endlessly.



You mention iterations over things in another email.  As I see it, our
code
doesn't yet do a good enough job of decoupling the persistent entities in
the data (tracks and their contents) from the view entities in the display

+1

(so, as you say, we ought to be freer to make many-to-one
correspondences).

So better abstracted iteration over both kinds of entity would be
valuable.  And for the latter kind, display, we are making progress right
now.

Please look at TrackPanelCellIterator in head.  I made changes to this
class only yesterday.

Yes.  That's partly what prompted my question.

   The purpose of this class is iteration over the
disjoint rectangular areas of TrackPanel that have different policies for
drawing themselves and for dispatching events when the pointer is in them.

I want to concentrate the knowledge of just what the screen-partitioning
policy is, into one class, that you might replace in order to define a
different policy.  This is an advancement toward that.

Cool.


PRL
--James.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: GetLinked()

James Crook
On 6/17/2017 7:54 PM, Paul Licameli wrote:

> On Sat, Jun 17, 2017 at 2:29 PM, James Crook <[hidden email]> wrote:
>
>> On 6/17/2017 6:47 PM, Paul Licameli wrote:
>>
>>> On Sat, Jun 17, 2017 at 1:34 PM, James Crook <[hidden email]> wrote:
>>>
>>> On 6/17/2017 4:23 PM, Paul Licameli wrote:
>>>> On Sat, Jun 17, 2017 at 5:11 AM, James Crook <[hidden email]> wrote:
>>>>> Paul, are we going to banish GetLinked() for 2.2.0 or wait until a later
>>>>>
>>>>>> release?
>>>>>>
>>>>>> My present thinking is no, this would be an overreach for 2.2.0.  I
>>>>> would
>>>>> rather like to get it ready as a big-bang for the start of 2.2.1.  Which
>>>>> could be stabilized sooner than otherwise, without these changes.
>>>>>
>>>>> Why do you ask now?  Do you want to persuade me otherwise?  Do you want
>>>>> to
>>>>> rebase some of your pending work onto it?
>>>>>
>>>>> I asked because of noticing work related to GetLinked() in
>>>> VisibleTrackIterator:: and wondering what your plan was.
>>>> I'd like to persuade you to do it early on in 2.2.1.
>>>>
>>>> There is a possible case for doing it sooner, as current HEAD, despite
>>>> your best efforts in the refactoring, seems rather buggy,
>>>>
>>> What are you referring to?
>>>
>>> Broken keystroke dispatching has been fixed.
>>>
>>> Peter has reported crashes on Windows for menu items, still to
>>> investigate.
>>>
>> I hit that one too, for TCP menu.
>> I also hit a 'sequence error' exception when doing a cut, which
>> unfortunately I could not make repeatable.  I can't find the text 'sequence
>> error' in the code, so I'm guessing I mis rememebered what the error
>> message dialog said.
>>
> You may have hit on a reproduction of
> http://bugzilla.audacityteam.org/show_bug.cgi?id=1655
>
> which seems not a consequence of the track panel refactor, though it is one
> of my earlier 2.2.0 work.
Yes, that's the one.  Glad it is reproducable.


>
> So again, it seems there are loose ends enough to tidy up without taking on
> the track iteration project too.
>
> PRL
>


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Jaws Crashes (was: GetLinked())

Paul Licameli
In reply to this post by Paul Licameli


On Sat, Jun 17, 2017 at 12:23 PM, Paul Licameli <[hidden email]> wrote:


On Sat, Jun 17, 2017 at 6:42 AM, James Crook <[hidden email]> wrote:
David,

I'm relatively new to the party in terms of looking at actual screen reader behaviour.  I notice that Audacity with Narrator on leaks memory like a sieve.  Paul's cleaning up of naked news means non-narrator use does not memory leak, so it makes these leaks more obvious.  That leads me to think that wxWidgets with any screen reader is code that is less well exercised and tested, i.e. expect many bugs relating to screen readers in wxWidgets. Probably we can find workarounds, staying on the parts that do work, rather than complete fixes.  But we may only be able to ameliorate the problems rather than fix them.

I talked with Leland about screen readers at AU14 conference, and we both were keen to provide access to Audacity algorithms at more of a command line like level to get better accessibility.  Long term this is still in my view the best way to improve accessibility of Audacity - i.e. have an alternative front end for the same audio engine.  Have the normal GUI which makes some assumptions about a user using a mouse, and a more command line GUI that still has menus for access, but that is geared for automation and keyboard use - with explicit binding of mouse wheel and x-y to controlled values rather than implicit binding by click and drag.


IF frequent repainting is causing problems for Jaws, we can / should ameliorate by removing redundant repaintings.  That will be better for all.  Audacity has a somewhat Jekyll and Hyde attitude to repainting.  It has event based repainting and periodic painting off a timer.  At some future time we should clean that up too.

The Windows 10 creators update has done some strange things at start up.  The splash screen now appears sooner, and so it appears before it has been positioned.  We have some strange code with the splash screen to work around a problem with the possibility of having splash screen and error dialog at the same time, before the app has fully started.  There should not be a need for our own event loop (which is what we have).  I think our workarounds maybe should be untangled.  As it is we show and hide our splash screen rather quickly now, so it does not serve the role of a splash screen, that reassures you that the app is starting up.

This doesn't directly answer the question of Jaws crashes, but may be useful none the less.

--James.


On 6/17/2017 10:52 AM, David Bailes wrote:
On Sat, Jun 17, 2017 at 10:11 AM, James Crook <[hidden email]> wrote:

Paul, are we going to banish GetLinked() for 2.2.0 or wait until a later
release?

James, is this a change to how stereo tracks are handled? What is the new
design going to be?

I've been looking at why Audacity is crashing when Jaws is also running on
the Windows 10 creators update.

How reliably can you make these crashes happen?

I notice this comment in TrackPanel.cpp, which dates to Jan 2010 or earlier -- I can't discover who wrote it first in the confusing git repository for the older commits.  I think it was new in version 1.3.9.

   // The check for a null linked track is necessary because there's

   // a possible race condition between the time the 2 linked tracks

   // are added and when wxAccessible methods are called.  This is

   // most evident when using Jaws.


So this suggests multi-threading was involved.  This makes me suspect that the bare pointer, Track *TrackPanelAx::mFocusedTrack, is the problematic dangling pointer here.

And this makes me suspect that a solution would involve changing our management of Track objects everywhere to use std::shared_ptr rather that std::unique_ptr.  That might not be as hard as it sounds -- it could be only a matter of changint some "using" type aliases.

Then, TrackPanelAx should point to its track by a shared_ptr.

This would not be sufficient:  uses of that shared_ptr member of TrackPanelAx should then themselves be guarded by a mutex.  Standard share_ptr gives you thread safety for free when threads each use different shared_ptrs to the shared object, but not when threads contend for the same shared_ptr.  That is explained here:  http://en.cppreference.com/w/cpp/memory/shared_ptr

Is this a sufficient description, and do you understand C++11 smart pointers enough, that you might attempt this rewrite yourself?

PRL

David, have you explored my suggestion?

PRL

 


 
When Jaws is running there appear to be more paint messages being sent to
Audacity, though it's unclear to me what is causing this.
For example when deleting a stereo track, a paint message can occur after
one of the tracks has been removed, and this causes audacity to  crash.
Regardless of whether these extra paint message should be occurring, the
code for removing stereo tracks sends update events after each of the two
tracks is removed from the track list, rather than a single update event
after both tracks have be removed. This doesn't seem right.

Hence my interest in any changes to the design of stereo tracks.

David.


--James.


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Jaws Crashes (was: GetLinked())

Paul Licameli


On Thu, Jun 22, 2017 at 7:31 PM, Paul Licameli <[hidden email]> wrote:


On Sat, Jun 17, 2017 at 12:23 PM, Paul Licameli <[hidden email]> wrote:


On Sat, Jun 17, 2017 at 6:42 AM, James Crook <[hidden email]> wrote:
David,

I'm relatively new to the party in terms of looking at actual screen reader behaviour.  I notice that Audacity with Narrator on leaks memory like a sieve.  Paul's cleaning up of naked news means non-narrator use does not memory leak, so it makes these leaks more obvious.  That leads me to think that wxWidgets with any screen reader is code that is less well exercised and tested, i.e. expect many bugs relating to screen readers in wxWidgets. Probably we can find workarounds, staying on the parts that do work, rather than complete fixes.  But we may only be able to ameliorate the problems rather than fix them.

I talked with Leland about screen readers at AU14 conference, and we both were keen to provide access to Audacity algorithms at more of a command line like level to get better accessibility.  Long term this is still in my view the best way to improve accessibility of Audacity - i.e. have an alternative front end for the same audio engine.  Have the normal GUI which makes some assumptions about a user using a mouse, and a more command line GUI that still has menus for access, but that is geared for automation and keyboard use - with explicit binding of mouse wheel and x-y to controlled values rather than implicit binding by click and drag.


IF frequent repainting is causing problems for Jaws, we can / should ameliorate by removing redundant repaintings.  That will be better for all.  Audacity has a somewhat Jekyll and Hyde attitude to repainting.  It has event based repainting and periodic painting off a timer.  At some future time we should clean that up too.

The Windows 10 creators update has done some strange things at start up.  The splash screen now appears sooner, and so it appears before it has been positioned.  We have some strange code with the splash screen to work around a problem with the possibility of having splash screen and error dialog at the same time, before the app has fully started.  There should not be a need for our own event loop (which is what we have).  I think our workarounds maybe should be untangled.  As it is we show and hide our splash screen rather quickly now, so it does not serve the role of a splash screen, that reassures you that the app is starting up.

This doesn't directly answer the question of Jaws crashes, but may be useful none the less.

--James.


On 6/17/2017 10:52 AM, David Bailes wrote:
On Sat, Jun 17, 2017 at 10:11 AM, James Crook <[hidden email]> wrote:

Paul, are we going to banish GetLinked() for 2.2.0 or wait until a later
release?

James, is this a change to how stereo tracks are handled? What is the new
design going to be?

I've been looking at why Audacity is crashing when Jaws is also running on
the Windows 10 creators update.

How reliably can you make these crashes happen?

I notice this comment in TrackPanel.cpp, which dates to Jan 2010 or earlier -- I can't discover who wrote it first in the confusing git repository for the older commits.  I think it was new in version 1.3.9.

   // The check for a null linked track is necessary because there's

   // a possible race condition between the time the 2 linked tracks

   // are added and when wxAccessible methods are called.  This is

   // most evident when using Jaws.


So this suggests multi-threading was involved.  This makes me suspect that the bare pointer, Track *TrackPanelAx::mFocusedTrack, is the problematic dangling pointer here.

And this makes me suspect that a solution would involve changing our management of Track objects everywhere to use std::shared_ptr rather that std::unique_ptr.  That might not be as hard as it sounds -- it could be only a matter of changint some "using" type aliases.

Then, TrackPanelAx should point to its track by a shared_ptr.

This would not be sufficient:  uses of that shared_ptr member of TrackPanelAx should then themselves be guarded by a mutex.  Standard share_ptr gives you thread safety for free when threads each use different shared_ptrs to the shared object, but not when threads contend for the same shared_ptr.  That is explained here:  http://en.cppreference.com/w/cpp/memory/shared_ptr

Is this a sufficient description, and do you understand C++11 smart pointers enough, that you might attempt this rewrite yourself?

PRL

David, have you explored my suggestion?

PRL


I asked because I decided that I want to manage Track objects by shared_ptr for other reasons too.  I figured out the necessary changes myself and will push them soon.

I am also considering the problem of possible race conditions in TrackPanelAx used by other threads.  It was hard to resist some coding here.  This is less easy to reason about.

I think there are three sources of races that might explain the crashes in JAWS.

First, it may be that TrackPanel is being destroyed, but methods of TrackPanelAx try to use it.  But this is unlikely to expain crashes that happen at other times than closing of windows.

Secondly, TrackPanelAx methods may iterate the list of tracks, while that list is changing because of addition, deletion, or permutation.  Perhaps this only causes an incorrect result in TrackPanelAx queries, but perhaps it also causes a dangling pointer to be followed.

Thirdly, TrackPanelAx stores a pointer to a track, but that track is being destroyed, or has been.  Using a shared_ptr would prevent this, but then uses of that shared_ptr must also be serialized.

So there would be problems in defining mutexes and locking them in all places, and then we must also make sure that this does not cause deadlocks.

PRL

 
 


 
When Jaws is running there appear to be more paint messages being sent to
Audacity, though it's unclear to me what is causing this.
For example when deleting a stereo track, a paint message can occur after
one of the tracks has been removed, and this causes audacity to  crash.
Regardless of whether these extra paint message should be occurring, the
code for removing stereo tracks sends update events after each of the two
tracks is removed from the track list, rather than a single update event
after both tracks have be removed. This doesn't seem right.

Hence my interest in any changes to the design of stereo tracks.

David.


--James.


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel




------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Jaws Crashes (was: GetLinked())

David Bailes-3
In reply to this post by Paul Licameli
On Sat, Jun 17, 2017 at 5:23 PM, Paul Licameli <[hidden email]> wrote:


On Sat, Jun 17, 2017 at 6:42 AM, James Crook <[hidden email]> wrote:
David,

I'm relatively new to the party in terms of looking at actual screen reader behaviour.  I notice that Audacity with Narrator on leaks memory like a sieve.  Paul's cleaning up of naked news means non-narrator use does not memory leak, so it makes these leaks more obvious.  That leads me to think that wxWidgets with any screen reader is code that is less well exercised and tested, i.e. expect many bugs relating to screen readers in wxWidgets. Probably we can find workarounds, staying on the parts that do work, rather than complete fixes.  But we may only be able to ameliorate the problems rather than fix them.

I talked with Leland about screen readers at AU14 conference, and we both were keen to provide access to Audacity algorithms at more of a command line like level to get better accessibility.  Long term this is still in my view the best way to improve accessibility of Audacity - i.e. have an alternative front end for the same audio engine.  Have the normal GUI which makes some assumptions about a user using a mouse, and a more command line GUI that still has menus for access, but that is geared for automation and keyboard use - with explicit binding of mouse wheel and x-y to controlled values rather than implicit binding by click and drag.


IF frequent repainting is causing problems for Jaws, we can / should ameliorate by removing redundant repaintings.  That will be better for all.  Audacity has a somewhat Jekyll and Hyde attitude to repainting.  It has event based repainting and periodic painting off a timer.  At some future time we should clean that up too.

The Windows 10 creators update has done some strange things at start up.  The splash screen now appears sooner, and so it appears before it has been positioned.  We have some strange code with the splash screen to work around a problem with the possibility of having splash screen and error dialog at the same time, before the app has fully started.  There should not be a need for our own event loop (which is what we have).  I think our workarounds maybe should be untangled.  As it is we show and hide our splash screen rather quickly now, so it does not serve the role of a splash screen, that reassures you that the app is starting up.

This doesn't directly answer the question of Jaws crashes, but may be useful none the less.

--James.


On 6/17/2017 10:52 AM, David Bailes wrote:
On Sat, Jun 17, 2017 at 10:11 AM, James Crook <[hidden email]> wrote:

Paul, are we going to banish GetLinked() for 2.2.0 or wait until a later
release?

James, is this a change to how stereo tracks are handled? What is the new
design going to be?

I've been looking at why Audacity is crashing when Jaws is also running on
the Windows 10 creators update.

How reliably can you make these crashes happen?

They are reproducible, and can occur in a number of different circumstances.

I notice this comment in TrackPanel.cpp, which dates to Jan 2010 or earlier -- I can't discover who wrote it first in the confusing git repository for the older commits.  I think it was new in version 1.3.9.

   // The check for a null linked track is necessary because there's

   // a possible race condition between the time the 2 linked tracks

   // are added and when wxAccessible methods are called.  This is

   // most evident when using Jaws.


So this suggests multi-threading was involved.

The problem was that an object_focus event was sent for the first track of a stereo track pair when the second track had not been added to the track list.
  
  This makes me suspect that the bare pointer, Track *TrackPanelAx::mFocusedTrack, is the problematic dangling pointer here.

I don't think that was the problem.

David.
 

And this makes me suspect that a solution would involve changing our management of Track objects everywhere to use std::shared_ptr rather that std::unique_ptr.  That might not be as hard as it sounds -- it could be only a matter of changint some "using" type aliases.

Then, TrackPanelAx should point to its track by a shared_ptr.

This would not be sufficient:  uses of that shared_ptr member of TrackPanelAx should then themselves be guarded by a mutex.  Standard share_ptr gives you thread safety for free when threads each use different shared_ptrs to the shared object, but not when threads contend for the same shared_ptr.  That is explained here:  http://en.cppreference.com/w/cpp/memory/shared_ptr

Is this a sufficient description, and do you understand C++11 smart pointers enough, that you might attempt this rewrite yourself?

PRL


 
When Jaws is running there appear to be more paint messages being sent to
Audacity, though it's unclear to me what is causing this.
For example when deleting a stereo track, a paint message can occur after
one of the tracks has been removed, and this causes audacity to  crash.
Regardless of whether these extra paint message should be occurring, the
code for removing stereo tracks sends update events after each of the two
tracks is removed from the track list, rather than a single update event
after both tracks have be removed. This doesn't seem right.

Hence my interest in any changes to the design of stereo tracks.

David.


--James.


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Jaws Crashes (was: GetLinked())

David Bailes-3
In reply to this post by Paul Licameli
On Fri, Jun 23, 2017 at 1:19 PM, Paul Licameli <[hidden email]> wrote:


On Thu, Jun 22, 2017 at 7:31 PM, Paul Licameli <[hidden email]> wrote:


On Sat, Jun 17, 2017 at 12:23 PM, Paul Licameli <[hidden email]> wrote:


On Sat, Jun 17, 2017 at 6:42 AM, James Crook <[hidden email]> wrote:
David,

I'm relatively new to the party in terms of looking at actual screen reader behaviour.  I notice that Audacity with Narrator on leaks memory like a sieve.  Paul's cleaning up of naked news means non-narrator use does not memory leak, so it makes these leaks more obvious.  That leads me to think that wxWidgets with any screen reader is code that is less well exercised and tested, i.e. expect many bugs relating to screen readers in wxWidgets. Probably we can find workarounds, staying on the parts that do work, rather than complete fixes.  But we may only be able to ameliorate the problems rather than fix them.

I talked with Leland about screen readers at AU14 conference, and we both were keen to provide access to Audacity algorithms at more of a command line like level to get better accessibility.  Long term this is still in my view the best way to improve accessibility of Audacity - i.e. have an alternative front end for the same audio engine.  Have the normal GUI which makes some assumptions about a user using a mouse, and a more command line GUI that still has menus for access, but that is geared for automation and keyboard use - with explicit binding of mouse wheel and x-y to controlled values rather than implicit binding by click and drag.


IF frequent repainting is causing problems for Jaws, we can / should ameliorate by removing redundant repaintings.  That will be better for all.  Audacity has a somewhat Jekyll and Hyde attitude to repainting.  It has event based repainting and periodic painting off a timer.  At some future time we should clean that up too.

The Windows 10 creators update has done some strange things at start up.  The splash screen now appears sooner, and so it appears before it has been positioned.  We have some strange code with the splash screen to work around a problem with the possibility of having splash screen and error dialog at the same time, before the app has fully started.  There should not be a need for our own event loop (which is what we have).  I think our workarounds maybe should be untangled.  As it is we show and hide our splash screen rather quickly now, so it does not serve the role of a splash screen, that reassures you that the app is starting up.

This doesn't directly answer the question of Jaws crashes, but may be useful none the less.

--James.


On 6/17/2017 10:52 AM, David Bailes wrote:
On Sat, Jun 17, 2017 at 10:11 AM, James Crook <[hidden email]> wrote:

Paul, are we going to banish GetLinked() for 2.2.0 or wait until a later
release?

James, is this a change to how stereo tracks are handled? What is the new
design going to be?

I've been looking at why Audacity is crashing when Jaws is also running on
the Windows 10 creators update.

How reliably can you make these crashes happen?

I notice this comment in TrackPanel.cpp, which dates to Jan 2010 or earlier -- I can't discover who wrote it first in the confusing git repository for the older commits.  I think it was new in version 1.3.9.

   // The check for a null linked track is necessary because there's

   // a possible race condition between the time the 2 linked tracks

   // are added and when wxAccessible methods are called.  This is

   // most evident when using Jaws.


So this suggests multi-threading was involved.  This makes me suspect that the bare pointer, Track *TrackPanelAx::mFocusedTrack, is the problematic dangling pointer here.

And this makes me suspect that a solution would involve changing our management of Track objects everywhere to use std::shared_ptr rather that std::unique_ptr.  That might not be as hard as it sounds -- it could be only a matter of changint some "using" type aliases.

Then, TrackPanelAx should point to its track by a shared_ptr.

This would not be sufficient:  uses of that shared_ptr member of TrackPanelAx should then themselves be guarded by a mutex.  Standard share_ptr gives you thread safety for free when threads each use different shared_ptrs to the shared object, but not when threads contend for the same shared_ptr.  That is explained here:  http://en.cppreference.com/w/cpp/memory/shared_ptr

Is this a sufficient description, and do you understand C++11 smart pointers enough, that you might attempt this rewrite yourself?

PRL

David, have you explored my suggestion?

PRL


I asked because I decided that I want to manage Track objects by shared_ptr for other reasons too.  I figured out the necessary changes myself and will push them soon.

I am also considering the problem of possible race conditions in TrackPanelAx used by other threads.  It was hard to resist some coding here.  This is less easy to reason about.

I think there are three sources of races that might explain the crashes in JAWS.

I don't think that these sources of races are the problem (with one exception noted below).
The main problem seems to be that when Jaws is running, audacity is getting more paint messages - no idea why.
Some of these messages can occur at times which cause the subsequent drawing to crash audacity due to null or dangling pointers. For example:
1. When closing an audacity window, there are a couple of causes of crashes, but these can be fixed by small modifications to the way/order objects are deleted.
2. When a stereo track is added, there is a similar problem to the one that Leland fixed - drawing when only one track of a stereo pair has been added to the track list.
 

First, it may be that TrackPanel is being destroyed, but methods of TrackPanelAx try to use it.  But this is unlikely to expain crashes that happen at other times than closing of windows.

Secondly, TrackPanelAx methods may iterate the list of tracks, while that list is changing because of addition, deletion, or permutation.  Perhaps this only causes an incorrect result in TrackPanelAx queries, but perhaps it also causes a dangling pointer to be followed.

Thirdly, TrackPanelAx stores a pointer to a track, but that track is being destroyed, or has been.  Using a shared_ptr would prevent this, but then uses of that shared_ptr must also be serialized.

There is a problem with deleting a stereo track, but the problem appears to be that an object_focus event for the track is sent for the track, and then the track is immediately deleted. However, the problem is that that object_focus event should not have been sent, and the solution is simply not to send it.
 

So there would be problems in defining mutexes and locking them in all places, and then we must also make sure that this does not cause deadlocks.

I don't think that adding mutexes is solving a problem, and it would just make the code unnecessarily complicated.

David.

 

PRL

 
 


 
When Jaws is running there appear to be more paint messages being sent to
Audacity, though it's unclear to me what is causing this.
For example when deleting a stereo track, a paint message can occur after
one of the tracks has been removed, and this causes audacity to  crash.
Regardless of whether these extra paint message should be occurring, the
code for removing stereo tracks sends update events after each of the two
tracks is removed from the track list, rather than a single update event
after both tracks have be removed. This doesn't seem right.

Hence my interest in any changes to the design of stereo tracks.

David.


--James.


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel




------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel



------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
audacity-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/audacity-devel
123
Loading...