cubeb's OnDefaultDeviceChanged handler can restart stopped streams

RESOLVED FIXED in Firefox 38

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kinetik, Assigned: kinetik)

Tracking

Trunk
mozilla38
All
Windows 8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
Created attachment 8565795 [details] [diff] [review]
Don't restart stopped cubeb streams when handling device change notifications
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.
Attachment #8565795 - Flags: review?(padenot)
Created attachment 8565798 [details] [diff] [review]
Don't restart stopped cubeb streams when handling device change notifications

Minor build fix; applied patch copied from another tree incorrectly.
Attachment #8565798 - Flags: review?(padenot)
Attachment #8565795 - Attachment is obsolete: true
Attachment #8565795 - Flags: review?(padenot)
Looks like the tests are hanging on shutdown, will investigate after dinner.
Missed updating an assert, but I can't see it firing in the try logs...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=60b0ad5dd053
Created attachment 8565908 [details] [diff] [review]
bug1134078_v1.patch
Attachment #8565798 - Attachment is obsolete: true
Attachment #8565798 - Flags: review?(padenot)
Attachment #8565908 - Flags: review?(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.
Attachment #8565908 - Flags: review?(padenot) → review+
(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).
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.
https://hg.mozilla.org/mozilla-central/rev/425f60223625
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1136107
Depends on: 1144199
You need to log in before you can comment on or make changes to this bug.