1134078 - cubeb's OnDefaultDeviceChanged handler can restart stopped streams

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Matthew Gregan [:kinetik]]

If a cubeb_stream is stopped when a default device change event is received, the existing handler reconfigures *and* restarts the stream.  The handler should pay attention to the current state of the stream rather than blindly restarting it.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: Don't restart stopped cubeb streams when handling device change notifications

Comment 2

[Matthew Gregan [:kinetik]]

Comment on attachment 8565795 [details] [diff] [review]
Don't restart stopped cubeb streams when handling device change notifications

This fixes a few issues I ran into during recent testing.  Restarting the stream when it's stopped caused various problems all over the media code, including leaked render threads and a fatal assert on EOS.

While working on this, I noticed that OnDefaultDeviceChanged seems to be called twice on my Windows 8.1 machine for each change.  The flow, role, and device_id are identical on each call.  Other than being suboptimal, I haven't found any problems (or a way to prevent it) yet.

Comment 3

[Matthew Gregan [:kinetik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cfda5faf682

Comment 4

[Matthew Gregan [:kinetik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ca3ec4e5497

Comment 5

[Matthew Gregan [:kinetik]]

Attachment: Don't restart stopped cubeb streams when handling device change notifications

Minor build fix; applied patch copied from another tree incorrectly.

Comment 6

[Matthew Gregan [:kinetik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3925c5e41091

Comment 7

[Matthew Gregan [:kinetik]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=462926ffcd1b

Comment 8

[Matthew Gregan [:kinetik]]

Looks like the tests are hanging on shutdown, will investigate after dinner.

Comment 9

[Matthew Gregan [:kinetik]]

Missed updating an assert, but I can't see it firing in the try logs...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=60b0ad5dd053

Comment 10

[Matthew Gregan [:kinetik]]

Attachment: bug1134078_v1.patch

Comment 11

[Paul Adenot (:padenot)]

Comment on attachment 8565908 [details] [diff] [review]
bug1134078_v1.patch

Review of attachment 8565908 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/libcubeb/src/cubeb_wasapi.cpp
@@ +30,5 @@
>  #ifndef STACK_SIZE_PARAM_IS_A_RESERVATION
>  #define STACK_SIZE_PARAM_IS_A_RESERVATION   0x00010000    // Threads only
>  #endif
>  
> +#define LOGGING_ENABLED

(don't forget to revert this)

@@ +585,5 @@
>      }
>      break;
> +    case WAIT_TIMEOUT:
> +      assert(GetThreadId(stm->thread) == GetCurrentThreadId() &&
> +             stm->shutdown_event == wait_array[0]);

Do we want to call the state callback if this happens? My patch for this set hr to a negative value so we fell in the CUBEB_STATE_STOPPED below.

Comment 12

[Matthew Gregan [:kinetik]]

(In reply to Paul Adenot (:padenot) from comment #11)
> Do we want to call the state callback if this happens? My patch for this set
> hr to a negative value so we fell in the CUBEB_STATE_STOPPED below.

Yeah, that makes sense to me.  I was assuming we'd end up merging both patches, net addition from me then being just the assert (hoping to gain a lead on bug 1108793).

Comment 13

[Matthew Gregan [:kinetik]]

FWIW the XP mochitests were failing because GetThreadId() isn't available before Vista, so there was a popup on the desktop with a dynamic linker error... took me a while to track that down.

Also, per comment 8 and bug 1133386, the asserts didn't show up in the log because they also cause a popup on the desktop.

Comment 14

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/425f60223625

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/425f60223625