New issue
Advanced search Search tips

Issue 598888 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug



Sign in to add a comment

Spitzer: Opus not reported supported in Ogg when unified pipeline enabled

Project Member Reported by ddorwin@chromium.org, Mar 29 2016

Issue description

Spitzer can support both Vorbis and Opus in Ogg because it uses non-platform decoders for audio. The fix for issue 559236 ([1]) was supposed to ensure canPlayType() correctly reported this and other changes.

Code was added to handle this correctly [2]. This relied on including Opus in kFormatCodecMappings on Android, which was originally part of the CL [3]. However, when the CL was rebased on the creation of mime_util_internal, this change was not included [4]. Thus, we will never reach the code in [2].

[1] https://codereview.chromium.org/1690063002/
[2] https://codereview.chromium.org/1690063002/diff/290001/media/base/mime_util_internal.cc#newcode485
[3] https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc#newcode309
[4] https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_internal.cc#newcode86
 
Summary: Spitzer: Opus not reported supported in Ogg when unified pipeline enabled (was: Spitzer: Opus not reported supported in Opus when unified pipeline enabled)
Labels: -M-51 M-50
The MediaCanPlayTypeTest.CodecSupportTest_ogg content browser test would have caught this when we switched the default to the unified media pipeline.
Hmm, that should already be happening now since we enabled variations for the unified media pipeline. Ah... this is content_browsertests and not browser_tests so it might not get the variation.
Yes, media_canplaytype_browsertest.cc still has the old expected results for Android. If I switch kOggOpusProbably and change IsUnifiedMediaPipelineEnabled() to return true always, the tests fail. With my patch, they pass.
Project Member

Comment 6 by bugdroid1@chromium.org, Mar 30 2016

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

commit 65c03640c4f3e32b1ddeca63bc8503beda45d482
Author: ddorwin <ddorwin@chromium.org>
Date: Wed Mar 30 07:14:23 2016

Spitzer: Report Opus in Ogg as supported by unified pipeline

BUG= 598888 
TEST=MediaCanPlayTypeTest.CodecSupportTest_ogg after switching the the default Android media pipleine to the unified pipeline and updating kOggOpusProbably to "probably".

Review URL: https://codereview.chromium.org/1842993002

Cr-Commit-Position: refs/heads/master@{#383919}

[modify] https://crrev.com/65c03640c4f3e32b1ddeca63bc8503beda45d482/media/base/mime_util_internal.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Mar 30 2016

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

commit 65c03640c4f3e32b1ddeca63bc8503beda45d482
Author: ddorwin <ddorwin@chromium.org>
Date: Wed Mar 30 07:14:23 2016

Spitzer: Report Opus in Ogg as supported by unified pipeline

BUG= 598888 
TEST=MediaCanPlayTypeTest.CodecSupportTest_ogg after switching the the default Android media pipleine to the unified pipeline and updating kOggOpusProbably to "probably".

Review URL: https://codereview.chromium.org/1842993002

Cr-Commit-Position: refs/heads/master@{#383919}

[modify] https://crrev.com/65c03640c4f3e32b1ddeca63bc8503beda45d482/media/base/mime_util_internal.cc

Labels: Merge-Request-50
This is a small fix for the unified media pipeline on Android.

The non-Android code remains unchanged: https://codereview.chromium.org/1842993002/diff/1/media/base/mime_util_internal.cc

Comment 9 by tin...@google.com, Mar 30 2016

Labels: -Merge-Request-50 Merge-Approved-50 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M50 (branch: 2661)
Project Member

Comment 10 by bugdroid1@chromium.org, Mar 30 2016

Labels: -merge-approved-50 merge-merged-2661
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5b5408adc903b2f64bb95f5f0688d74ea69bf75f

commit 5b5408adc903b2f64bb95f5f0688d74ea69bf75f
Author: David Dorwin <ddorwin@chromium.org>
Date: Wed Mar 30 23:25:18 2016

Merge M50: "Spitzer: Report Opus in Ogg as supported by unified pipeline"

BUG= 598888 
TEST=MediaCanPlayTypeTest.CodecSupportTest_ogg after switching the the default Android media pipleine to the unified pipeline and updating kOggOpusProbably to "probably".

Review URL: https://codereview.chromium.org/1842993002

Cr-Commit-Position: refs/heads/master@{#383919}
(cherry picked from commit 65c03640c4f3e32b1ddeca63bc8503beda45d482)

Review URL: https://codereview.chromium.org/1846683003 .

Cr-Commit-Position: refs/branch-heads/2661@{#440}
Cr-Branched-From: ef6f6ae5e4c96622286b563658d5cd62a6cf1197-refs/heads/master@{#378081}

[modify] https://crrev.com/5b5408adc903b2f64bb95f5f0688d74ea69bf75f/media/base/mime_util_internal.cc

Status: Fixed (was: Started)
Fixed in M50 and later.

Sign in to add a comment