New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 783282 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: 2017-12-11
OS: Mac
Pri: 2
Type: Bug



Sign in to add a comment

A/V Sync problem with AirPlay

Reported by davidsar...@gmail.com, Nov 9 2017

Issue description

UserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36

Example URL:
mlbtv.com

Steps to reproduce the problem:
1. Route Audio on your Mac to an AirPort Express or other AirPlay Audio device (not AppleTV) using the Audio Picker in the Menu Bar (set the Output Device)
2. Got watch a video on mlbtv.com or YouTube

What is the expected behavior?
Audio and Video are synchronized

What went wrong?
Audio is 2 seconds behind the Video.

It looks like the output latency is not being accounted for.

Did this work before? N/A 

Is it a problem with Flash or HTML5? N/A

Does this work in other browsers? Yes

Chrome version: 61.0.3163.100  Channel: n/a
OS Version: OS X 10.13.4
Flash Version: 

Contents of chrome://gpu:
 
This occurs on 10.13
Owner: dalecur...@chromium.org
Status: Assigned (was: Unconfirmed)
Cc: maxmorin@chromium.org olka@chromium.org
For posterity, via e-mail it was pointed out that we're not taking into account the stream latency per this note in the header documentation:

    @constant       kAudioDevicePropertyLatency
                        A UInt32 containing the number of frames of latency in the AudioDevice. Note
                        that input and output latency may differ. Further, the AudioDevice's
                        AudioStreams may have additional latency so they should be queried as well.
                        If both the device and the stream say they have latency, then the total
                        latency for the stream is the device latency summed with the stream latency.

To wit, WebRTCs own internal driver already does this:

https://cs.chromium.org/chromium/src/third_party/webrtc/modules/audio_device/mac/audio_device_mac.cc?l=1267


Project Member

Comment 4 by bugdroid1@chromium.org, Nov 13 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/973509d48434c287fce4510b3a2fe218285a50c4

commit 973509d48434c287fce4510b3a2fe218285a50c4
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Mon Nov 13 21:24:50 2017

Include stream latency for macOS audio latency calculations.

We were incorrectly ignoring the stream latency component when
calculating the fixed latency of streams. This needs to be done
for both input and output.

Since these methods are very similar I've refactored the one our
input and output streams had into a common AudioMangerMac method.

BUG= 783282 
TEST=none

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: Ic583a2f314728b566a5a3e3de8c8ffd118652f91
Reviewed-on: https://chromium-review.googlesource.com/764542
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Olga Sharonova <olka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#516056}
[modify] https://crrev.com/973509d48434c287fce4510b3a2fe218285a50c4/media/audio/mac/audio_auhal_mac.cc
[modify] https://crrev.com/973509d48434c287fce4510b3a2fe218285a50c4/media/audio/mac/audio_auhal_mac.h
[modify] https://crrev.com/973509d48434c287fce4510b3a2fe218285a50c4/media/audio/mac/audio_low_latency_input_mac.cc
[modify] https://crrev.com/973509d48434c287fce4510b3a2fe218285a50c4/media/audio/mac/audio_low_latency_input_mac.h
[modify] https://crrev.com/973509d48434c287fce4510b3a2fe218285a50c4/media/audio/mac/audio_manager_mac.cc
[modify] https://crrev.com/973509d48434c287fce4510b3a2fe218285a50c4/media/audio/mac/audio_manager_mac.h

Status: Fixed (was: Assigned)
Marking Fixed, please ping if it's still an issue.
Still occurs with Chrome Canary v 64.0.3279.0.

Is there a way to log the latency being used by the audio pipeline? Thanks.
Status: Assigned (was: Fixed)
No, but we should probably have one. Will send a CL.
Project Member

Comment 9 by bugdroid1@chromium.org, Nov 29 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/eba82e8dfc3a9f43cb740a408959d23301d158f5

commit eba82e8dfc3a9f43cb740a408959d23301d158f5
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Wed Nov 29 02:15:47 2017

Add trace information for audio output delay data.

Useful in debugging delay issues.

BUG= 783282 
TEST=chrome://tracing has delay information.

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I4973c451efa2236d680bbef334c2a49d073a7c81
Reviewed-on: https://chromium-review.googlesource.com/794972
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#519968}
[modify] https://crrev.com/eba82e8dfc3a9f43cb740a408959d23301d158f5/media/audio/audio_output_device.cc

For posterity, via e-mail the advice from Apple was, "You should just add latency of a single stream and add that to the device latency. In your case, I’d take the stream at index zero.

This will fail when there’s a device with multiple streams each having different latencies - think a break-out box with both a S/PDIF and an analog out. But that’s pretty rare and the code isn’t really architected to handle that case anyway, so taking the zeroth index should get you there 99% of the time"
(This also means the driver in WebRTC code base is incorrectly using device id)
Project Member

Comment 12 by bugdroid1@chromium.org, Dec 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/93e742112a49d6ded6489f87bbf10b337b9b9f00

commit 93e742112a49d6ded6489f87bbf10b337b9b9f00
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Fri Dec 01 18:18:16 2017

Use stream id to get stream latency for first stream.

The previous change 973509d48434c287fce4510b3a2fe218285a50c4, which
added querying of stream latency, incorrectly uses the device id
for querying the stream latency.

We must instead, first query the list of stream ids and query the
latency of a given stream. While there may be multiple streams,
we can't do anything with that information; e.g., to handle that
case we would need to attempt to synchronize all streams to the
highest latency. Apple instead recommends just taking the latency
of the first stream since that will cover the majority of cases.

BUG= 783282 
TEST=none

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I98b65b1fa7030f63abbb6d96c7540352d1bb896b
Reviewed-on: https://chromium-review.googlesource.com/802194
Reviewed-by: Olga Sharonova <olka@chromium.org>
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520988}
[modify] https://crrev.com/93e742112a49d6ded6489f87bbf10b337b9b9f00/media/audio/mac/audio_manager_mac.cc

Labels: M-65
Status: Fixed (was: Assigned)
Fix verified by reporter.
NextAction: 2017-12-11
Will let it soak on the next dev for a week and request merge to m64 after.
The NextAction date has arrived: 2017-12-11
Labels: Merge-Request-64
Project Member

Comment 17 by sheriffbot@chromium.org, Dec 12 2017

Labels: -Merge-Request-64 Hotlist-Merge-Approved Merge-Approved-64
Your change meets the bar and is auto-approved for M64. Please go ahead and merge the CL to branch 3282 manually. Please contact milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 18 by bugdroid1@chromium.org, Dec 12 2017

Labels: -merge-approved-64 merge-merged-3282
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b1a8da88794be9fba341adbe7537ea2e8646f732

commit b1a8da88794be9fba341adbe7537ea2e8646f732
Author: Dale Curtis <dalecurtis@chromium.org>
Date: Tue Dec 12 22:53:26 2017

Merge M64: "Use stream id to get stream latency for first stream."

The previous change 973509d48434c287fce4510b3a2fe218285a50c4, which
added querying of stream latency, incorrectly uses the device id
for querying the stream latency.

We must instead, first query the list of stream ids and query the
latency of a given stream. While there may be multiple streams,
we can't do anything with that information; e.g., to handle that
case we would need to attempt to synchronize all streams to the
highest latency. Apple instead recommends just taking the latency
of the first stream since that will cover the majority of cases.

BUG= 783282 
TEST=none
TBR=dalecurtis@chromium.org

(cherry picked from commit 93e742112a49d6ded6489f87bbf10b337b9b9f00)

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I98b65b1fa7030f63abbb6d96c7540352d1bb896b
Reviewed-on: https://chromium-review.googlesource.com/802194
Reviewed-by: Olga Sharonova <olka@chromium.org>
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#520988}
Reviewed-on: https://chromium-review.googlesource.com/823253
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/branch-heads/3282@{#185}
Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840}
[modify] https://crrev.com/b1a8da88794be9fba341adbe7537ea2e8646f732/media/audio/mac/audio_manager_mac.cc

Sign in to add a comment