Closed Bug 1134078 Opened 5 years ago Closed 5 years ago

cubeb's OnDefaultDeviceChanged handler can restart stopped streams

Categories

(Core :: Audio/Video, defect)

All
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: kinetik, Assigned: kinetik)

References

Details

Attachments

(1 file, 2 obsolete files)

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 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)
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
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
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Blocks: 698079
Depends on: 1136107
Depends on: 1144199
You need to log in before you can comment on or make changes to this bug.