1136360 - AV sync late on Windows

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P2)

Description

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

STR:

* Navigate to URL
* Watch AV sync flashes

Expected results:

Audio and video are in sync

Actual results:

Audio is delayed

Comment 1

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Required bluetooth headset

Comment 2

[Paul Adenot (:padenot)]

We need to offset the clock like so [0]. We can't rely on IAudioClock alone, because there is a non-predictable delay when switching devices/reseting streams, so we need to do this little delay prediction dance.

[0]: http://mxr.mozilla.org/chromium/source/src/media/audio/win/audio_low_latency_output_win.cc#473

Comment 3

[Paul Adenot (:padenot)]

To properly do this, we need to do the following:
- Switch to storing the `stm->clock` in mixing rate, and not stream rate. Accumulating rounded value still causes a drift, I think we need to accumulate the frames in mixing-rate, and convert in wasapi_stream_get_position.
- When switching device (and resetting the stream), store `stm->clock` in `stm->base_clock` (which is initially 0). We do this on the rendering thread now, so we'll have no clock mismatch issue.
- Get the audio frames that's currently being output by the speaker using IAudioClock, store that in `stm->out_clock`, and compare that to `stm->clock - stm->base_clock` to get the latency value (that I quickly implemented, it works fine, numbers are good).
- In wasapi_stream_get_position, return `max(stm->clock - stm->latency, 0) * stream_to_mix_samplerate_ratio(stm)`: round at the end, and substract the latency, clamped to zero.

I think that will remove all the accumulation of rounded value (which, I think, are the cause of the smaller clock drift causing A/V desync reported yesterday on IRC by Callek) and give us an accurate latency value. I don't have a bluetooth headset handy today to properly test, I'll see tomorrow. I'm also concerned about switching to a high-latency device from a low-latency device, and having a non-monotonic clock because the latency is now higher than the number of frames we received last callback. I'll try to break in the debugger to see if IAudioClock gradually adjusts the latency.

Matthew, does that look like a reasonable plan?

Comment 4

[Matthew Gregan [:kinetik]]

Sounds good!  Thanks.

Comment 5

[Paul Adenot (:padenot)]

(In reply to Paul Adenot (:padenot) from comment #3)
> To properly do this, we need to do the following:
> - Switch to storing the `stm->clock` in mixing rate, and not stream rate.
> Accumulating rounded value still causes a drift, I think we need to
> accumulate the frames in mixing-rate, and convert in
> wasapi_stream_get_position.

I forgot about the problem that doing the accumulation in mixing rate causes all sorts of problems when the mixing rate changes...

Maybe accumulating values in a float or a double would fix the drift, and then we can simply offset the latency?

Comment 6

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Would offsetting the latency be a faster path to "good enough"?

Comment 7

[Paul Adenot (:padenot)]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #6)
> Would offsetting the latency be a faster path to "good enough"?

Certainly. That said I'm planning to upload patches for both issues today.

Comment 8

[Paul Adenot (:padenot)]

Attachment: patch

This works, but I want to get/set the latency and clock together I think.

Comment 9

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

(In reply to Paul Adenot (:padenot) from comment #7)
> (In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #6)
> > Would offsetting the latency be a faster path to "good enough"?
> 
> Certainly. That said I'm planning to upload patches for both issues today.

You're a star! Thanks!

Comment 10

[Paul Adenot (:padenot)]

Comment on attachment 8573364 [details] [diff] [review]
patch

yeah in fact putting those two together is not really better.

Comment 11

[Matthew Gregan [:kinetik]]

Comment on attachment 8573364 [details] [diff] [review]
patch

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

r+ with comments

::: media/libcubeb/src/cubeb_wasapi.cpp
@@ +239,5 @@
>    /* Each cubeb_stream has its own thread. */
>    HANDLE thread;
> +  /* We synthesize our clock from the callbacks. This in fractional frames, in
> +   * the stream samplerate. */
> +  double clock;

Isn't this also in the mix samplerate?

@@ +242,5 @@
> +   * the stream samplerate. */
> +  double clock;
> +  /* This is the clock value last time we reset the stream. This is in
> +   * fractional frames, in the mix samplerate. */
> +  double base_clock;

The comments here are confusing because clock and base_clock are used together without any conversions, suggesting they're in the same units.

@@ +1139,5 @@
>    stm->stream_params = stream_params;
>    stm->draining = false;
>    stm->latency = latency;
> +  stm->clock = 0.;
> +  stm->base_clock = 0.;

Minor nit: 0 or 0.0 please.

@@ +1291,5 @@
>  int wasapi_stream_get_position(cubeb_stream * stm, uint64_t * position)
>  {
>    XASSERT(stm && position);
>  
> +  *position = max(0, clock_get(stm) - latency_get(stm));

It's a bit odd that clock_get and latency_get return different types... and do we care about rounding vs truncation here?

Comment 12

[Paul Adenot (:padenot)]

(In reply to Matthew Gregan [:kinetik] from comment #11)
> Comment on attachment 8573364 [details] [diff] [review]
> patch
> 
> Review of attachment 8573364 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comments
> 
> ::: media/libcubeb/src/cubeb_wasapi.cpp
> @@ +239,5 @@
> >    /* Each cubeb_stream has its own thread. */
> >    HANDLE thread;
> > +  /* We synthesize our clock from the callbacks. This in fractional frames, in
> > +   * the stream samplerate. */
> > +  double clock;
> 
> Isn't this also in the mix samplerate?
> 
> @@ +242,5 @@
> > +   * the stream samplerate. */
> > +  double clock;
> > +  /* This is the clock value last time we reset the stream. This is in
> > +   * fractional frames, in the mix samplerate. */
> > +  double base_clock;
> 
> The comments here are confusing because clock and base_clock are used
> together without any conversions, suggesting they're in the same units.

They are both in the stream sample rate, I forgot to update the second occurrence.

Thing is, we can only do the accumulation in stream sample rate (that is constant during the life time of the cubeb_stream), otherwise, we would have to track the sample rate changes when plugging/unplugging devices with different sample rates.

> @@ +1291,5 @@
> >  int wasapi_stream_get_position(cubeb_stream * stm, uint64_t * position)
> >  {
> >    XASSERT(stm && position);
> >  
> > +  *position = max(0, clock_get(stm) - latency_get(stm));
> 
> It's a bit odd that clock_get and latency_get return different types... and
> do we care about rounding vs truncation here?

I changed this to return a rounded UINT64, it's indeed better to be explicit about it. Although it does not matter much at this stage, since worst case, the clock is off by one audio frame.

Comment 13

[Paul Adenot (:padenot)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a53ee0cc3ae

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out for Windows debug crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c59ac44efce

https://treeherder.mozilla.org/logviewer.html#?job_id=7663637&repo=mozilla-inbound

Comment 15

[Paul Adenot (:padenot)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=08b1de471e7e

Comment 16

[Paul Adenot (:padenot)]

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

Comment 17

[Paul Adenot (:padenot)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=38e7d5f720a1

Comment 18

[Maire Reavy [:mreavy]]

Talked with Anthony and Paul about this bug and how far we should uplift this.  I'm commenting here to capture what we discussed and verify that we're all aware of the plan and on board. 

I'd/we'd like to focus on getting it landed in Nightly (Fx39) and target uplifting it to Aurora (Fx38), not Beta(Fx37).  If anyone has concerns or objections, please speak up -- else let's mark this tracking-firefox38.  Thanks.

Comment 19

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

This bug was discussed in the MSE meeting today and everyone agreed that it wasn't worth risking uplift to 37.

Comment 20

[Paul Adenot (:padenot)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ff5489607cb

Comment 21

[Maire Reavy [:mreavy]]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #19)
> This bug was discussed in the MSE meeting today and everyone agreed that it
> wasn't worth risking uplift to 37.

Thanks for following up, Anthony.  I'll mark this for tracking-firefox38 and won't fix for fx37.

Comment 22

[Maire Reavy [:mreavy]]

[Tracking Requested - why for this release]:

Audio is delayed when using a bluetooth headset. It is a regression. We've decided not to risk uplift to Fx37 (see previous comments in this bug), but we'd like to get a fix out sooner than 12-18 weeks.  So once we have a fix in Nightly (Fx39), we'll ask for uplift to Aurora (Fx38) only.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 38 and 39 since this is a regression.

Comment 24

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Paul - what needs to happen to get this landed?

Comment 25

[Paul Adenot (:padenot)]

I found an orange. I've got a fix, but I want to make sure I'm not papering over an issue I don't know about, so it's going through try.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Paul, if possible, could you land your work? Thanks

Comment 27

[Paul Adenot (:padenot)]

Attachment: Take into account the output device latency in the clock, and be more robust about rounding error accumulation, in cubeb_wasapi.cpp

Matthew, this patch is green, and I tried to explain the subtelty in a comment.
I'm not really happy about this, but I'm not sure to make the clock monotonic
otherwise...

Comment 28

[Matthew Gregan [:kinetik]]

Comment on attachment 8592292 [details] [diff] [review]
Take into account the output device latency in the clock, and be more robust about rounding error accumulation, in cubeb_wasapi.cpp

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

Looks great, two comments below.

::: media/libcubeb/src/cubeb_wasapi.cpp
@@ +542,5 @@
>        {
>          auto_lock lock(stm->stream_reset_lock);
>          close_wasapi_stream(stm);
> +        stm->base_clock = stm->clock;
> +        stm->latency_frames = 0;

Do we need to do this for the reconfigure case in stream_start also?

@@ +1321,5 @@
>  
> +  UINT64 clock = clock_get(stm);
> +  UINT32 latency = latency_get(stm);
> +
> +  *position = max(0, clock - latency);

If latency > clock, isn't the result of type promotion here an unsigned type with an underflow?

Comment 29

[Paul Adenot (:padenot)]

(In reply to Matthew Gregan [:kinetik] from comment #28)
> Comment on attachment 8592292 [details] [diff] [review]
> Take into account the output device latency in the clock, and be more robust
> about rounding error accumulation, in cubeb_wasapi.cpp
> 
> Review of attachment 8592292 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks great, two comments below.
> 
> ::: media/libcubeb/src/cubeb_wasapi.cpp
> @@ +542,5 @@
> >        {
> >          auto_lock lock(stm->stream_reset_lock);
> >          close_wasapi_stream(stm);
> > +        stm->base_clock = stm->clock;
> > +        stm->latency_frames = 0;
> 
> Do we need to do this for the reconfigure case in stream_start also?

Yes I think so.

> @@ +1321,5 @@
> >  
> > +  UINT64 clock = clock_get(stm);
> > +  UINT32 latency = latency_get(stm);
> > +
> > +  *position = max(0, clock - latency);
> 
> If latency > clock, isn't the result of type promotion here an unsigned type
> with an underflow?

Yep, I fixed that.

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3920b67e97a3

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3920b67e97a3

Comment 32

[Sylvestre Ledru [:Sylvestre]]

Paul, is it possible to have the uplift request to aurora & beta? merci!

Comment 33

[Paul Adenot (:padenot)]

Comment on attachment 8592292 [details] [diff] [review]
Take into account the output device latency in the clock, and be more robust about rounding error accumulation, in cubeb_wasapi.cpp

Approval Request Comment
[Feature/regressing bug #]:698079
[User impact if declined]: very slow a/v desynchronization with some audio devices (depending on the sample rate of the media), and a/v sync issues with devices that are high latency, like bluetooth devices
[Describe test coverage new/current, TreeHerder]: no new tests, and this has been pushed and retriggered a lot on try
[Risks and why]: this is just offsetting the clock by some amount. worst case, we have logic in place to reclock. 
[String/UUID change made/needed]: none

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/da661f3aa19c

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fff936b47a9f

Comment 36

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Backout patch

Approval Request Comment
[Feature/regressing bug #]: EME
[User impact if declined]: The patch in this bug caused a regression in A/V sync. I would like approval to back it out from all pre-release branches (we don't need a chemspill, but it needs to ship in 38). If we don't back it out, our audio/video will be out of sync on our EME Launch Partner's site, and YouTube too potentially.
[Describe test coverage new/current, TreeHerder]: Local testing. We don't really have A/V sync unit tests.
[Risks and why]: We'll un-fix the original bug here; some bluetooth headsets will have bad A/V sync, but over-all we'll have better A/V sync in more places.
[String/UUID change made/needed]: None.

Comment 37

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bae54dd3ade1

Comment 38

[Chris Pearce [:cpearce (Not reading bugmail)]]

I have backed out this in order to fix bug 1148299.

Comment 39

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8598325 [details] [diff] [review]
Backout patch

Let's do it.
No need to uplift in beta as we merge m-r into m-b


should be in 38.0 b 9

Comment 40

[Chris Pearce [:cpearce (Not reading bugmail)]]

Backed out of Firefox 38:
https://hg.mozilla.org/releases/mozilla-release/rev/4ea8cdc621e8

Comment 41

[Chris Pearce [:cpearce (Not reading bugmail)]]

Backed out of Firefox 39:
https://hg.mozilla.org/releases/mozilla-aurora/rev/4947d6bca9fa

Comment 42

[Ryan VanderMeulen [:RyanVM]]

(Cleared the flags so this bug doesn't turn up in the bug queries as one needing action still from an uplift standpoint)

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4ea8cdc621e8

Comment 44

[Matthew Gregan [:kinetik]]

Paul, I know you're busy, so if you'd like me to take this over I'm happy to add it to my queue.

Comment 45

[Paul Adenot (:padenot)]

Matthew, that would be super yeah.

A couple things I thought about since discovering those issues:
- Using IAudioClock and the synthetized clock is the recommended method by MS (see https://social.msdn.microsoft.com/Forums/windowsdesktop/en-US/c4fd959b-a0c8-40a0-b037-f949c2428337/audio-inputoutput-synchronization-under-wasapi, the answer from the MS guy). I have no idea why this patch does not work on a Surface Pro, I must have made a mistake somewhere ?
- In any case, the doing the accumulation in a double seems like the right thing to do to prevent a very slow error accumulation when stream and mix sample rates don't match.

Comment 46

[Maire Reavy [:mreavy]]

Thanks, Matthew, for finishing this off.  Paul and I just had our 1:1.  As you say, Paul is super busy, and if other audio backend issues come up, please feel free to take them.  Paul is focused on web audio (both perf improvements and getting the spec in good shape for workers).

Comment 47

[Sylvestre Ledru [:Sylvestre]]

Too late for 38. We could take a super safe patch for 38.0.5.

Comment 48

[Liz Henry (:lizzard) (relman/hg->git project)]

Matthew are you still working on this? 39 beta will only be on beta for 4 weeks, this is just a heads up.

Comment 49

[Matthew Gregan [:kinetik]]

Thanks.  I am, it's on the top of my queue now!

Comment 50

[Liz Henry (:lizzard) (relman/hg->git project)]

Checking in --  we can likely still take a patch for this for 39 beta 4 (going to build early next Monday morning.  If not then we may need to aim for 40.

Comment 51

[Matthew Gregan [:kinetik]]

Unfortunately, I think this'll need to be in 40, the risk/reward of pushing a fix without sufficient testing doesn't work out.

Comment 52

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, thanks kinetik!

Comment 53

[Sylvestre Ledru [:Sylvestre]]

Matthew, have you been able to work on this for 40?
Thanks

Comment 54

[Matthew Gregan [:kinetik]]

Yes, will hopefully have a patch up soon... just trying to track down one remaining issue.

Comment 55

[Lawrence Mandel [:lmandel] (use needinfo)]

We're up to the 40 RC. We're not going to take a fix in this release at this point. I see that we backed out a change for 38 and 39. Do we need to backout again for 40?

Comment 56

[Matthew Gregan [:kinetik]]

No, the original change was already backed out in comment 37.

Comment 57

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Matthew - any closer to an ETA for this bug?

Comment 58

[Matthew Gregan [:kinetik]]

Attachment: bug1136360_v0.patch

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

I have a series of patches in my queue to clean this code up (which I'll attach to a new bug), between the threading model and the mix of audio formats with resampling and device changes, there's a lot of potential for error.

Comment 59

[Matthew Gregan [:kinetik]]

Attachment: bug1136360_drain_v0.patch

(In reply to Matthew Gregan [:kinetik] from comment #58)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=b64a84bf0a2d

test_sanity is failing in the drain test because the computed delay becomes negative and the assert fires.  This shouldn't be possible - we're writing more to the stream than we're recording in frames_written.

Forgot that this relies on another fix in my queue.  Call ReleaseBuffer with the actual number of frames we wrote.  Patch attached, and repushed to try with that included:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=95b50562dd3b

Comment 60

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Paul is back on Monday

Comment 61

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8650874 [details] [diff] [review]
bug1136360_drain_v0.patch

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

LGTM.  You can wait for padenot or not as you prefer.

::: media/libcubeb/src/cubeb_wasapi.cpp
@@ +449,5 @@
>      dest = data;
>    }
>  
>    long out_frames = cubeb_resampler_fill(stm->resampler, dest, frames_needed);
> +  /* XXX: Handle this error. */

Any indication this ever really can happen?  If so, let's at least file a bug for dealing with it.  I don't see any downmix() hits in crashstats so this likely is not worth worrying about

@@ +453,5 @@
> +  /* XXX: Handle this error. */
> +  if (out_frames < 0) {
> +    XASSERT(false);
> +  }
> +  

ws.  Also: why not XASSERT(out_frames >= 0)?

@@ +580,5 @@
>        BYTE * data;
>        hr = stm->render_client->GetBuffer(available, &data);
>        if (SUCCEEDED(hr)) {
> +        long wrote = refill(stm, reinterpret_cast<float *>(data), available);
> +		XASSERT(wrote == available || stm->draining);

indent (tabs?)

Comment 62

[Matthew Gregan [:kinetik]]

Comment on attachment 8650806 [details] [diff] [review]
bug1136360_v0.patch

Happy to land with your review jesup, but this patch needs review too.

Comment 63

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8650806 [details] [diff] [review]
bug1136360_v0.patch

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

Please document which variables are protected by stream_reset_lock (and preferably group them in the definition of cubeb_stream).

Comment 64

[Matthew Gregan [:kinetik]]

Thanks for the reviews!

(In reply to Randell Jesup [:jesup] from comment #61)
> Any indication this ever really can happen?  If so, let's at least file a
> bug for dealing with it.  I don't see any downmix() hits in crashstats so
> this likely is not worth worrying about

No, it'd be a bug if it happened, the XXX comment and assert is there (and in many of the other backends) to catch API misuse (being an external library, there are users outside of Gecko).  It's marked XXX because it'd be better to report the API misuse as an error than fatally fail, but it's also not particularly high priority.

Comment 65

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b568fb8d4477b0095eaf31c93fd53f3d2f4aa45
Bug 1136360 - Account for stream and device latency in stream position calculation.  r=jesup

https://hg.mozilla.org/integration/mozilla-inbound/rev/004f6db9c2dd293009ea3bb934bc399f776bc15d
Bug 1136360 - Report actual number of frames written to ReleaseBuffer.  r=jesup

Comment 66

[Matthew Gregan [:kinetik]]

(In reply to Randell Jesup [:jesup] from comment #63)
> Please document which variables are protected by stream_reset_lock (and
> preferably group them in the definition of cubeb_stream).

I added comments, I have a bunch of cleanup patches that will make the threading and ownership clearer, so I'll leave larger changes for that bug.

Comment 67

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7c52ea2f221d8c082c463a60e2d9a26f72f6902
Backed out 2 changesets (bug 1136360)

Comment 68

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ece893a2e16fdd0b903520d434d1aa454f70e1c8
Bug 1136360 - Account for stream and device latency in stream position calculation.  r=jesup

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7f6748ee57d45b441a9fde36ec9a31da60d6118
Bug 1136360 - Report actual number of frames written to ReleaseBuffer.  r=jesup

Comment 69

[Matthew Gregan [:kinetik]]

Attachment: bug1136360_v1.patch

Comment 70

[Matthew Gregan [:kinetik]]

Attachment: bug1136360_drain_v1.patch

Comment 72

[Matthew Gregan [:kinetik]]

Comment on attachment 8651573 [details] [diff] [review]
bug1136360_v1.patch

Approval Request Comment
[Feature/regressing bug #]: bug 698079
[User impact if declined]: A/V sync issues during media playback
[Describe test coverage new/current, TreeHerder]: as-is
[Risks and why]: low-med; we are using the audio stream clock that we used before bug 698079, but in a new way with a small amount of new code to calculate stream delay and apply it to our own stream clock.
[String/UUID change made/needed]: none

Comment 73

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8651573 [details] [diff] [review]
bug1136360_v1.patch

I do not feel comfortable approving this patch for uplift to Beta for the following reasons:
1. We have been shipping with this bug since FF37 so it is not a release blocker per se.
2. As mentioned in the uplift request the risk is low-medium (given that we are changing the audio stream clock) the impact could be widespread and such fixes should ideally ride trains to weed out any possible fallout before reaching the Beta channel.
3. This code change has just landed on Nightly so we do not have enough data to indicate how stable the fix is yet.

I will let Sylvestre decide whether it can be uplifted to Aurora or not.

Comment 74

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8651574 [details] [diff] [review]
bug1136360_drain_v1.patch

Please see my last comment.

Comment 75

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ece893a2e16f
https://hg.mozilla.org/mozilla-central/rev/e7f6748ee57d

Comment 76

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Ritu - can I ask you to reconsider your treatment of this bug in relation to beta?

This bug is moderately annoying for people who have (often expensive) Bluetooth headsets or other audio paths that have some kind of delay. Bluetooth headsets are unusable for HTML5 video. There have been a number of complaints about this issue. I'd also like to avoid a regression when people switch over from Silverlight to EME. This is a high value fix.

Matthew is generally cautious and has made an effort to minimise risk. There haven't been a lot of changes in this code so if anything does go wrong it should be relatively easy to implicate and back out this change should it cause any issues.

Comment 77

[DeepII]

I just want to point out that his bug does NOT only affect users with bluetooth headsets as stated in this bug, which would limit it to a rather small userbase. As seen on bug 1164966 this also affects people with "normal" hardware. You can also find occasional reports of this bug on reddit and other platforms from people with no special devices but normal configurations.

Comment 78

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #76)
> Ritu - can I ask you to reconsider your treatment of this bug in relation to
> beta?
> 
> This bug is moderately annoying for people who have (often expensive)
> Bluetooth headsets or other audio paths that have some kind of delay.
> Bluetooth headsets are unusable for HTML5 video. There have been a number of
> complaints about this issue. I'd also like to avoid a regression when people
> switch over from Silverlight to EME. This is a high value fix.
> 
> Matthew is generally cautious and has made an effort to minimise risk. There
> haven't been a lot of changes in this code so if anything does go wrong it
> should be relatively easy to implicate and back out this change should it
> cause any issues.

Anthony, thank you for adding the additional context here. I discussed this with Sylvestre and for now we will uplift to Aurora and let it stabilize for a week. If there are no unexpected fall outs, we might consider uplifting to Beta next week.

Comment 79

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8651573 [details] [diff] [review]
bug1136360_v1.patch

Let's uplift this to Aurora to stabilize this fix before considering uplift to Beta.

Comment 80

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8651574 [details] [diff] [review]
bug1136360_drain_v1.patch

Aurora42+

Comment 81

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/a524e8ce1deb
https://hg.mozilla.org/releases/mozilla-aurora/rev/951ac09d3d3a

Comment 82

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8651573 [details] [diff] [review]
bug1136360_v1.patch

Let's uplift to Beta. I have also noticed bug 1164966 on 40 and 41. Hoping it is fixed 41.0b5 on wards.

Comment 83

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8651574 [details] [diff] [review]
bug1136360_drain_v1.patch

Beta41+

Comment 84

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/95ccaf6413b7
https://hg.mozilla.org/releases/mozilla-beta/rev/efdbacb6f2ab