Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
866573004:
media: Enable Opus support in Clank <video> and MSE (Closed)
|
Created:
5 years, 11 months ago by vignesh Modified:
5 years, 10 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, cbentzel+watch_chromium.org, posciak+watch_chromium.org, avayvod+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, wjia+watch_chromium.org, fgalligan1, DaleCurtis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmedia: Enable Opus support in Clank <video> and MSE
Opus audio codec is supported by the android platform starting from Lollipop.
This CL enables canPlayType() support for Opus on Clank and MSE playback of Opus
in Clank. This brings Opus feature parity with Desktop Chromium.
BUG=318436
Committed: https://crrev.com/95fcf214b5a9ba718d53efbf509c9d92e2514e4f
Cr-Commit-Position: refs/heads/master@{#313549}
Patch Set 1 #
Total comments: 10
Patch Set 2 : nit fixes #
Total comments: 8
Patch Set 3 : s/0x00/0/ #
Total comments: 2
Patch Set 4 : addressing comments #Patch Set 5 : rebase #Messages
Total messages: 31 (7 generated)
vigneshv@chromium.org changed reviewers: + dalecurtis@chromium.org, tomfinegan@chromium.org, wolenetz@chromium.org
dalecurtis@chromium.org changed reviewers: - dalecurtis@chromium.org
This looks good to me, I'll let wolenetz@ give final approval though.
Also looks good to me, % nits, but I'll let a media owner actually do the approving. https://codereview.chromium.org/866573004/diff/1/content/renderer/media/andro... File content/renderer/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/866573004/diff/1/content/renderer/media/andro... content/renderer/media/android/media_source_delegate.cc:740: config.codec_delay() * (1000000000.0 / config.samples_per_second())); Time::kNanosecondsPerSecond ? https://codereview.chromium.org/866573004/diff/1/content/renderer/media/andro... content/renderer/media/android/media_source_delegate.cc:742: config.seek_preroll().InMicroseconds() * 1000; Time::kNanosecondsPerMicrosecond ? https://codereview.chromium.org/866573004/diff/1/media/base/android/media_cod... File media/base/android/media_codec_bridge.cc (right): https://codereview.chromium.org/866573004/diff/1/media/base/android/media_cod... media/base/android/media_codec_bridge.cc:661: env, extra_data, extra_data_size); I'd wrap after the =, assuming it fits.. And even if it doesn't fit, that seems to be the prevailing local style. /pedantic https://codereview.chromium.org/866573004/diff/1/media/base/android/media_cod... File media/base/android/media_codec_bridge_unittest.cc (right): https://codereview.chromium.org/866573004/diff/1/media/base/android/media_cod... media/base/android/media_codec_bridge_unittest.cc:259: uint8 extra_data[] = { 0x00, 0x00 }; 0, 0 ?
https://codereview.chromium.org/866573004/diff/1/content/renderer/media/andro... File content/renderer/media/android/media_source_delegate.cc (right): https://codereview.chromium.org/866573004/diff/1/content/renderer/media/andro... content/renderer/media/android/media_source_delegate.cc:740: config.codec_delay() * (1000000000.0 / config.samples_per_second())); On 2015/01/22 02:35:14, Tom Finegan wrote: > Time::kNanosecondsPerSecond ? Done. https://codereview.chromium.org/866573004/diff/1/content/renderer/media/andro... content/renderer/media/android/media_source_delegate.cc:742: config.seek_preroll().InMicroseconds() * 1000; On 2015/01/22 02:35:14, Tom Finegan wrote: > Time::kNanosecondsPerMicrosecond ? Done. https://codereview.chromium.org/866573004/diff/1/media/base/android/media_cod... File media/base/android/media_codec_bridge.cc (right): https://codereview.chromium.org/866573004/diff/1/media/base/android/media_cod... media/base/android/media_codec_bridge.cc:661: env, extra_data, extra_data_size); On 2015/01/22 02:35:14, Tom Finegan wrote: > I'd wrap after the =, assuming it fits.. And even if it doesn't fit, that seems > to be the prevailing local style. > > /pedantic Done. https://codereview.chromium.org/866573004/diff/1/media/base/android/media_cod... File media/base/android/media_codec_bridge_unittest.cc (right): https://codereview.chromium.org/866573004/diff/1/media/base/android/media_cod... media/base/android/media_codec_bridge_unittest.cc:259: uint8 extra_data[] = { 0x00, 0x00 }; On 2015/01/22 02:35:14, Tom Finegan wrote: > 0, 0 ? it doesn't matter what this data is. we just need something. renamed it dummy_extra_data to be clear.
https://codereview.chromium.org/866573004/diff/1/media/base/android/media_cod... File media/base/android/media_codec_bridge_unittest.cc (right): https://codereview.chromium.org/866573004/diff/1/media/base/android/media_cod... media/base/android/media_codec_bridge_unittest.cc:259: uint8 extra_data[] = { 0x00, 0x00 }; On 2015/01/22 21:29:50, vignesh wrote: > On 2015/01/22 02:35:14, Tom Finegan wrote: > > 0, 0 ? > > it doesn't matter what this data is. we just need something. renamed it > dummy_extra_data to be clear. I was just pointing out that 0,0 is perfectly adequate for expressing the values. :)
https://codereview.chromium.org/866573004/diff/1/media/base/android/media_cod... File media/base/android/media_codec_bridge_unittest.cc (right): https://codereview.chromium.org/866573004/diff/1/media/base/android/media_cod... media/base/android/media_codec_bridge_unittest.cc:259: uint8 extra_data[] = { 0x00, 0x00 }; On 2015/01/22 21:32:05, Tom Finegan wrote: > On 2015/01/22 21:29:50, vignesh wrote: > > On 2015/01/22 02:35:14, Tom Finegan wrote: > > > 0, 0 ? > > > > it doesn't matter what this data is. we just need something. renamed it > > dummy_extra_data to be clear. > > I was just pointing out that 0,0 is perfectly adequate for expressing the > values. :) ah alright, Done.
Thanks for doing this. This is looking pretty good. Beyond my comments, you'll also need: * net/base OWNER (maybe rsleevi@?) * content/common/media OWNER (dalecurtis@?) * media/base/android/ preferred reviewer (qinmin@) https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... File media/base/android/media_codec_bridge.cc (right): https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... media/base/android/media_codec_bridge.cc:562: if (extra_data_size == 0) Might it ever be valid to configure opus with 0 extradata, but some non-negative codec delay or seek preroll? If so, it appears this code doesn't use codec delay or seek preroll. https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... media/base/android/media_codec_bridge.cc:654: if (codec_delay_ns < 0 || seek_preroll_ns < 0) { nit: default DemuxerConfig initializer values for these to be negative, to ensure that actual values are set before usage, too? https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... media/base/android/media_codec_bridge.cc:676: Java_MediaCodecBridge_setCodecSpecificData(env, j_format, 2, csd1.obj()); hmm. this looks wrong. should it instead be sending csd2.obj? Also, please add some test to capture things like this. https://codereview.chromium.org/866573004/diff/40001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java (right): https://codereview.chromium.org/866573004/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:660: name = "csd-2"; nit: where are these csd-n documented in Android MediaFormat.setByteBuffer()'s usage docs? Include a reference comment please.
On 2015/01/22 22:06:23, wolenetz wrote: > Thanks for doing this. This is looking pretty good. > Beyond my comments, you'll also need: > * net/base OWNER (maybe rsleevi@?) > * content/common/media OWNER (dalecurtis@?) since this CL changes *_messages_*.h in content/common/media, it needs one of the security people to review it. i'll add someone from there. > * media/base/android/ preferred reviewer (qinmin@) i was planning on adding those people once i get a basic okay from you. > > https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... > File media/base/android/media_codec_bridge.cc (right): > > https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... > media/base/android/media_codec_bridge.cc:562: if (extra_data_size == 0) > Might it ever be valid to configure opus with 0 extradata, but some non-negative > codec delay or seek preroll? If so, it appears this code doesn't use codec delay > or seek preroll. > > https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... > media/base/android/media_codec_bridge.cc:654: if (codec_delay_ns < 0 || > seek_preroll_ns < 0) { > nit: default DemuxerConfig initializer values for these to be negative, to > ensure that actual values are set before usage, too? > > https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... > media/base/android/media_codec_bridge.cc:676: > Java_MediaCodecBridge_setCodecSpecificData(env, j_format, 2, csd1.obj()); > hmm. this looks wrong. should it instead be sending csd2.obj? > Also, please add some test to capture things like this. > > https://codereview.chromium.org/866573004/diff/40001/media/base/android/java/... > File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java > (right): > > https://codereview.chromium.org/866573004/diff/40001/media/base/android/java/... > media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:660: name = > "csd-2"; > nit: where are these csd-n documented in Android MediaFormat.setByteBuffer()'s > usage docs? Include a reference comment please.
vigneshv@chromium.org changed reviewers: + jschuh@chromium.org, qinmin@chromium.org, rsleevi@chromium.org
Adding rsleevi@ for net/base OWNER Adding jschuh@ for content/common/media security review of IPC message file. Adding qinmin@ for media/base/android OWNER https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... File media/base/android/media_codec_bridge.cc (right): https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... media/base/android/media_codec_bridge.cc:562: if (extra_data_size == 0) On 2015/01/22 22:06:22, wolenetz wrote: > Might it ever be valid to configure opus with 0 extradata, but some non-negative > codec delay or seek preroll? If so, it appears this code doesn't use codec delay > or seek preroll. Opus always need extra data from the headers. I have changed this code to handle that and added a test as well. https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... media/base/android/media_codec_bridge.cc:654: if (codec_delay_ns < 0 || seek_preroll_ns < 0) { On 2015/01/22 22:06:22, wolenetz wrote: > nit: default DemuxerConfig initializer values for these to be negative, to > ensure that actual values are set before usage, too? Done. https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... media/base/android/media_codec_bridge.cc:676: Java_MediaCodecBridge_setCodecSpecificData(env, j_format, 2, csd1.obj()); On 2015/01/22 22:06:22, wolenetz wrote: > hmm. this looks wrong. should it instead be sending csd2.obj? > Also, please add some test to capture things like this. Whoops. Any idea how i can add a test for this? Since this is being set on the java side, seems like i'd have to add a method on the java file for the sake of this test. https://codereview.chromium.org/866573004/diff/40001/media/base/android/java/... File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java (right): https://codereview.chromium.org/866573004/diff/40001/media/base/android/java/... media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:660: name = "csd-2"; On 2015/01/22 22:06:22, wolenetz wrote: > nit: where are these csd-n documented in Android MediaFormat.setByteBuffer()'s > usage docs? Include a reference comment please. Done.
https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... File media/base/android/media_codec_bridge.cc (right): https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... media/base/android/media_codec_bridge.cc:562: if (extra_data_size == 0) On 2015/01/22 22:48:42, vignesh wrote: > On 2015/01/22 22:06:22, wolenetz wrote: > > Might it ever be valid to configure opus with 0 extradata, but some > non-negative > > codec delay or seek preroll? If so, it appears this code doesn't use codec > delay > > or seek preroll. > > Opus always need extra data from the headers. I have changed this code to handle > that and added a test as well. Acknowledged. https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... media/base/android/media_codec_bridge.cc:676: Java_MediaCodecBridge_setCodecSpecificData(env, j_format, 2, csd1.obj()); On 2015/01/22 22:48:42, vignesh wrote: > On 2015/01/22 22:06:22, wolenetz wrote: > > hmm. this looks wrong. should it instead be sending csd2.obj? > > Also, please add some test to capture things like this. > > Whoops. > > Any idea how i can add a test for this? Since this is being set on the java > side, seems like i'd have to add a method on the java file for the sake of this > test. I guess you could add a mediasourceplayertest that feeds various opus headers followed by opus blocks and checks the resulting clock to see how much time advanced due to audio beyond the delay or preroll. qinmin@: can you help clarify possibilities for testing this please?
ipc security lgtm (notes: integer time values)
lgtm
On 2015/01/22 23:01:06, wolenetz wrote: > https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... > File media/base/android/media_codec_bridge.cc (right): > > https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... > media/base/android/media_codec_bridge.cc:562: if (extra_data_size == 0) > On 2015/01/22 22:48:42, vignesh wrote: > > On 2015/01/22 22:06:22, wolenetz wrote: > > > Might it ever be valid to configure opus with 0 extradata, but some > > non-negative > > > codec delay or seek preroll? If so, it appears this code doesn't use codec > > delay > > > or seek preroll. > > > > Opus always need extra data from the headers. I have changed this code to > handle > > that and added a test as well. > > Acknowledged. > > https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... > media/base/android/media_codec_bridge.cc:676: > Java_MediaCodecBridge_setCodecSpecificData(env, j_format, 2, csd1.obj()); > On 2015/01/22 22:48:42, vignesh wrote: > > On 2015/01/22 22:06:22, wolenetz wrote: > > > hmm. this looks wrong. should it instead be sending csd2.obj? > > > Also, please add some test to capture things like this. > > > > Whoops. > > > > Any idea how i can add a test for this? Since this is being set on the java > > side, seems like i'd have to add a method on the java file for the sake of > this > > test. > > I guess you could add a mediasourceplayertest that feeds various opus headers > followed by opus blocks and checks the resulting clock to see how much time > advanced due to audio beyond the delay or preroll. we had a similar test on desktop chrome before (based on number of samples decoded) and it turned out to be too fragile. even here, the exact time the clock moves depends on how the android platform handles these values (it's up to the platform to use these - but it expects them to be there the very least). so i am not sure if such a test would be a good idea. i was thinking more along the lines of a unit test that makes sure we pass only the intended values to the android platform. thoughts? > qinmin@: can you help clarify possibilities for testing this please?
On 2015/01/26 23:36:37, vignesh wrote: > On 2015/01/22 23:01:06, wolenetz wrote: > > > https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... > > File media/base/android/media_codec_bridge.cc (right): > > > > > https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... > > media/base/android/media_codec_bridge.cc:562: if (extra_data_size == 0) > > On 2015/01/22 22:48:42, vignesh wrote: > > > On 2015/01/22 22:06:22, wolenetz wrote: > > > > Might it ever be valid to configure opus with 0 extradata, but some > > > non-negative > > > > codec delay or seek preroll? If so, it appears this code doesn't use codec > > > delay > > > > or seek preroll. > > > > > > Opus always need extra data from the headers. I have changed this code to > > handle > > > that and added a test as well. > > > > Acknowledged. > > > > > https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... > > media/base/android/media_codec_bridge.cc:676: > > Java_MediaCodecBridge_setCodecSpecificData(env, j_format, 2, csd1.obj()); > > On 2015/01/22 22:48:42, vignesh wrote: > > > On 2015/01/22 22:06:22, wolenetz wrote: > > > > hmm. this looks wrong. should it instead be sending csd2.obj? > > > > Also, please add some test to capture things like this. > > > > > > Whoops. > > > > > > Any idea how i can add a test for this? Since this is being set on the java > > > side, seems like i'd have to add a method on the java file for the sake of > > this > > > test. > > > > I guess you could add a mediasourceplayertest that feeds various opus headers > > followed by opus blocks and checks the resulting clock to see how much time > > advanced due to audio beyond the delay or preroll. > > we had a similar test on desktop chrome before (based on number of samples > decoded) and it turned out to be too fragile. even here, the exact time the > clock moves depends on how the android platform handles these values (it's up to > the platform to use these - but it expects them to be there the very least). so > i am not sure if such a test would be a good idea. > > i was thinking more along the lines of a unit test that makes sure we pass only > the intended values to the android platform. thoughts? > > > qinmin@: can you help clarify possibilities for testing this please? Good point about exact timings varying across devices. Ideally, CTS would enforce consistency there. Is there a plan for that? Also, are you certain similar flakiness as observed on desktop would show up in MSPTest? For this CL, I was hoping that a test would help verify assumptions about what exactly csd-* are used for in the Opus case. From the link (http://developer.android.com/reference/android/media/MediaCodec.html), I am not certain how MediaCodec interprets csd-1 vs csv-2 vs .. for Opus. If there is some middle ground (such as a more specific documentation link for how csd-* are used for Opus + the test you're proposing), I'd be more comfortable about not including the specific timing test that I was proposing. WDYT?
On 2015/01/27 00:18:28, wolenetz wrote: > On 2015/01/26 23:36:37, vignesh wrote: > > On 2015/01/22 23:01:06, wolenetz wrote: > > > > > > https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... > > > File media/base/android/media_codec_bridge.cc (right): > > > > > > > > > https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... > > > media/base/android/media_codec_bridge.cc:562: if (extra_data_size == 0) > > > On 2015/01/22 22:48:42, vignesh wrote: > > > > On 2015/01/22 22:06:22, wolenetz wrote: > > > > > Might it ever be valid to configure opus with 0 extradata, but some > > > > non-negative > > > > > codec delay or seek preroll? If so, it appears this code doesn't use > codec > > > > delay > > > > > or seek preroll. > > > > > > > > Opus always need extra data from the headers. I have changed this code to > > > handle > > > > that and added a test as well. > > > > > > Acknowledged. > > > > > > > > > https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... > > > media/base/android/media_codec_bridge.cc:676: > > > Java_MediaCodecBridge_setCodecSpecificData(env, j_format, 2, csd1.obj()); > > > On 2015/01/22 22:48:42, vignesh wrote: > > > > On 2015/01/22 22:06:22, wolenetz wrote: > > > > > hmm. this looks wrong. should it instead be sending csd2.obj? > > > > > Also, please add some test to capture things like this. > > > > > > > > Whoops. > > > > > > > > Any idea how i can add a test for this? Since this is being set on the > java > > > > side, seems like i'd have to add a method on the java file for the sake of > > > this > > > > test. > > > > > > I guess you could add a mediasourceplayertest that feeds various opus > headers > > > followed by opus blocks and checks the resulting clock to see how much time > > > advanced due to audio beyond the delay or preroll. > > > > we had a similar test on desktop chrome before (based on number of samples > > decoded) and it turned out to be too fragile. even here, the exact time the > > clock moves depends on how the android platform handles these values (it's up > to > > the platform to use these - but it expects them to be there the very least). > so > > i am not sure if such a test would be a good idea. > > > > i was thinking more along the lines of a unit test that makes sure we pass > only > > the intended values to the android platform. thoughts? > > > > > qinmin@: can you help clarify possibilities for testing this please? > > Good point about exact timings varying across devices. Ideally, CTS would > enforce consistency there. Is there a plan for that? Also, are you certain > similar flakiness as observed on desktop would show up in MSPTest? I'm pretty sure we will encounter flakyness. Also, based on a quick check [1], i don't think the android platform has any CTS tests at all for Opus (it is a WIP i would presume). > For this CL, I was hoping that a test would help verify assumptions about what > exactly csd-* are used for in the Opus case. From the link > (http://developer.android.com/reference/android/media/MediaCodec.html). Ideally, these should be populated by the platform's demuxer (android.media.MediaExtractor). But for MSE, since we use our own demuxer, we are having to fill this up similar to how MediaExtractor would fill it up. > I am not certain how MediaCodec interprets csd-1 vs csv-2 vs .. for Opus. I don't think there is any documentation about what csd-0,1,etc means in the case of Opus. I know it (as i added that to the android platform as well). The only reference i have is to the source code of Opus decoder in android platform [2]. We do similar stuff for Vorbis as well (see the switch case in media_codec_bridge). And i can't seem to find any MediaCodec documentation on what csd-* means for that either. So, if we have some tests for Vorbis, we can try and replicate that for Opus. > If there is > some middle ground (such as a more specific documentation link for how csd-* are > used for Opus + the test you're proposing), I'd be more comfortable about not > including the specific timing test that I was proposing. > WDYT? My stand is that, we should just make sure that we are setting the right values and leave it up to the platform to ensure it uses them as intended. [1] http://androidxref.com/5.0.0_r2/xref/cts/tests/tests/media/src/android/media/... [2] http://androidxref.com/5.0.0_r2/xref/frameworks/av/media/libstagefright/codec...
On 2015/01/27 00:33:26, vignesh wrote: > On 2015/01/27 00:18:28, wolenetz wrote: > > On 2015/01/26 23:36:37, vignesh wrote: > > > On 2015/01/22 23:01:06, wolenetz wrote: > > > > > > > > > > https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... > > > > File media/base/android/media_codec_bridge.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... > > > > media/base/android/media_codec_bridge.cc:562: if (extra_data_size == 0) > > > > On 2015/01/22 22:48:42, vignesh wrote: > > > > > On 2015/01/22 22:06:22, wolenetz wrote: > > > > > > Might it ever be valid to configure opus with 0 extradata, but some > > > > > non-negative > > > > > > codec delay or seek preroll? If so, it appears this code doesn't use > > codec > > > > > delay > > > > > > or seek preroll. > > > > > > > > > > Opus always need extra data from the headers. I have changed this code > to > > > > handle > > > > > that and added a test as well. > > > > > > > > Acknowledged. > > > > > > > > > > > > > > https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... > > > > media/base/android/media_codec_bridge.cc:676: > > > > Java_MediaCodecBridge_setCodecSpecificData(env, j_format, 2, csd1.obj()); > > > > On 2015/01/22 22:48:42, vignesh wrote: > > > > > On 2015/01/22 22:06:22, wolenetz wrote: > > > > > > hmm. this looks wrong. should it instead be sending csd2.obj? > > > > > > Also, please add some test to capture things like this. > > > > > > > > > > Whoops. > > > > > > > > > > Any idea how i can add a test for this? Since this is being set on the > > java > > > > > side, seems like i'd have to add a method on the java file for the sake > of > > > > this > > > > > test. > > > > > > > > I guess you could add a mediasourceplayertest that feeds various opus > > headers > > > > followed by opus blocks and checks the resulting clock to see how much > time > > > > advanced due to audio beyond the delay or preroll. > > > > > > we had a similar test on desktop chrome before (based on number of samples > > > decoded) and it turned out to be too fragile. even here, the exact time the > > > clock moves depends on how the android platform handles these values (it's > up > > to > > > the platform to use these - but it expects them to be there the very least). > > so > > > i am not sure if such a test would be a good idea. > > > > > > i was thinking more along the lines of a unit test that makes sure we pass > > only > > > the intended values to the android platform. thoughts? > > > > > > > qinmin@: can you help clarify possibilities for testing this please? > > > > Good point about exact timings varying across devices. Ideally, CTS would > > enforce consistency there. Is there a plan for that? Also, are you certain > > similar flakiness as observed on desktop would show up in MSPTest? > > I'm pretty sure we will encounter flakyness. Also, based on a quick check [1], i > don't think the android platform has any CTS tests at all for Opus (it is a WIP > i would presume). > > > For this CL, I was hoping that a test would help verify assumptions about what > > exactly csd-* are used for in the Opus case. From the link > > (http://developer.android.com/reference/android/media/MediaCodec.html). > > Ideally, these should be populated by the platform's demuxer > (android.media.MediaExtractor). But for MSE, since we use our own demuxer, we > are having to fill this up similar to how MediaExtractor would fill it up. > > > I am not certain how MediaCodec interprets csd-1 vs csv-2 vs .. for Opus. > > I don't think there is any documentation about what csd-0,1,etc means in the > case of Opus. I know it (as i added that to the android platform as well). The > only reference i have is to the source code of Opus decoder in android platform > [2]. We do similar stuff for Vorbis as well (see the switch case in > media_codec_bridge). And i can't seem to find any MediaCodec documentation on > what csd-* means for that either. So, if we have some tests for Vorbis, we can > try and replicate that for Opus. > > > If there is > > some middle ground (such as a more specific documentation link for how csd-* > are > > used for Opus + the test you're proposing), I'd be more comfortable about not > > including the specific timing test that I was proposing. > > WDYT? > > My stand is that, we should just make sure that we are setting the right values > and leave it up to the platform to ensure it uses them as intended. > > > [1] > http://androidxref.com/5.0.0_r2/xref/cts/tests/tests/media/src/android/media/... > [2] > http://androidxref.com/5.0.0_r2/xref/frameworks/av/media/libstagefright/codec... sg and lgtm. Thank you for clarifying. We use AAC and Vorbis throughout many of the MSPTests, and have some very basic "invalid extradata" media_codec_bridge. Seems our coverage was fairly low previously for "what if csd-x" were passed wrong for AAC, for example.
lgtm
On 2015/01/27 01:31:00, wolenetz wrote: > On 2015/01/27 00:33:26, vignesh wrote: > > On 2015/01/27 00:18:28, wolenetz wrote: > > > On 2015/01/26 23:36:37, vignesh wrote: > > > > On 2015/01/22 23:01:06, wolenetz wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... > > > > > File media/base/android/media_codec_bridge.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... > > > > > media/base/android/media_codec_bridge.cc:562: if (extra_data_size == 0) > > > > > On 2015/01/22 22:48:42, vignesh wrote: > > > > > > On 2015/01/22 22:06:22, wolenetz wrote: > > > > > > > Might it ever be valid to configure opus with 0 extradata, but some > > > > > > non-negative > > > > > > > codec delay or seek preroll? If so, it appears this code doesn't use > > > codec > > > > > > delay > > > > > > > or seek preroll. > > > > > > > > > > > > Opus always need extra data from the headers. I have changed this code > > to > > > > > handle > > > > > > that and added a test as well. > > > > > > > > > > Acknowledged. > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/866573004/diff/20001/media/base/android/media... > > > > > media/base/android/media_codec_bridge.cc:676: > > > > > Java_MediaCodecBridge_setCodecSpecificData(env, j_format, 2, > csd1.obj()); > > > > > On 2015/01/22 22:48:42, vignesh wrote: > > > > > > On 2015/01/22 22:06:22, wolenetz wrote: > > > > > > > hmm. this looks wrong. should it instead be sending csd2.obj? > > > > > > > Also, please add some test to capture things like this. > > > > > > > > > > > > Whoops. > > > > > > > > > > > > Any idea how i can add a test for this? Since this is being set on the > > > java > > > > > > side, seems like i'd have to add a method on the java file for the > sake > > of > > > > > this > > > > > > test. > > > > > > > > > > I guess you could add a mediasourceplayertest that feeds various opus > > > headers > > > > > followed by opus blocks and checks the resulting clock to see how much > > time > > > > > advanced due to audio beyond the delay or preroll. > > > > > > > > we had a similar test on desktop chrome before (based on number of samples > > > > decoded) and it turned out to be too fragile. even here, the exact time > the > > > > clock moves depends on how the android platform handles these values (it's > > up > > > to > > > > the platform to use these - but it expects them to be there the very > least). > > > so > > > > i am not sure if such a test would be a good idea. > > > > > > > > i was thinking more along the lines of a unit test that makes sure we pass > > > only > > > > the intended values to the android platform. thoughts? > > > > > > > > > qinmin@: can you help clarify possibilities for testing this please? > > > > > > Good point about exact timings varying across devices. Ideally, CTS would > > > enforce consistency there. Is there a plan for that? Also, are you certain > > > similar flakiness as observed on desktop would show up in MSPTest? > > > > I'm pretty sure we will encounter flakyness. Also, based on a quick check [1], > i > > don't think the android platform has any CTS tests at all for Opus (it is a > WIP > > i would presume). > > > > > For this CL, I was hoping that a test would help verify assumptions about > what > > > exactly csd-* are used for in the Opus case. From the link > > > (http://developer.android.com/reference/android/media/MediaCodec.html). > > > > Ideally, these should be populated by the platform's demuxer > > (android.media.MediaExtractor). But for MSE, since we use our own demuxer, we > > are having to fill this up similar to how MediaExtractor would fill it up. > > > > > I am not certain how MediaCodec interprets csd-1 vs csv-2 vs .. for Opus. > > > > I don't think there is any documentation about what csd-0,1,etc means in the > > case of Opus. I know it (as i added that to the android platform as well). The > > only reference i have is to the source code of Opus decoder in android > platform > > [2]. We do similar stuff for Vorbis as well (see the switch case in > > media_codec_bridge). And i can't seem to find any MediaCodec documentation on > > what csd-* means for that either. So, if we have some tests for Vorbis, we can > > try and replicate that for Opus. > > > > > If there is > > > some middle ground (such as a more specific documentation link for how csd-* > > are > > > used for Opus + the test you're proposing), I'd be more comfortable about > not > > > including the specific timing test that I was proposing. > > > WDYT? > > > > My stand is that, we should just make sure that we are setting the right > values > > and leave it up to the platform to ensure it uses them as intended. > > > > > > [1] > > > http://androidxref.com/5.0.0_r2/xref/cts/tests/tests/media/src/android/media/... > > [2] > > > http://androidxref.com/5.0.0_r2/xref/frameworks/av/media/libstagefright/codec... > > sg and lgtm. Thank you for clarifying. We use AAC and Vorbis throughout many of > the MSPTests, and have some very basic "invalid extradata" media_codec_bridge. > Seems our coverage was fairly low previously for "what if csd-x" were passed > wrong for AAC, for example. Thanks Matt!
The CQ bit was checked by vigneshv@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/866573004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...) ios_rel_device_ninja_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #5 (id:80001) has been deleted
The CQ bit was checked by vigneshv@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/866573004/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/95fcf214b5a9ba718d53efbf509c9d92e2514e4f Cr-Commit-Position: refs/heads/master@{#313549}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:100001) has been created in https://codereview.chromium.org/889053003/ by tedchoc@chromium.org. The reason for reverting is: This is breaking downstream builders on L: C 1029.038s Main [FAIL] MediaCanPlayTypeTest.CodecSupportTest_webm: C 1029.038s Main [ERROR:unix_domain_server_socket_posix.cc(106)] Not implemented reached in virtual int net::UnixDomainServerSocket::GetLocalAddress(net::IPEndPoint*) const C 1029.039s Main [WARNING:proxy_service.cc(898)] PAC support disabled because there is no system implementation C 1029.039s Main ../../content/browser/media/media_canplaytype_browsertest.cc:278: Failure C 1029.039s Main Value of: CanPlay("'video/webm; codecs=\"vp8, opus\"'") C 1029.039s Main Actual: "probably" C 1029.039s Main Expected: kOpusProbably C 1029.039s Main Which is: "" C 1029.039s Main ../../content/browser/media/media_canplaytype_browsertest.cc:279: Failure C 1029.039s Main Value of: CanPlay("'video/webm; codecs=\"vp8.0, opus\"'") C 1029.039s Main Actual: "probably" C 1029.039s Main Expected: kOpusProbably C 1029.039s Main Which is: "" C 1029.039s Main ../../content/browser/media/media_canplaytype_browsertest.cc:285: Failure C 1029.039s Main Value of: CanPlay("'video/webm; codecs=\"vp9, opus\"'") C 1029.039s Main Actual: "probably" C 1029.039s Main Expected: VP9AndOpusProbably C 1029.039s Main Which is: "" C 1029.039s Main ../../content/browser/media/media_canplaytype_browsertest.cc:287: Failure C 1029.039s Main Value of: CanPlay("'video/webm; codecs=\"vp9.0, opus\"'") C 1029.039s Main Actual: "probably" C 1029.040s Main Expected: VP9AndOpusProbably C 1029.040s Main Which is: "" C 1029.040s Main ../../content/browser/media/media_canplaytype_browsertest.cc:296: Failure C 1029.040s Main Value of: CanPlay("'audio/webm; codecs=\"opus\"'") C 1029.040s Main Actual: "probably" C 1029.040s Main Expected: kOpusProbably C 1029.040s Main Which is: "" C 1029.040s Main ../../content/browser/media/media_canplaytype_browsertest.cc:297: Failure C 1029.040s Main Value of: CanPlay("'audio/webm; codecs=\"opus, vorbis\"'") C 1029.040s Main Actual: "probably" C 1029.040s Main Expected: kOpusProbably C 1029.040s Main Which is: "" Spoke with vigneshv@ and reverting for now is the best thing to do while a fix is prepared.. |
