Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1837963004:
Make CanPlayType return "probably" for HI10P h264 videos. (Closed)
|
Created:
4 years, 8 months ago by hubbe Modified:
4 years, 8 months ago CC:
chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake canPlayType return "probably" for HI10P h264 videos.
Note that HI10P does not currently work on android or with EME, and those cases are handled
in the code.
Minor cleanup for EncryptedMediaSupportedTypes* tests so they don't time out when something unexpected happens.
Committed: https://crrev.com/d58d0cfb40ea275076dab537d093cd2d9a615710
Cr-Commit-Position: refs/heads/master@{#385269}
Patch Set 1 #
Total comments: 16
Patch Set 2 : negative test for encrypted video added #
Total comments: 38
Patch Set 3 : comments addressed #
Total comments: 10
Patch Set 4 : comments addressed #Patch Set 5 : testing android failure #Patch Set 6 : testing android failure with logging #Patch Set 7 : moar logging #Patch Set 8 : tests fixed #Patch Set 9 : tests fixed #Patch Set 10 : oops #Patch Set 11 : merged #Patch Set 12 : more tests #
Total comments: 11
Patch Set 13 : comments addressed #
Messages
Total messages: 27 (9 generated)
hubbe@chromium.org changed reviewers: + ddorwin@chromium.org
In the description: * _c_anPlayType * Mention H.264 https://codereview.chromium.org/1837963004/diff/1/content/browser/media/media... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1837963004/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. Add negative EME tests for hi10p to chrome/browser/media/encrypted_media_supported_types_browsertest.cc. We should probably also make sure the three profiles are each tested (currently, there are only two avc1 strings). https://codereview.chromium.org/1837963004/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:1083: // High 10-bit profile is not currently supported on android because it nit: _A_ndroid https://codereview.chromium.org/1837963004/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:1084: // relies on ffmpeg decoder fallback. This makes it sound like Android has FFmpeg fallback for H.264, which is not true. https://codereview.chromium.org/1837963004/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:1086: #if defined(OS_ANDROID) The actual test calls should not be duplicated. Instead, define kHigh10Probably with others at line 32. https://codereview.chromium.org/1837963004/diff/1/media/base/mime_util_intern... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1837963004/diff/1/media/base/mime_util_intern... media/base/mime_util_internal.cc:568: if (is_encrypted) { All of the above profiles fall through to here. This should make chrome/browser/media/encrypted_media_supported_types_browsertest.cc fail. https://codereview.chromium.org/1837963004/diff/1/media/base/mime_util_intern... media/base/mime_util_internal.cc:569: // Currently we do not support EME on 10-bit videos. Should we return false instead?
https://codereview.chromium.org/1837963004/diff/1/content/browser/media/media... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1837963004/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:1086: #if defined(OS_ANDROID) On 2016/03/31 05:44:05, ddorwin wrote: > The actual test calls should not be duplicated. Instead, define kHigh10Probably > with others at line 32. You may need more complex logic, though, to avoid failing on Chromecast, etc. See my comment in the last file. https://codereview.chromium.org/1837963004/diff/1/media/base/mime_util_intern... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1837963004/diff/1/media/base/mime_util_intern... media/base/mime_util_internal.cc:564: #if !defined(OS_ANDROID) This means that Chromecast (and other platforms that don't use FFmpeg) will claim to support Hi10. We need to be more selective and potentially limit it to FFmpeg H.264 decoder-using platforms. The tests probably need to be conditioned similarly.
Description was changed from ========== Make CanPlayType return "probably" for HI10P videos. Note that HI10P does not currently work on android or with EME, and those cases are handled in the code. ========== to ========== Make CanPlayType return "probably" for HI10P videos. Note that HI10P does not currently work on android or with EME, and those cases are handled in the code. Minor cleanup for EncryptedMediaSupportedTypes* tests so they don't time out when something unexpected happens. ==========
PTAL https://codereview.chromium.org/1837963004/diff/1/content/browser/media/media... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1837963004/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2016/03/31 05:44:05, ddorwin wrote: > Add negative EME tests for hi10p to > chrome/browser/media/encrypted_media_supported_types_browsertest.cc. We should > probably also make sure the three profiles are each tested (currently, there are > only two avc1 strings). Negative test added, also made some minor fixes to the tests so they don't time out when the tests fail. https://codereview.chromium.org/1837963004/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:1083: // High 10-bit profile is not currently supported on android because it On 2016/03/31 05:44:05, ddorwin wrote: > nit: _A_ndroid Done. https://codereview.chromium.org/1837963004/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:1084: // relies on ffmpeg decoder fallback. On 2016/03/31 05:44:05, ddorwin wrote: > This makes it sound like Android has FFmpeg fallback for H.264, which is not > true. Done. https://codereview.chromium.org/1837963004/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:1086: #if defined(OS_ANDROID) On 2016/03/31 05:44:05, ddorwin wrote: > The actual test calls should not be duplicated. Instead, define kHigh10Probably > with others at line 32. Done. https://codereview.chromium.org/1837963004/diff/1/content/browser/media/media... content/browser/media/media_canplaytype_browsertest.cc:1086: #if defined(OS_ANDROID) On 2016/03/31 05:58:44, ddorwin wrote: > On 2016/03/31 05:44:05, ddorwin wrote: > > The actual test calls should not be duplicated. Instead, define > kHigh10Probably > > with others at line 32. > > You may need more complex logic, though, to avoid failing on Chromecast, etc. > See my comment in the last file. Done. https://codereview.chromium.org/1837963004/diff/1/media/base/mime_util_intern... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1837963004/diff/1/media/base/mime_util_intern... media/base/mime_util_internal.cc:564: #if !defined(OS_ANDROID) On 2016/03/31 05:58:44, ddorwin wrote: > This means that Chromecast (and other platforms that don't use FFmpeg) will > claim to support Hi10. We need to be more selective and potentially limit it to > FFmpeg H.264 decoder-using platforms. > > The tests probably need to be conditioned similarly. Hmm, after reading a bunch of config files, I *think*, I have the right condition, but it's not easy to be sure. https://codereview.chromium.org/1837963004/diff/1/media/base/mime_util_intern... media/base/mime_util_internal.cc:568: if (is_encrypted) { On 2016/03/31 05:44:05, ddorwin wrote: > All of the above profiles fall through to here. This should make > chrome/browser/media/encrypted_media_supported_types_browsertest.cc fail. Oops, fixed. https://codereview.chromium.org/1837963004/diff/1/media/base/mime_util_intern... media/base/mime_util_internal.cc:569: // Currently we do not support EME on 10-bit videos. On 2016/03/31 05:44:05, ddorwin wrote: > Should we return false instead? We don't do that for any of the other profiles we don't support, why do it here?
Description was changed from ========== Make CanPlayType return "probably" for HI10P videos. Note that HI10P does not currently work on android or with EME, and those cases are handled in the code. Minor cleanup for EncryptedMediaSupportedTypes* tests so they don't time out when something unexpected happens. ========== to ========== Make canPlayType return "probably" for HI10P videos. Note that HI10P does not currently work on android or with EME, and those cases are handled in the code. Minor cleanup for EncryptedMediaSupportedTypes* tests so they don't time out when something unexpected happens. ==========
Description was changed from ========== Make canPlayType return "probably" for HI10P videos. Note that HI10P does not currently work on android or with EME, and those cases are handled in the code. Minor cleanup for EncryptedMediaSupportedTypes* tests so they don't time out when something unexpected happens. ========== to ========== Make canPlayType return "probably" for HI10P h264 videos. Note that HI10P does not currently work on android or with EME, and those cases are handled in the code. Minor cleanup for EncryptedMediaSupportedTypes* tests so they don't time out when something unexpected happens. ==========
ddorwin@chromium.org changed reviewers: + dalecurtis@chromium.org, jrummell@chromium.org
dalecurtis and jrummell: Please see the comments with your names below. https://codereview.chromium.org/1837963004/diff/20001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_supported_types_browsertest.cc (right): https://codereview.chromium.org/1837963004/diff/20001/chrome/browser/media/en... chrome/browser/media/encrypted_media_supported_types_browsertest.cc:113: video_mp4_codecs_.push_back("avc1.4D400C"); We should make sure we have baseline, main, and high here. We should comment each line (e.g. // "Base profile.") https://codereview.chromium.org/1837963004/diff/20001/chrome/browser/media/en... chrome/browser/media/encrypted_media_supported_types_browsertest.cc:620: CodecVector tmp; hi10_codec or something like that https://codereview.chromium.org/1837963004/diff/20001/chrome/browser/media/en... chrome/browser/media/encrypted_media_supported_types_browsertest.cc:621: tmp.push_back("avc3.6E001E"); Is there a reason to use avc3 here? https://codereview.chromium.org/1837963004/diff/20001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1837963004/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:77: const char* kHI10PProbably = kPropProbably; The logic above might not be clear, so you might comment that Hi10p is supported wherever the FFmpeg H.264 decoder is used or is available as a fallback. https://codereview.chromium.org/1837963004/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:79: const char* kHI10PProbably = kMaybe; nit: lowercase 'i' is probably more appropriate. https://codereview.chromium.org/1837963004/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:993: // This test is the same but with "avc1". Duplicate the new tests here. https://codereview.chromium.org/1837963004/diff/20001/media/base/mime_util_in... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1837963004/diff/20001/media/base/mime_util_in... media/base/mime_util_internal.cc:559: // HIGH10PROFILE is supported through fallback to the ffmpeg decoder. nit: , which https://codereview.chromium.org/1837963004/diff/20001/media/base/mime_util_in... media/base/mime_util_internal.cc:560: // Which is not available on Android, chromium builds or if Chromium is irrelevant to this (that's handled elsewhere) and no different than the other profiles, so I'd drop that part. https://codereview.chromium.org/1837963004/diff/20001/media/base/mime_util_in... media/base/mime_util_internal.cc:561: // FFMPEG has been disabled. nit suggestion: s/has been disabled/is not used/. The former makes it sound like it's an option. https://codereview.chromium.org/1837963004/diff/20001/media/base/mime_util_in... media/base/mime_util_internal.cc:562: #if !defined(MEDIA_DISABLE_FFMPEG) && !defined(OS_ANDROID) :) +dalecurtis to confirm this logic. We should probably make this more obvious when switching to build flags. https://codereview.chromium.org/1837963004/diff/20001/media/base/mime_util_in... media/base/mime_util_internal.cc:563: case H264PROFILE_HIGH10PROFILE: It might be nicer to always have this case and put the #if inside it. That does require a #else to break, though. https://codereview.chromium.org/1837963004/diff/20001/media/base/mime_util_in... media/base/mime_util_internal.cc:565: // Currently we do not support EME on 10-bit videos. More accurately, especially since we don't return false: FFmpeg is not generally used for encrypted videos, so we do not know whether 10-bit is supported. https://codereview.chromium.org/1837963004/diff/20001/media/base/mime_util_in... media/base/mime_util_internal.cc:568: } Add an explicit comment to "fall through" so it's clear this is not a bug. :) https://codereview.chromium.org/1837963004/diff/20001/media/test/data/test_ke... File media/test/data/test_key_system_instantiation.html (right): https://codereview.chromium.org/1837963004/diff/20001/media/test/data/test_ke... media/test/data/test_key_system_instantiation.html:13: document.title = "failure"; I believe the title watcher only checks the start of the string, so we can do "failure: unexpected result '" + title + "'"; +jrummell
dalecurtis@chromium.org changed reviewers: - dalecurtis@chromium.org
that portion lg2m - defer to ddorwin@ for overall review. https://codereview.chromium.org/1837963004/diff/20001/media/base/mime_util_in... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1837963004/diff/20001/media/base/mime_util_in... media/base/mime_util_internal.cc:562: #if !defined(MEDIA_DISABLE_FFMPEG) && !defined(OS_ANDROID) On 2016/03/31 at 22:31:12, ddorwin wrote: > :) +dalecurtis to confirm this logic. > > We should probably make this more obvious when switching to build flags. this is correct. android doesn't have ff_h264.
https://codereview.chromium.org/1837963004/diff/20001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_supported_types_browsertest.cc (right): https://codereview.chromium.org/1837963004/diff/20001/chrome/browser/media/en... chrome/browser/media/encrypted_media_supported_types_browsertest.cc:113: video_mp4_codecs_.push_back("avc1.4D400C"); On 2016/03/31 22:31:11, ddorwin wrote: > We should make sure we have baseline, main, and high here. We should comment > each line (e.g. // "Base profile.") Done. https://codereview.chromium.org/1837963004/diff/20001/chrome/browser/media/en... chrome/browser/media/encrypted_media_supported_types_browsertest.cc:620: CodecVector tmp; On 2016/03/31 22:31:12, ddorwin wrote: > hi10_codec or something like that Done. https://codereview.chromium.org/1837963004/diff/20001/chrome/browser/media/en... chrome/browser/media/encrypted_media_supported_types_browsertest.cc:621: tmp.push_back("avc3.6E001E"); On 2016/03/31 22:31:11, ddorwin wrote: > Is there a reason to use avc3 here? No, honestly I don't actually know what the difference between avc1 and avc3 is. Changed to avc1 since you seem to prefer that... https://codereview.chromium.org/1837963004/diff/20001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1837963004/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:77: const char* kHI10PProbably = kPropProbably; On 2016/03/31 22:31:12, ddorwin wrote: > The logic above might not be clear, so you might comment that Hi10p is supported > wherever the FFmpeg H.264 decoder is used or is available as a fallback. Comment added. https://codereview.chromium.org/1837963004/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:79: const char* kHI10PProbably = kMaybe; On 2016/03/31 22:31:12, ddorwin wrote: > nit: lowercase 'i' is probably more appropriate. Done (I also lower-cased the first P) https://codereview.chromium.org/1837963004/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:993: // On 2016/03/31 22:31:12, ddorwin wrote: > This test is the same but with "avc1". Duplicate the new tests here. Done. (Wonder if we could make this a parametric test?) https://codereview.chromium.org/1837963004/diff/20001/media/base/mime_util_in... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1837963004/diff/20001/media/base/mime_util_in... media/base/mime_util_internal.cc:559: // HIGH10PROFILE is supported through fallback to the ffmpeg decoder. On 2016/03/31 22:31:12, ddorwin wrote: > nit: , which Done. https://codereview.chromium.org/1837963004/diff/20001/media/base/mime_util_in... media/base/mime_util_internal.cc:560: // Which is not available on Android, chromium builds or if On 2016/03/31 22:31:12, ddorwin wrote: > Chromium is irrelevant to this (that's handled elsewhere) and no different than > the other profiles, so I'd drop that part. Done. https://codereview.chromium.org/1837963004/diff/20001/media/base/mime_util_in... media/base/mime_util_internal.cc:561: // FFMPEG has been disabled. On 2016/03/31 22:31:12, ddorwin wrote: > nit suggestion: s/has been disabled/is not used/. The former makes it sound like > it's an option. Done. https://codereview.chromium.org/1837963004/diff/20001/media/base/mime_util_in... media/base/mime_util_internal.cc:562: #if !defined(MEDIA_DISABLE_FFMPEG) && !defined(OS_ANDROID) On 2016/03/31 22:31:12, ddorwin wrote: > :) +dalecurtis to confirm this logic. > > We should probably make this more obvious when switching to build flags. Acknowledged. https://codereview.chromium.org/1837963004/diff/20001/media/base/mime_util_in... media/base/mime_util_internal.cc:562: #if !defined(MEDIA_DISABLE_FFMPEG) && !defined(OS_ANDROID) On 2016/03/31 22:58:44, DaleCurtis wrote: > On 2016/03/31 at 22:31:12, ddorwin wrote: > > :) +dalecurtis to confirm this logic. > > > > We should probably make this more obvious when switching to build flags. > > this is correct. android doesn't have ff_h264. Acknowledged. https://codereview.chromium.org/1837963004/diff/20001/media/base/mime_util_in... media/base/mime_util_internal.cc:563: case H264PROFILE_HIGH10PROFILE: On 2016/03/31 22:31:12, ddorwin wrote: > It might be nicer to always have this case and put the #if inside it. That does > require a #else to break, though. Tried it, but I didn't like how it turned out. https://codereview.chromium.org/1837963004/diff/20001/media/base/mime_util_in... media/base/mime_util_internal.cc:565: // Currently we do not support EME on 10-bit videos. On 2016/03/31 22:31:12, ddorwin wrote: > More accurately, especially since we don't return false: > FFmpeg is not generally used for encrypted videos, so we do not know whether > 10-bit is supported. Done. https://codereview.chromium.org/1837963004/diff/20001/media/base/mime_util_in... media/base/mime_util_internal.cc:568: } On 2016/03/31 22:31:12, ddorwin wrote: > Add an explicit comment to "fall through" so it's clear this is not a bug. :) Done. https://codereview.chromium.org/1837963004/diff/20001/media/test/data/test_ke... File media/test/data/test_key_system_instantiation.html (right): https://codereview.chromium.org/1837963004/diff/20001/media/test/data/test_ke... media/test/data/test_key_system_instantiation.html:13: document.title = "failure"; On 2016/03/31 22:31:12, ddorwin wrote: > I believe the title watcher only checks the start of the string, so we can do > "failure: unexpected result '" + title + "'"; > > +jrummell It does not: https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...
Thanks. Just nits. FYI, this will conflict with https://codereview.chromium.org/1728193004/, which moves a StringToCodec call. https://codereview.chromium.org/1837963004/diff/20001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_supported_types_browsertest.cc (right): https://codereview.chromium.org/1837963004/diff/20001/chrome/browser/media/en... chrome/browser/media/encrypted_media_supported_types_browsertest.cc:621: tmp.push_back("avc3.6E001E"); On 2016/04/01 00:20:58, hubbe wrote: > On 2016/03/31 22:31:11, ddorwin wrote: > > Is there a reason to use avc3 here? > > No, honestly I don't actually know what the difference between avc1 and avc3 is. > > Changed to avc1 since you seem to prefer that... It's just more common, so there is usually a reason that "avc3" is used. :) https://codereview.chromium.org/1837963004/diff/20001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1837963004/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:993: // On 2016/04/01 00:20:58, hubbe wrote: > On 2016/03/31 22:31:12, ddorwin wrote: > > This test is the same but with "avc1". Duplicate the new tests here. > > Done. > (Wonder if we could make this a parametric test?) Perhaps. We didn't expect it to change so the simplicity (especially important for tests) of duplication won out. https://codereview.chromium.org/1837963004/diff/20001/media/test/data/test_ke... File media/test/data/test_key_system_instantiation.html (right): https://codereview.chromium.org/1837963004/diff/20001/media/test/data/test_ke... media/test/data/test_key_system_instantiation.html:13: document.title = "failure"; On 2016/04/01 00:20:59, hubbe wrote: > On 2016/03/31 22:31:12, ddorwin wrote: > > I believe the title watcher only checks the start of the string, so we can do > > "failure: unexpected result '" + title + "'"; > > > > +jrummell > > It does not: > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... Hmm. We had one test where something like that was a problem. Maybe we should use "unexpected result" or something to be more accurate and searchable. https://codereview.chromium.org/1837963004/diff/40001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1837963004/diff/40001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:77: // to decode h264. On Android, we only use hardware codecs. Android is a little more nuanced: "... we only use platform decoders for H.264." Or perhaps replace the last two sentences with "Even though FFmpeg is used on Android, we only use platform decoders for H.264." https://codereview.chromium.org/1837963004/diff/40001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:78: // Also check if ffmpeg has been disabled. This is the main condition; Android is the corner case. The suggestion above makes this moot. https://codereview.chromium.org/1837963004/diff/40001/media/base/mime_util_in... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1837963004/diff/40001/media/base/mime_util_in... media/base/mime_util_internal.cc:560: // which is not available on Android, or if FFMPEG is not used. nit: remove this comma now https://codereview.chromium.org/1837963004/diff/40001/media/base/mime_util_in... media/base/mime_util_internal.cc:565: // know wheter 10-bit is supported. whether
Question on whether ClearKey/ExternalClearKey should also be tested. https://codereview.chromium.org/1837963004/diff/20001/media/test/data/test_ke... File media/test/data/test_key_system_instantiation.html (right): https://codereview.chromium.org/1837963004/diff/20001/media/test/data/test_ke... media/test/data/test_key_system_instantiation.html:13: document.title = "failure"; On 2016/04/01 00:36:42, ddorwin wrote: > On 2016/04/01 00:20:59, hubbe wrote: > > On 2016/03/31 22:31:12, ddorwin wrote: > > > I believe the title watcher only checks the start of the string, so we can > do > > > "failure: unexpected result '" + title + "'"; > > > > > > +jrummell > > > > It does not: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... > > Hmm. We had one test where something like that was a problem. > Maybe we should use "unexpected result" or something to be more accurate and > searchable. I think if the title is not matched completely the TitleWatcher doesn't recognize it and the test will eventually time out. https://codereview.chromium.org/1837963004/diff/40001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_supported_types_browsertest.cc (right): https://codereview.chromium.org/1837963004/diff/40001/chrome/browser/media/en... chrome/browser/media/encrypted_media_supported_types_browsertest.cc:620: // High 10-bit Profile is not supported through WideVine. Should you do a similar test for Clearkey/ExternalClearKey?
https://codereview.chromium.org/1837963004/diff/20001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_supported_types_browsertest.cc (right): https://codereview.chromium.org/1837963004/diff/20001/chrome/browser/media/en... chrome/browser/media/encrypted_media_supported_types_browsertest.cc:621: tmp.push_back("avc3.6E001E"); On 2016/04/01 00:36:42, ddorwin wrote: > On 2016/04/01 00:20:58, hubbe wrote: > > On 2016/03/31 22:31:11, ddorwin wrote: > > > Is there a reason to use avc3 here? > > > > No, honestly I don't actually know what the difference between avc1 and avc3 > is. > > > > Changed to avc1 since you seem to prefer that... > > It's just more common, so there is usually a reason that "avc3" is used. :) Acknowledged. https://codereview.chromium.org/1837963004/diff/20001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1837963004/diff/20001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:993: // On 2016/04/01 00:36:42, ddorwin wrote: > On 2016/04/01 00:20:58, hubbe wrote: > > On 2016/03/31 22:31:12, ddorwin wrote: > > > This test is the same but with "avc1". Duplicate the new tests here. > > > > Done. > > (Wonder if we could make this a parametric test?) > > Perhaps. We didn't expect it to change so the simplicity (especially important > for tests) of duplication won out. Acknowledged. https://codereview.chromium.org/1837963004/diff/20001/media/test/data/test_ke... File media/test/data/test_key_system_instantiation.html (right): https://codereview.chromium.org/1837963004/diff/20001/media/test/data/test_ke... media/test/data/test_key_system_instantiation.html:13: document.title = "failure"; On 2016/04/01 00:36:42, ddorwin wrote: > On 2016/04/01 00:20:59, hubbe wrote: > > On 2016/03/31 22:31:12, ddorwin wrote: > > > I believe the title watcher only checks the start of the string, so we can > do > > > "failure: unexpected result '" + title + "'"; > > > > > > +jrummell > > > > It does not: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... > > Hmm. We had one test where something like that was a problem. > Maybe we should use "unexpected result" or something to be more accurate and > searchable. Sounds like a good idea, done. https://codereview.chromium.org/1837963004/diff/20001/media/test/data/test_ke... media/test/data/test_key_system_instantiation.html:13: document.title = "failure"; On 2016/04/01 00:52:52, jrummell wrote: > On 2016/04/01 00:36:42, ddorwin wrote: > > On 2016/04/01 00:20:59, hubbe wrote: > > > On 2016/03/31 22:31:12, ddorwin wrote: > > > > I believe the title watcher only checks the start of the string, so we can > > do > > > > "failure: unexpected result '" + title + "'"; > > > > > > > > +jrummell > > > > > > It does not: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... > > > > Hmm. We had one test where something like that was a problem. > > Maybe we should use "unexpected result" or something to be more accurate and > > searchable. > > I think if the title is not matched completely the TitleWatcher doesn't > recognize it and the test will eventually time out. Yep, that is exactly what happens. https://codereview.chromium.org/1837963004/diff/40001/chrome/browser/media/en... File chrome/browser/media/encrypted_media_supported_types_browsertest.cc (right): https://codereview.chromium.org/1837963004/diff/40001/chrome/browser/media/en... chrome/browser/media/encrypted_media_supported_types_browsertest.cc:620: // High 10-bit Profile is not supported through WideVine. On 2016/04/01 00:52:52, jrummell wrote: > Should you do a similar test for Clearkey/ExternalClearKey? Done. https://codereview.chromium.org/1837963004/diff/40001/content/browser/media/m... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1837963004/diff/40001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:77: // to decode h264. On Android, we only use hardware codecs. On 2016/04/01 00:36:42, ddorwin wrote: > Android is a little more nuanced: "... we only use platform decoders for H.264." > > Or perhaps replace the last two sentences with "Even though FFmpeg is used on > Android, we only use platform decoders for H.264." Done. https://codereview.chromium.org/1837963004/diff/40001/content/browser/media/m... content/browser/media/media_canplaytype_browsertest.cc:78: // Also check if ffmpeg has been disabled. On 2016/04/01 00:36:42, ddorwin wrote: > This is the main condition; Android is the corner case. The suggestion above > makes this moot. Done. https://codereview.chromium.org/1837963004/diff/40001/media/base/mime_util_in... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1837963004/diff/40001/media/base/mime_util_in... media/base/mime_util_internal.cc:560: // which is not available on Android, or if FFMPEG is not used. On 2016/04/01 00:36:42, ddorwin wrote: > nit: remove this comma now Done. https://codereview.chromium.org/1837963004/diff/40001/media/base/mime_util_in... media/base/mime_util_internal.cc:565: // know wheter 10-bit is supported. On 2016/04/01 00:36:42, ddorwin wrote: > whether Done.
LGTM % comments. https://codereview.chromium.org/1837963004/diff/220001/chrome/browser/media/e... File chrome/browser/media/encrypted_media_supported_types_browsertest.cc (right): https://codereview.chromium.org/1837963004/diff/220001/chrome/browser/media/e... chrome/browser/media/encrypted_media_supported_types_browsertest.cc:59: const char kUnexpected[] = "unexpected result"; nit: name ...Result like the others https://codereview.chromium.org/1837963004/diff/220001/chrome/browser/media/e... chrome/browser/media/encrypted_media_supported_types_browsertest.cc:412: // the platform supports it. "Platform" is an overloaded word, but it will likely be associated with platform decoders here. Instead, you might say "if it is supported for clear content on this platform." https://codereview.chromium.org/1837963004/diff/220001/chrome/browser/media/e... chrome/browser/media/encrypted_media_supported_types_browsertest.cc:546: hi10_codec.push_back("avc1.6E001E"); With two appearances, I wonder if we shouldn't just make this a member like the others to keep all the literals together. Yes, this is the opposite of the canPlayType tests, but it's at least consistent. https://codereview.chromium.org/1837963004/diff/220001/content/browser/media/... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1837963004/diff/220001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:975: EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.6E400A\"'")); We should explain why these are kPropProbably. I assume it is because they set the baseline constraint bit. Same at 1080. Did you determine whether this is a valid string? Should it be kPropMaybe instead?
https://codereview.chromium.org/1837963004/diff/220001/chrome/browser/media/e... File chrome/browser/media/encrypted_media_supported_types_browsertest.cc (right): https://codereview.chromium.org/1837963004/diff/220001/chrome/browser/media/e... chrome/browser/media/encrypted_media_supported_types_browsertest.cc:638: // High 10-bit Profile is not supported when using WideVine. nit: lower case 'v' in Widevine.
https://codereview.chromium.org/1837963004/diff/220001/chrome/browser/media/e... File chrome/browser/media/encrypted_media_supported_types_browsertest.cc (right): https://codereview.chromium.org/1837963004/diff/220001/chrome/browser/media/e... chrome/browser/media/encrypted_media_supported_types_browsertest.cc:59: const char kUnexpected[] = "unexpected result"; On 2016/04/04 23:23:20, ddorwin wrote: > nit: name ...Result like the others Done. https://codereview.chromium.org/1837963004/diff/220001/chrome/browser/media/e... chrome/browser/media/encrypted_media_supported_types_browsertest.cc:412: // the platform supports it. On 2016/04/04 23:23:20, ddorwin wrote: > "Platform" is an overloaded word, but it will likely be associated with platform > decoders here. > > Instead, you might say "if it is supported for clear content on this platform." Done. https://codereview.chromium.org/1837963004/diff/220001/chrome/browser/media/e... chrome/browser/media/encrypted_media_supported_types_browsertest.cc:546: hi10_codec.push_back("avc1.6E001E"); On 2016/04/04 23:23:20, ddorwin wrote: > With two appearances, I wonder if we shouldn't just make this a member like the > others to keep all the literals together. Yes, this is the opposite of the > canPlayType tests, but it's at least consistent. Done. https://codereview.chromium.org/1837963004/diff/220001/chrome/browser/media/e... chrome/browser/media/encrypted_media_supported_types_browsertest.cc:638: // High 10-bit Profile is not supported when using WideVine. On 2016/04/04 23:24:03, ddorwin wrote: > nit: lower case 'v' in Widevine. Done. https://codereview.chromium.org/1837963004/diff/220001/content/browser/media/... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1837963004/diff/220001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:975: EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.6E400A\"'")); On 2016/04/04 23:23:20, ddorwin wrote: > We should explain why these are kPropProbably. I assume it is because they set > the baseline constraint bit. Same at 1080. > > Did you determine whether this is a valid string? Should it be kPropMaybe > instead? Reading the standard seems to indicate that this is not a supported combination. (If profile is 110, constraint bits 0, 1 and 2 must be zero, according to section 7.4.2.1.1, Note 1) Not sure if kPropMaybe is the right response for that.
lgtm https://codereview.chromium.org/1837963004/diff/220001/content/browser/media/... File content/browser/media/media_canplaytype_browsertest.cc (right): https://codereview.chromium.org/1837963004/diff/220001/content/browser/media/... content/browser/media/media_canplaytype_browsertest.cc:975: EXPECT_EQ(kPropProbably, CanPlay("'video/mp4; codecs=\"avc1.6E400A\"'")); On 2016/04/05 00:25:34, hubbe wrote: > On 2016/04/04 23:23:20, ddorwin wrote: > > We should explain why these are kPropProbably. I assume it is because they set > > the baseline constraint bit. Same at 1080. > > > > Did you determine whether this is a valid string? Should it be kPropMaybe > > instead? > > Reading the standard seems to indicate that this is not a supported combination. > (If profile is 110, constraint bits 0, 1 and 2 must be zero, according to > section 7.4.2.1.1, Note 1) Not sure if kPropMaybe is the right response for > that. This would seem similar to invalid constraint bit values, invalid levels, etc. I think we return maybe for that.
lgtm
The CQ bit was checked by hubbe@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1837963004/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1837963004/240001
Message was sent while issue was closed.
Description was changed from ========== Make canPlayType return "probably" for HI10P h264 videos. Note that HI10P does not currently work on android or with EME, and those cases are handled in the code. Minor cleanup for EncryptedMediaSupportedTypes* tests so they don't time out when something unexpected happens. ========== to ========== Make canPlayType return "probably" for HI10P h264 videos. Note that HI10P does not currently work on android or with EME, and those cases are handled in the code. Minor cleanup for EncryptedMediaSupportedTypes* tests so they don't time out when something unexpected happens. ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Make canPlayType return "probably" for HI10P h264 videos. Note that HI10P does not currently work on android or with EME, and those cases are handled in the code. Minor cleanup for EncryptedMediaSupportedTypes* tests so they don't time out when something unexpected happens. ========== to ========== Make canPlayType return "probably" for HI10P h264 videos. Note that HI10P does not currently work on android or with EME, and those cases are handled in the code. Minor cleanup for EncryptedMediaSupportedTypes* tests so they don't time out when something unexpected happens. Committed: https://crrev.com/d58d0cfb40ea275076dab537d093cd2d9a615710 Cr-Commit-Position: refs/heads/master@{#385269} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/d58d0cfb40ea275076dab537d093cd2d9a615710 Cr-Commit-Position: refs/heads/master@{#385269} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
