Status

()

defect
P2
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ajones, Assigned: kinetik)

Tracking

(Blocks 1 bug)

Trunk
mozilla43
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox37 wontfix, firefox38+ wontfix, firefox39+ wontfix, firefox38.0.5 wontfix, firefox40+ wontfix, firefox41+ fixed, firefox42 fixed, firefox43 fixed)

Details

()

Attachments

(2 attachments, 5 obsolete attachments)

STR:

* Navigate to URL
* Watch AV sync flashes

Expected results:

Audio and video are in sync

Actual results:

Audio is delayed
Required bluetooth headset
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
Assignee: nobody → 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?
Flags: needinfo?(kinetik)
Assignee

Comment 4

4 years ago
Sounds good!  Thanks.
Flags: needinfo?(kinetik)

Updated

4 years ago
Priority: -- → P2
(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?
Would offsetting the latency be a faster path to "good enough"?
(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.
Posted patch patch (obsolete) — Splinter Review
This works, but I want to get/set the latency and clock together I think.
(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 on attachment 8573364 [details] [diff] [review]
patch

yeah in fact putting those two together is not really better.
Attachment #8573364 - Flags: review?(kinetik)
Assignee

Updated

4 years ago
Attachment #8573364 - Attachment is patch: true
Assignee

Comment 11

4 years ago
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?
Attachment #8573364 - Flags: review?(kinetik) → review+
(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.
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.
This bug was discussed in the MSE meeting today and everyone agreed that it wasn't worth risking uplift to 37.
(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.
[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.
Tracking for 38 and 39 since this is a regression.
Paul - what needs to happen to get this landed?
Flags: needinfo?(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.
Flags: needinfo?(padenot)
Paul, if possible, could you land your work? Thanks
Flags: needinfo?(padenot)
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...
Attachment #8592292 - Flags: review?(kinetik)
Assignee

Comment 28

4 years ago
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?
Attachment #8592292 - Flags: review?(kinetik) → review+
(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.
Flags: needinfo?(padenot)
https://hg.mozilla.org/mozilla-central/rev/3920b67e97a3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Paul, is it possible to have the uplift request to aurora & beta? merci!
Flags: needinfo?(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
Flags: needinfo?(padenot)
Attachment #8592292 - Flags: approval-mozilla-beta?
Attachment #8592292 - Flags: approval-mozilla-aurora?
Attachment #8592292 - Flags: approval-mozilla-beta?
Attachment #8592292 - Flags: approval-mozilla-beta+
Attachment #8592292 - Flags: approval-mozilla-aurora?
Attachment #8592292 - Flags: approval-mozilla-aurora+
Posted patch Backout patch (obsolete) — Splinter Review
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.
Attachment #8598325 - Flags: review+
Attachment #8598325 - Flags: approval-mozilla-release?
Attachment #8598325 - Flags: approval-mozilla-beta?
Attachment #8598325 - Flags: approval-mozilla-aurora?
I have backed out this in order to fix bug 1148299.
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Assignee

Updated

4 years ago
Depends on: 1148299
Assignee

Updated

4 years ago
Depends on: 1157886
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
Attachment #8598325 - Flags: approval-mozilla-release?
Attachment #8598325 - Flags: approval-mozilla-release+
Attachment #8598325 - Flags: approval-mozilla-beta?
Attachment #8598325 - Flags: approval-mozilla-aurora?
Attachment #8598325 - Flags: approval-mozilla-aurora+
Target Milestone: mozilla40 → ---
Attachment #8592292 - Flags: approval-mozilla-beta+
Attachment #8592292 - Flags: approval-mozilla-aurora+
Attachment #8598325 - Flags: approval-mozilla-release+
Attachment #8598325 - Flags: approval-mozilla-aurora+
(Cleared the flags so this bug doesn't turn up in the bug queries as one needing action still from an uplift standpoint)
Assignee

Comment 44

4 years ago
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.
Flags: needinfo?(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.
Flags: needinfo?(padenot)
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).
Assignee: padenot → kinetik
Too late for 38. We could take a super safe patch for 38.0.5.
Assignee

Updated

4 years ago
Attachment #8573364 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Keywords: leave-open
Matthew are you still working on this? 39 beta will only be on beta for 4 weeks, this is just a heads up.
Flags: needinfo?(kinetik)
Assignee

Comment 49

4 years ago
Thanks.  I am, it's on the top of my queue now!
Flags: needinfo?(kinetik)
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.
Flags: needinfo?(kinetik)
Assignee

Comment 51

4 years ago
Unfortunately, I think this'll need to be in 40, the risk/reward of pushing a fix without sufficient testing doesn't work out.
Flags: needinfo?(kinetik)
Matthew, have you been able to work on this for 40?
Thanks
Flags: needinfo?(kinetik)
Assignee

Comment 54

4 years ago
Yes, will hopefully have a patch up soon... just trying to track down one remaining issue.
Flags: needinfo?(kinetik)
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?
Flags: needinfo?(kinetik)
Assignee

Comment 56

4 years ago
No, the original change was already backed out in comment 37.
Flags: needinfo?(kinetik)
Matthew - any closer to an ETA for this bug?
Flags: needinfo?(kinetik)
Assignee

Comment 58

4 years ago
Posted patch bug1136360_v0.patch (obsolete) — Splinter Review
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.
Attachment #8592292 - Attachment is obsolete: true
Attachment #8598325 - Attachment is obsolete: true
Flags: needinfo?(kinetik)
Attachment #8650806 - Flags: review?(padenot)
Assignee

Comment 59

4 years ago
Posted patch bug1136360_drain_v0.patch (obsolete) — Splinter Review
(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
Attachment #8650874 - Flags: review?(padenot)
Paul is back on Monday
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?)
Attachment #8650874 - Flags: review+
Assignee

Comment 62

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

Happy to land with your review jesup, but this patch needs review too.
Attachment #8650806 - Flags: review?(rjesup)
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).
Attachment #8650806 - Flags: review?(rjesup) → review+
Assignee

Updated

4 years ago
Attachment #8650806 - Flags: review?(padenot)
Assignee

Updated

4 years ago
Attachment #8650874 - Flags: review?(padenot)
Assignee

Comment 64

4 years ago
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.
Assignee

Comment 65

4 years ago
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
Assignee

Comment 66

4 years ago
(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.
Assignee

Comment 68

4 years ago
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
Assignee

Comment 69

4 years ago
Attachment #8650806 - Attachment is obsolete: true
Attachment #8651573 - Flags: review+
Assignee

Comment 70

4 years ago
Attachment #8650874 - Attachment is obsolete: true
Attachment #8651574 - Flags: review+
Assignee

Updated

4 years ago
Duplicate of this bug: 1164966
Assignee

Comment 72

4 years ago
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
Attachment #8651573 - Flags: approval-mozilla-beta?
Attachment #8651573 - Flags: approval-mozilla-aurora?
Assignee

Updated

4 years ago
Attachment #8651574 - Flags: approval-mozilla-beta?
Attachment #8651574 - Flags: approval-mozilla-aurora?
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.
Attachment #8651573 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment on attachment 8651574 [details] [diff] [review]
bug1136360_drain_v1.patch

Please see my last comment.
Attachment #8651574 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
https://hg.mozilla.org/mozilla-central/rev/ece893a2e16f
https://hg.mozilla.org/mozilla-central/rev/e7f6748ee57d
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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.
Flags: needinfo?(rkothari)

Comment 77

4 years ago
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.
(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.
Flags: needinfo?(rkothari)
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.
Attachment #8651573 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8651574 [details] [diff] [review]
bug1136360_drain_v1.patch

Aurora42+
Attachment #8651574 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8651573 - Flags: approval-mozilla-beta- → approval-mozilla-beta+
Comment on attachment 8651574 [details] [diff] [review]
bug1136360_drain_v1.patch

Beta41+
Attachment #8651574 - Flags: approval-mozilla-beta- → approval-mozilla-beta+
Assignee

Updated

4 years ago
Summary: AV sync late on Windows Bluetooth → AV sync late on Windows
Depends on: 1199794
Duplicate of this bug: 1113228
Duplicate of this bug: 1179957
You need to log in before you can comment on or make changes to this bug.