Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1690063002:
Fix mime type mappings when the unified media pipeline is enabled. (Closed)
|
Created:
4 years, 10 months ago by DaleCurtis Modified:
4 years, 10 months ago CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix mime type mappings when the unified media pipeline is enabled.
Enables codecs which are supported by the unified media pipeline
when enabled. Removes unnecessary compile time restrictions on the
codecs supported on Android.
Additional notes:
- Fixes VP8 codecs being included when they should be blacklisted.
- Fixes VP9 codecs being included when the platform doesn't support them.
- Fixes MSE continuing to be disabled when supported by the unified
pipeline.
BUG=559236, 582548
TEST=new mime util tests.
Committed: https://crrev.com/88af393b8b7cc535978f95f32e8c83b5e83f4ebc
Cr-Commit-Position: refs/heads/master@{#376571}
Patch Set 1 #Patch Set 2 : Include MSE fixes. #
Total comments: 12
Patch Set 3 : Fix browser tests. #
Total comments: 24
Patch Set 4 : Fix all teh things. #
Total comments: 58
Patch Set 5 : Simplify, test, rebase on split. #
Total comments: 91
Patch Set 6 : Comments. #
Total comments: 50
Patch Set 7 : Comments #
Total comments: 15
Patch Set 8 : Comments. Fix vp8 inversion. Fix vp9 exclusion. Fix hevc. #
Total comments: 3
Patch Set 9 : Avoid MSE being enabled via experiment. #Patch Set 10 : Fix compile error. #
Messages
Total messages: 59 (24 generated)
dalecurtis@chromium.org changed reviewers: + ddorwin@chromium.org, nick@chromium.org
lgtm
https://codereview.chromium.org/1690063002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1690063002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:2501: if (media::IsUnifiedMediaPipelineEnabled()) Is this logic backwards? https://codereview.chromium.org/1690063002/diff/20001/media/base/mime_util.cc File media/base/mime_util.cc (right): https://codereview.chromium.org/1690063002/diff/20001/media/base/mime_util.cc... media/base/mime_util.cc:167: return false; return IsUnifiedMediaPipelineEnabled()? https://codereview.chromium.org/1690063002/diff/20001/media/base/mime_util.cc... media/base/mime_util.cc:262: {"video/ogg", COMMON, "opus,theora,vorbis"}, Previously, "VIDEO/ogg" was not supported on Android (because there is no supported video codec). Do we want to change this? As is, this should also change the results of a canplaytype browser test. https://codereview.chromium.org/1690063002/diff/20001/media/base/mime_util.cc... media/base/mime_util.cc:455: bool MimeUtil::IsSupportedMediaMimeType(const std::string& mime_type) const { "video/ogg" will now succeed here. This is called (via the wrapper) by non-media code. https://codereview.chromium.org/1690063002/diff/20001/media/filters/stream_pa... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/1690063002/diff/20001/media/filters/stream_pa... media/filters/stream_parser_factory.cc:334: // VP9 is only supported on KitKat+ (API Level 19). Should we wrap this comment and logic up in a media.h function and use it in both places rather than spreading the Android version logic in multiple locations?
Resolved most comments, one question on refactoring. https://codereview.chromium.org/1690063002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1690063002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:2501: if (media::IsUnifiedMediaPipelineEnabled()) On 2016/02/11 21:08:24, ddorwin wrote: > Is this logic backwards? Whoops, yes I dropped the ! while editing. https://codereview.chromium.org/1690063002/diff/20001/media/base/mime_util.cc File media/base/mime_util.cc (right): https://codereview.chromium.org/1690063002/diff/20001/media/base/mime_util.cc... media/base/mime_util.cc:167: return false; On 2016/02/11 21:08:24, ddorwin wrote: > return IsUnifiedMediaPipelineEnabled()? Done. https://codereview.chromium.org/1690063002/diff/20001/media/base/mime_util.cc... media/base/mime_util.cc:262: {"video/ogg", COMMON, "opus,theora,vorbis"}, On 2016/02/11 21:08:24, ddorwin wrote: > Previously, "VIDEO/ogg" was not supported on Android (because there is no > supported video codec). Do we want to change this? > > As is, this should also change the results of a canplaytype browser test. Ah, yeah I missed that, it seems that restriction should remain. https://codereview.chromium.org/1690063002/diff/20001/media/base/mime_util.cc... media/base/mime_util.cc:455: bool MimeUtil::IsSupportedMediaMimeType(const std::string& mime_type) const { On 2016/02/11 21:08:24, ddorwin wrote: > "video/ogg" will now succeed here. This is called (via the wrapper) by non-media > code. Done. https://codereview.chromium.org/1690063002/diff/20001/media/filters/stream_pa... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/1690063002/diff/20001/media/filters/stream_pa... media/filters/stream_parser_factory.cc:334: // VP9 is only supported on KitKat+ (API Level 19). On 2016/02/11 21:08:24, ddorwin wrote: > Should we wrap this comment and logic up in a media.h function and use it in > both places rather than spreading the Android version logic in multiple > locations? WDYT about exposing IsCodecSupportedAndroid() on MimeUtil and calling it here with IsCodecSupportedAndroid(MimeUtil::(OPUS|VP9)) ?
https://codereview.chromium.org/1690063002/diff/20001/media/filters/stream_pa... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/1690063002/diff/20001/media/filters/stream_pa... media/filters/stream_parser_factory.cc:334: // VP9 is only supported on KitKat+ (API Level 19). On 2016/02/11 22:04:53, DaleCurtis wrote: > On 2016/02/11 21:08:24, ddorwin wrote: > > Should we wrap this comment and logic up in a media.h function and use it in > > both places rather than spreading the Android version logic in multiple > > locations? > > WDYT about exposing IsCodecSupportedAndroid() on MimeUtil and calling it here > with IsCodecSupportedAndroid(MimeUtil::(OPUS|VP9)) ? It seems strange to expose that, but it's better than duplicated logic. Ideally, we'd call it for every codec here, but then we'd have to convert the HISTOGRAM_ enums to Codec. Absent that, we already have logic here about _which_ codecs are special, so maybe we should just expose Is{Vp9|Opus}Supported() to avoid future use of IsCodecSupportedAndroid() by other code. https://codereview.chromium.org/1690063002/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1690063002/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:776: if (base::EndsWith(url.path(), ".webm", base::CompareCase::INSENSITIVE_ASCII)) Note that there will be other cases that _could_ work once issue 580623 is implemented that won't be caught by this and will depend on the logic below. It's probably not worth handling that case, though. https://codereview.chromium.org/1690063002/diff/40001/media/base/media.cc File media/base/media.cc (right): https://codereview.chromium.org/1690063002/diff/40001/media/base/media.cc#new... media/base/media.cc:64: bool IsUnifiedMediaPipelineEnabled() { This logic needs to account for the blacklist (see https://code.google.com/p/chromium/issues/detail?id=582548). Some of this logic is duplicated with CanUseWebMediaPlayerImpl(), but it does not account for the field trial. This probably worked okay where this code was removed from, but it doesn't work in general. I think these need to be combined or call each other and that the function called by mime_util needs more checks. https://codereview.chromium.org/1690063002/diff/40001/media/base/mime_util.cc File media/base/mime_util.cc (right): https://codereview.chromium.org/1690063002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:155: case MimeUtil::VP8: There is also the codec-specific blacklist (https://code.google.com/p/chromium/issues/detail?id=582548). Currently, this only affects VP8. Do we use and/or fallback to libvpx? (If we only use libvpx, perhaps we should make that blacklist NOTREACHED().) https://codereview.chromium.org/1690063002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:167: return IsUnifiedMediaPipelineEnabled(); As mentioned previously, we need to know that it can be used (is not blacklisted) too. https://codereview.chromium.org/1690063002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:186: IsUnifiedMediaPipelineEnabled(); This one may not be affected by the blacklist (based on comments about libvpx in render_frame_impl.cc. Another reason to make sure all this logic is together and consistent. https://codereview.chromium.org/1690063002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:199: if (base::EndsWith(mime_type, "ogg", base::CompareCase::INSENSITIVE_ASCII) && Minor: Since this comes from our code, the check can be case case sensitive. https://codereview.chromium.org/1690063002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:201: return IsUnifiedMediaPipelineEnabled(); Ditto on the blacklist. (As long as we support WMPA for such devices, we'll need to keep this function.) https://codereview.chromium.org/1690063002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:204: return true; As mentioned below, we might want to call IsCodecSupportedOnAndroid() here. https://codereview.chromium.org/1690063002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:273: #if !defined(OS_ANDROID) Should we retain this comment? // Android does not support Theora and thus video/ogg. https://codereview.chromium.org/1690063002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:685: if (!IsCodecSupportedOnAndroid(codec)) We check unsupported codecs for each call but container-codec combinations when we build the list. It seems we should be consistent. https://codereview.chromium.org/1690063002/diff/40001/media/filters/stream_pa... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/1690063002/diff/40001/media/filters/stream_pa... media/filters/stream_parser_factory.cc:330: audio_codecs->push_back(codec_info->tag); Assuming MSE supports the MPEG2_AAC_* codecs, we should probably have been checking those here too. Since these will always be available to MSE once we fully switch to the unified pipeline, it's probably not worth changing. https://codereview.chromium.org/1690063002/diff/40001/media/filters/stream_pa... media/filters/stream_parser_factory.cc:337: !media::IsUnifiedMediaPipelineEnabled()) { Potentially ditto on the blacklist, though MSE may be disabled in that case. (Reusing the MimeUtil code may make this moot.)
Patchset #4 (id:60001) has been deleted
Description was changed from ========== Fix mime type mappings when the unified media pipeline is enabled. Enables codecs which are supported by the unified media pipeline when enabled. Removes unnecessary compile time restrictions on the codecs supported on Android. BUG=559236 TEST=none ========== to ========== Fix mime type mappings when the unified media pipeline is enabled. Enables codecs which are supported by the unified media pipeline when enabled. Removes unnecessary compile time restrictions on the codecs supported on Android. Also fixes VP8 codecs being included when they should be blacklisted. BUG=559236 TEST=none ==========
Okay this cleans everything up as discussed, though the logic is still quite complicated. I'll see about writing some CanPlayType tests next week. https://codereview.chromium.org/1690063002/diff/20001/media/filters/stream_pa... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/1690063002/diff/20001/media/filters/stream_pa... media/filters/stream_parser_factory.cc:334: // VP9 is only supported on KitKat+ (API Level 19). On 2016/02/12 18:45:12, ddorwin wrote: > On 2016/02/11 22:04:53, DaleCurtis wrote: > > On 2016/02/11 21:08:24, ddorwin wrote: > > > Should we wrap this comment and logic up in a media.h function and use it in > > > both places rather than spreading the Android version logic in multiple > > > locations? > > > > WDYT about exposing IsCodecSupportedAndroid() on MimeUtil and calling it here > > with IsCodecSupportedAndroid(MimeUtil::(OPUS|VP9)) ? > > It seems strange to expose that, but it's better than duplicated logic. > > Ideally, we'd call it for every codec here, but then we'd have to convert the > HISTOGRAM_ enums to Codec. Absent that, we already have logic here about _which_ > codecs are special, so maybe we should just expose Is{Vp9|Opus}Supported() to > avoid future use of IsCodecSupportedAndroid() by other code. Done using your suggestion of Is(Vp9|Opus)Supported. https://codereview.chromium.org/1690063002/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1690063002/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:776: if (base::EndsWith(url.path(), ".webm", base::CompareCase::INSENSITIVE_ASCII)) On 2016/02/12 18:45:12, ddorwin wrote: > Note that there will be other cases that _could_ work once issue 580623 is > implemented that won't be caught by this and will depend on the logic below. > It's probably not worth handling that case, though. Yeah this is temporary, short term the only part that should remain after we have MSE+EME working is fallback for HLS and MediaCodec. Long term we'll move MediaPlayer into a mojo renderer which will let us fallback to MediaPlayer after detecting something like a broken MediaCodec + H264. https://codereview.chromium.org/1690063002/diff/40001/media/base/media.cc File media/base/media.cc (right): https://codereview.chromium.org/1690063002/diff/40001/media/base/media.cc#new... media/base/media.cc:64: bool IsUnifiedMediaPipelineEnabled() { On 2016/02/12 18:45:12, ddorwin wrote: > This logic needs to account for the blacklist (see > https://code.google.com/p/chromium/issues/detail?id=582548). > > Some of this logic is duplicated with CanUseWebMediaPlayerImpl(), but it does > not account for the field trial. > > This probably worked okay where this code was removed from, but it doesn't work > in general. I think these need to be combined or call each other and that the > function called by mime_util needs more checks. Thanks. You're correct, cleaned this up per our offline discussion. https://codereview.chromium.org/1690063002/diff/40001/media/base/mime_util.cc File media/base/mime_util.cc (right): https://codereview.chromium.org/1690063002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:155: case MimeUtil::VP8: On 2016/02/12 18:45:12, ddorwin wrote: > There is also the codec-specific blacklist > (https://code.google.com/p/chromium/issues/detail?id=582548). Currently, this > only affects VP8. Do we use and/or fallback to libvpx? > > (If we only use libvpx, perhaps we should make that blacklist NOTREACHED().) Fixed. For now I've just added an IsVp8Blacklisted() since that's the only codec with a big warning that anyone who adds more codecs to the blacklist will need to update mime util. I don't want to expose the general purpose method since it uses its own strings. https://codereview.chromium.org/1690063002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:167: return IsUnifiedMediaPipelineEnabled(); On 2016/02/12 18:45:12, ddorwin wrote: > As mentioned previously, we need to know that it can be used (is not > blacklisted) too. We'll either be using MediaPlayer or the unified media pipeline w/ software codecs here so the blacklist doesn't apply to this case; though |is_encrypted| does. https://codereview.chromium.org/1690063002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:186: IsUnifiedMediaPipelineEnabled(); On 2016/02/12 18:45:12, ddorwin wrote: > This one may not be affected by the blacklist (based on comments about libvpx in > render_frame_impl.cc. Another reason to make sure all this logic is together and > consistent. The blacklist doesn't matter here (currently), so I've just expanded this check to include |is_encrypted|. https://codereview.chromium.org/1690063002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:199: if (base::EndsWith(mime_type, "ogg", base::CompareCase::INSENSITIVE_ASCII) && On 2016/02/12 18:45:12, ddorwin wrote: > Minor: Since this comes from our code, the check can be case case sensitive. Done. https://codereview.chromium.org/1690063002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:201: return IsUnifiedMediaPipelineEnabled(); On 2016/02/12 18:45:12, ddorwin wrote: > Ditto on the blacklist. > > (As long as we support WMPA for such devices, we'll need to keep this function.) Blacklist doesn't matter here since either the pipeline is enabled and we're using software codecs or its not and we don't use MediaCodec. |is_encrypted| would matter, but we don't support encrypted ogg files, so I think this is fine. https://codereview.chromium.org/1690063002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:204: return true; On 2016/02/12 18:45:12, ddorwin wrote: > As mentioned below, we might want to call IsCodecSupportedOnAndroid() here. Done. https://codereview.chromium.org/1690063002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:273: #if !defined(OS_ANDROID) On 2016/02/12 18:45:12, ddorwin wrote: > Should we retain this comment? > // Android does not support Theora and thus video/ogg. Done. https://codereview.chromium.org/1690063002/diff/40001/media/base/mime_util.cc... media/base/mime_util.cc:685: if (!IsCodecSupportedOnAndroid(codec)) On 2016/02/12 18:45:12, ddorwin wrote: > We check unsupported codecs for each call but container-codec combinations when > we build the list. It seems we should be consistent. Done. https://codereview.chromium.org/1690063002/diff/40001/media/filters/stream_pa... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/1690063002/diff/40001/media/filters/stream_pa... media/filters/stream_parser_factory.cc:330: audio_codecs->push_back(codec_info->tag); On 2016/02/12 18:45:12, ddorwin wrote: > Assuming MSE supports the MPEG2_AAC_* codecs, we should probably have been > checking those here too. Since these will always be available to MSE once we > fully switch to the unified pipeline, it's probably not worth changing. Yeah, just going to leave this one alone. https://codereview.chromium.org/1690063002/diff/40001/media/filters/stream_pa... media/filters/stream_parser_factory.cc:337: !media::IsUnifiedMediaPipelineEnabled()) { On 2016/02/12 18:45:12, ddorwin wrote: > Potentially ditto on the blacklist, though MSE may be disabled in that case. > (Reusing the MimeUtil code may make this moot.) Should be fixed now since !media_codec => unified media pipeline is enabled for MSE.
https://codereview.chromium.org/1690063002/diff/80001/media/base/media.cc File media/base/media.cc (right): https://codereview.chromium.org/1690063002/diff/80001/media/base/media.cc#new... media/base/media.cc:105: return IsUnifiedMediaPipelineEnabled() || Just realized this part is wrong. It should not include the field trial test here. Will fix next week.
Thanks. This is complex. https://codereview.chromium.org/1690063002/diff/80001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1690063002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:760: // Don't use WMPI if the container likely contains h264 and hardware decoders nit: s/h264/non-vpx codec/? IOW, if we don't have a SW codec. h264 happens to be that case, but there's nothing special about h264 specifically and it's not checked below. https://codereview.chromium.org/1690063002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:764: (base::CommandLine::ForCurrentProcess()->HasSwitch( Should this be a call to HasPlatformDecoderSupport() instead? https://codereview.chromium.org/1690063002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:771: // decode may still be blacklisted for some codecs and platforms based on nit: Make it clearer that this means "there could still be problems after true is returned." How do we handle that (as of today, not the future Mojo solution)? https://codereview.chromium.org/1690063002/diff/80001/media/base/android/medi... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/1690063002/diff/80001/media/base/android/medi... media/base/android/media_codec_util.cc:260: return false; // Android MediaPlayer will be used. ? Still, I wonder if IsMediaCodecAvailable() covers all the cases. What exactly is this handling? https://codereview.chromium.org/1690063002/diff/80001/media/base/media.cc File media/base/media.cc (right): https://codereview.chromium.org/1690063002/diff/80001/media/base/media.cc#new... media/base/media.cc:102: // The unified media pipeline is available for MSE even when not normally This comment applies to the second half of the condition. Perhaps break up the return so that this comment can be above line 106 only. https://codereview.chromium.org/1690063002/diff/80001/media/base/media.cc#new... media/base/media.cc:103: // enabled if the MediaCodec is not available -- since otherwise it would This won't work unless you change the path that disables the MSE APIs altogether in runtime_features.cc. https://codereview.chromium.org/1690063002/diff/80001/media/base/media.cc#new... media/base/media.cc:104: // mean that MSE can't be used at all. Codecs that are only supported by the platform (e.g. H.264) will not be available. https://codereview.chromium.org/1690063002/diff/80001/media/base/media.h File media/base/media.h (right): https://codereview.chromium.org/1690063002/diff/80001/media/base/media.h#newc... media/base/media.h:25: MEDIA_EXPORT void EnablePlatformDecoderSupport(); This is only called for Android. Should these be ifdef'd? https://codereview.chromium.org/1690063002/diff/80001/media/base/media.h#newc... media/base/media.h:31: MEDIA_EXPORT bool IsUnifiedMediaPipelineEnabled(); // Returns only whether the pipeline is enabled. It may not actually be available, such as due to blacklisting. https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc File media/base/mime_util.cc (right): https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:61: bool is_encrypted, nit: It seems odd that |is_encrypted| is the first and "most important" parameter. https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:86: // supported by the platform when contained in |mime_type_lower_case|. "platform" - This was the existing phrasing, but elsewhere (*PlatformDecoderSupport) this CL uses it to mean OS-level codecs. Is there a better phrasing? https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:89: // is returned if at least one codec ID is not supported by the platform. nit: two spaces https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:90: // |is_encrypted| means the codec will be used with encrypted samples. nit: s/samples/blocks/? https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:108: // Returns true if |codec| and |mime_type_lower_case| are supported by the Add "when contained in" as above? https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:109: // platform. Note: This method will return false if the platform supports ditto for "platform" https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:111: // |is_encrypted| means the codec will be used with encrypted samples. ditto for "samples" https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:163: return is_encrypted ? MediaCodecUtil::IsMediaCodecAvailable() : true; Note: The !is_encrypted path works for MSE because MSE will not be enabled if IsMediaCodecAvailable() is false. (Otherwise, it would be similar to the is_encrypted path.) Perhaps this should be noted in a function-level comment. https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:169: return HasPlatformDecoderSupport() && All HasPlatformDecoderSupport() means is that the HW video decoding is not disabled. The name seems misleading. (Also, on Android, it may be disabling audio codecs too(?).) https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:172: return is_encrypted ? MediaCodecUtil::IsMediaCodecAvailable() : true; Why are encrypted videos not also subject to HasPlatformDecoderSupport()? https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:175: if (!is_encrypted) Doesn't encrypted require IsMediaCodecAvailable() (and HasPlatformDecoderSupport())? https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:178: if (MediaCodecUtil::IsVp8Blacklisted()) Is VP8 blacklisted for Android MediaPlayer too? It seems we should return true early for the WMPA non-encrypted non-MSE* case, then check the blacklist, then check the others. * Of course, we have no way to know if this is MSE here. https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:181: return IsUnifiedMediaPipelineEnabled() ? HasPlatformDecoderSupport() libvpx can't used for VP8? https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:193: // media pipeline can be used. ... and the software decoders... https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:198: return true; Perhaps: // Software decoder. https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:234: return has_unified_media_pipeline ? HasPlatformDecoderSupport() : true; Why is HasPlatformDecoderSupport() only required for the unified pipeline? All encrypted media uses the platform decoders. It seems 231 & 234 should be collapsed. (That would also make it a bit clearer that this is all handling the encrypted case.) https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:788: SupportsType IsSupportedMediaFormat(const std::string& mime_type, To avoid misuse in the future, should we rename this to IsSupportedClearMediaFormat? We don't need to do that churn in this CL. https://codereview.chromium.org/1690063002/diff/80001/media/filters/stream_pa... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/1690063002/diff/80001/media/filters/stream_pa... media/filters/stream_parser_factory.cc:331: return true; Should we add a comment that in the unified pipeline, all audio codecs are software? Really, though, that is an assumption that only holds for Chrome browser. Perhaps MSE should be validating all the codecs here just like the EME path. Perhaps we should just file a bug to consider since this is a larger issue than this CL. https://codereview.chromium.org/1690063002/diff/80001/media/filters/stream_pa... media/filters/stream_parser_factory.cc:333: #if defined(OS_ANDROID) Given the MediaCodec not available path in IsUnifiedMediaPipelineEnabledForMse(), do we need to add an H264 check too? I think it would be "if (!MediaCodecUtil::IsMediaCodecAvailable()), return false;"
Patchset #5 (id:100001) has been deleted
Patchset #5 (id:120001) has been deleted
Patchset #6 (id:150001) has been deleted
Patchset #5 (id:130001) has been deleted
PTAL. Thanks for the detailed review. Now includes 200+ lines of tests :) https://codereview.chromium.org/1690063002/diff/80001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1690063002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:760: // Don't use WMPI if the container likely contains h264 and hardware decoders On 2016/02/16 20:34:36, ddorwin wrote: > nit: s/h264/non-vpx codec/? IOW, if we don't have a SW codec. h264 happens to be > that case, but there's nothing special about h264 specifically and it's not > checked below. Done. https://codereview.chromium.org/1690063002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:764: (base::CommandLine::ForCurrentProcess()->HasSwitch( On 2016/02/16 20:34:36, ddorwin wrote: > Should this be a call to HasPlatformDecoderSupport() instead? Done. https://codereview.chromium.org/1690063002/diff/80001/content/renderer/render... content/renderer/render_frame_impl.cc:771: // decode may still be blacklisted for some codecs and platforms based on On 2016/02/16 20:34:36, ddorwin wrote: > nit: Make it clearer that this means "there could still be problems after true > is returned." > > How do we handle that (as of today, not the future Mojo solution)? Deleted this whole comment since I think it's just confusing. If we return true here we 100% expect to be able to play the clip. Some codecs may fallback to software though. https://codereview.chromium.org/1690063002/diff/80001/media/base/android/medi... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/1690063002/diff/80001/media/base/android/medi... media/base/android/media_codec_util.cc:260: return false; On 2016/02/16 20:34:36, ddorwin wrote: > // Android MediaPlayer will be used. ? > > Still, I wonder if IsMediaCodecAvailable() covers all the cases. What exactly is > this handling? I don't think we need to mention MediaPlayer here, this is after all "MediaCodecUtil" :) I've added that comment to mime_util though. The IsMediaCodecAvailable() method is a complete-device blacklist, whereas this just blacklists the vp8 codec. E.g. we'll still use the MediaCodec for h264. https://codereview.chromium.org/1690063002/diff/80001/media/base/media.cc File media/base/media.cc (right): https://codereview.chromium.org/1690063002/diff/80001/media/base/media.cc#new... media/base/media.cc:102: // The unified media pipeline is available for MSE even when not normally On 2016/02/16 20:34:36, ddorwin wrote: > This comment applies to the second half of the condition. Perhaps break up the > return so that this comment can be above line 106 only. Removed the comment and put it on the method prototype instead. https://codereview.chromium.org/1690063002/diff/80001/media/base/media.cc#new... media/base/media.cc:103: // enabled if the MediaCodec is not available -- since otherwise it would On 2016/02/16 20:34:36, ddorwin wrote: > This won't work unless you change the path that disables the MSE APIs altogether > in runtime_features.cc. Thanks for the pointer, done! https://codereview.chromium.org/1690063002/diff/80001/media/base/media.cc#new... media/base/media.cc:105: return IsUnifiedMediaPipelineEnabled() || On 2016/02/13 05:54:28, DaleCurtis wrote: > Just realized this part is wrong. It should not include the field trial test > here. Will fix next week. Disregard, this seems okay. Don't remember why I wrote this :) https://codereview.chromium.org/1690063002/diff/80001/media/base/media.h File media/base/media.h (right): https://codereview.chromium.org/1690063002/diff/80001/media/base/media.h#newc... media/base/media.h:25: MEDIA_EXPORT void EnablePlatformDecoderSupport(); On 2016/02/16 20:34:37, ddorwin wrote: > This is only called for Android. Should these be ifdef'd? Done. https://codereview.chromium.org/1690063002/diff/80001/media/base/media.h#newc... media/base/media.h:31: MEDIA_EXPORT bool IsUnifiedMediaPipelineEnabled(); On 2016/02/16 20:34:37, ddorwin wrote: > // Returns only whether the pipeline is enabled. It may not actually be > available, such as due to blacklisting. Done. https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc File media/base/mime_util.cc (right): https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:61: bool is_encrypted, On 2016/02/16 20:34:37, ddorwin wrote: > nit: It seems odd that |is_encrypted| is the first and "most important" > parameter. Had to redo all of these changes after the split, so done :) https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:86: // supported by the platform when contained in |mime_type_lower_case|. On 2016/02/16 20:34:37, ddorwin wrote: > "platform" - This was the existing phrasing, but elsewhere > (*PlatformDecoderSupport) this CL uses it to mean OS-level codecs. Is there a > better phrasing? Deleted platform from wording here. https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:89: // is returned if at least one codec ID is not supported by the platform. On 2016/02/16 20:34:37, ddorwin wrote: > nit: two spaces Done. https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:90: // |is_encrypted| means the codec will be used with encrypted samples. On 2016/02/16 20:34:37, ddorwin wrote: > nit: s/samples/blocks/? Done. https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:108: // Returns true if |codec| and |mime_type_lower_case| are supported by the On 2016/02/16 20:34:37, ddorwin wrote: > Add "when contained in" as above? Done. https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:109: // platform. Note: This method will return false if the platform supports On 2016/02/16 20:34:37, ddorwin wrote: > ditto for "platform" Done. https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:111: // |is_encrypted| means the codec will be used with encrypted samples. On 2016/02/16 20:34:37, ddorwin wrote: > ditto for "samples" Done. https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:163: return is_encrypted ? MediaCodecUtil::IsMediaCodecAvailable() : true; On 2016/02/16 20:34:37, ddorwin wrote: > Note: The !is_encrypted path works for MSE because MSE will not be enabled if > IsMediaCodecAvailable() is false. (Otherwise, it would be similar to the > is_encrypted path.) > > Perhaps this should be noted in a function-level comment. As discussed via chat, added a comment above the switch about why we don't care about MSE here. https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:169: return HasPlatformDecoderSupport() && On 2016/02/16 20:34:37, ddorwin wrote: > All HasPlatformDecoderSupport() means is that the HW video decoding is not > disabled. The name seems misleading. (Also, on Android, it may be disabling > audio codecs too(?).) It can mean: no gpu process, vda is blacklisted, or !media_codec. As such, I think the name is okay the way it is. It includes any reason why we wouldn't have platform decoder support. Audio should be included for encrypted codecs since we'll be hosting the decoder in the GPU process. https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:172: return is_encrypted ? MediaCodecUtil::IsMediaCodecAvailable() : true; On 2016/02/16 20:34:37, ddorwin wrote: > Why are encrypted videos not also subject to HasPlatformDecoderSupport()? Done. https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:175: if (!is_encrypted) On 2016/02/16 20:34:37, ddorwin wrote: > Doesn't encrypted require IsMediaCodecAvailable() (and > HasPlatformDecoderSupport())? Done. https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:178: if (MediaCodecUtil::IsVp8Blacklisted()) On 2016/02/16 20:34:37, ddorwin wrote: > Is VP8 blacklisted for Android MediaPlayer too? > > It seems we should return true early for the WMPA non-encrypted non-MSE* case, > then check the blacklist, then check the others. > > * Of course, we have no way to know if this is MSE here. No it's only blacklisted for MediaCodec, I've cleaned this up a fair bit. https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:181: return IsUnifiedMediaPipelineEnabled() ? HasPlatformDecoderSupport() On 2016/02/16 20:34:37, ddorwin wrote: > libvpx can't used for VP8? This was for encrypted cases, but is unclear, I've simplified the logic. https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:193: // media pipeline can be used. On 2016/02/16 20:34:37, ddorwin wrote: > ... and the software decoders... Done. https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:198: return true; On 2016/02/16 20:34:37, ddorwin wrote: > Perhaps: > // Software decoder. Done. https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:234: return has_unified_media_pipeline ? HasPlatformDecoderSupport() : true; On 2016/02/16 20:34:37, ddorwin wrote: > Why is HasPlatformDecoderSupport() only required for the unified pipeline? All > encrypted media uses the platform decoders. > > It seems 231 & 234 should be collapsed. (That would also make it a bit clearer > that this is all handling the encrypted case.) Again this was confusing, though correct, simplified greatly. https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:788: SupportsType IsSupportedMediaFormat(const std::string& mime_type, On 2016/02/16 20:34:37, ddorwin wrote: > To avoid misuse in the future, should we rename this to > IsSupportedClearMediaFormat? We don't need to do that churn in this CL. Acknowledged. https://codereview.chromium.org/1690063002/diff/80001/media/filters/stream_pa... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/1690063002/diff/80001/media/filters/stream_pa... media/filters/stream_parser_factory.cc:331: return true; On 2016/02/16 20:34:37, ddorwin wrote: > Should we add a comment that in the unified pipeline, all audio codecs are > software? > > Really, though, that is an assumption that only holds for Chrome browser. > Perhaps MSE should be validating all the codecs here just like the EME path. > Perhaps we should just file a bug to consider since this is a larger issue than > this CL. filed http://crbug.com/587303 https://codereview.chromium.org/1690063002/diff/80001/media/filters/stream_pa... media/filters/stream_parser_factory.cc:333: #if defined(OS_ANDROID) On 2016/02/16 20:34:37, ddorwin wrote: > Given the MediaCodec not available path in > IsUnifiedMediaPipelineEnabledForMse(), do we need to add an H264 check too? I > think it would be "if (!MediaCodecUtil::IsMediaCodecAvailable()), return false;" Good catch, done.
Description was changed from ========== Fix mime type mappings when the unified media pipeline is enabled. Enables codecs which are supported by the unified media pipeline when enabled. Removes unnecessary compile time restrictions on the codecs supported on Android. Also fixes VP8 codecs being included when they should be blacklisted. BUG=559236 TEST=none ========== to ========== Fix mime type mappings when the unified media pipeline is enabled. Enables codecs which are supported by the unified media pipeline when enabled. Removes unnecessary compile time restrictions on the codecs supported on Android. Additional notes: - Fixes VP8 codecs being included when they should be blacklisted. - Fixes MSE continuing to be disabled when supported by the unified pipeline. BUG=559236 TEST=new mime util tests. ==========
Description was changed from ========== Fix mime type mappings when the unified media pipeline is enabled. Enables codecs which are supported by the unified media pipeline when enabled. Removes unnecessary compile time restrictions on the codecs supported on Android. Additional notes: - Fixes VP8 codecs being included when they should be blacklisted. - Fixes MSE continuing to be disabled when supported by the unified pipeline. BUG=559236 TEST=new mime util tests. ========== to ========== Fix mime type mappings when the unified media pipeline is enabled. Enables codecs which are supported by the unified media pipeline when enabled. Removes unnecessary compile time restrictions on the codecs supported on Android. Additional notes: - Fixes VP8 codecs being included when they should be blacklisted. - Fixes MSE continuing to be disabled when supported by the unified pipeline. BUG=559236 TEST=new mime util tests. ==========
Thanks for refactoring. This is easier to follow. Mostly minor stuff and nits. https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc File media/base/mime_util.cc (right): https://codereview.chromium.org/1690063002/diff/80001/media/base/mime_util.cc... media/base/mime_util.cc:169: return HasPlatformDecoderSupport() && On 2016/02/17 03:01:05, DaleCurtis wrote: > On 2016/02/16 20:34:37, ddorwin wrote: > > All HasPlatformDecoderSupport() means is that the HW video decoding is not > > disabled. The name seems misleading. (Also, on Android, it may be disabling > > audio codecs too(?).) > > It can mean: no gpu process, vda is blacklisted, or !media_codec. As such, I > think the name is okay the way it is. It includes any reason why we wouldn't > have platform decoder support. Audio should be included for encrypted codecs > since we'll be hosting the decoder in the GPU process. The MediaCodec check was added in the next patch set, so this makes sense now. https://codereview.chromium.org/1690063002/diff/170001/media/base/android/med... File media/base/android/media_codec_util.h (right): https://codereview.chromium.org/1690063002/diff/170001/media/base/android/med... media/base/android/media_codec_util.h:70: // MediaCodec is available or can successfully be configured. Is it confusing to have these in this class then rather than in media.h or similar? https://codereview.chromium.org/1690063002/diff/170001/media/base/media.h File media/base/media.h (right): https://codereview.chromium.org/1690063002/diff/170001/media/base/media.h#new... media/base/media.h:25: // Tells the media library it has support for OS level decoders. This only applies to MediaCodec, not MediaPlayer. :) https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:326: platform_info_.has_vp8 = MediaCodecUtil::IsVp8Blacklisted(); ! https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:326: platform_info_.has_vp8 = MediaCodecUtil::IsVp8Blacklisted(); VP8 is different from Opus and VP9. The VP8 blacklisting only applies to MediaCodec, right? Whereas the Opus and VP9 checks apply to MediaPlayer too. The functions are on MediaCodecUtil, which makes the relevance to MediaPlayer confusing... The difference - without being communicated by the names - also confused me when reviewing the code below. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:477: void MimeUtil::SetPlatformCodecInfoForTests(const PlatformCodecInfo& info) { #ifdef https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:563: bool MimeUtil::IsDefaultCodecSupportedLowerCase( Unrelated to this CL: These ...DefaultCodec...LowerCase functions seem poorly named. It's the MIME type that is lower case, but it's never mentioned. ...DefaultCodec...{In|For}MimeTypeLowerCase? Also, the implementation(s) should probably DCHECK that the string is lower case [ASCII]. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:573: bool MimeUtil::IsCodecSupportedOnAndroid( Much simpler now. Thanks! https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:593: // The following codecs may be supported depending on platform abilities. nit: This comment appears to apply to these 6 codecs, but I think it applies to all. (Actually, this is not strictly true for the first 6.) Perhaps: // The remaining... <empty line> https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:595: case MP3: Note: The results for proprietary audio codecs are incorrect when has_unified_media_pipeline && ffmpeg_branding!=Chrome* If there was a preprocessor define for the latter, this would be easy to address, but I don't believe there is. (proprietary_codecs is always set for Android, so we can't check that here.) There are various other bugs that might make this easier to address. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:606: case H264_BASELINE: We should try to keep audio and video (especially VPx) together. These were out of order because we had a big block that all returned true. Now that we have logic, I think we should group them and try to keep the order as consistent with the enum definition as possible. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:618: if (is_encrypted) I wonder if we should swap the logic here to be more consistent with VP9, especially if we simplify VP9 per my comments there. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:621: // MediaPlayer or the unified pipeline can always play vp8 via software. nit: This wording is ambiguous. MP does not use SW. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:622: return true; Note: This is not true for MSE and !has_unified_media_pipeline, but MSE doesn't currently use this code. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:640: // Android does not support opus in ogg containers. "The Android platform..."? "Android MediaPlayer..."? (Not sure if MediaCodec would support Opus extracted from Ogg, but I think that's irrelevant here since MSE and EME do not support it.) https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:646: DCHECK(!is_encrypted || platform_info_.has_platform_decoder); This is one place where the differences in the has_<codec> values is confusing. has_opus does not imply has_platform_decoder (but we would have bailed on this condition above). https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:664: if (!platform_info_.has_vp9) At this point, can we just return the value of this? https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... File media/base/mime_util_internal.h (right): https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.h:21: // Platform configuration structure. Controls which codecs are supported at Is there a reason this isn't nested in the class? https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.h:24: bool has_platform_decoder; nit: decoder*s*? Since it's Android, should we just say has_mediacodec? Also, "platform" is redundant here and makes it look like the last three are missing it. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.h:25: bool has_unified_media_pipeline; nit: This isn't really a platform property. Perhaps change it to use_... and move it to the top or bottom. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.h:28: bool has_vp9; define default values? https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.h:73: void SetPlatformCodecInfoForTests(const PlatformCodecInfo& info); Move inside #ifdef. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.h:102: // codecs are supported. IsNotSupported is returned if at least one codec ID To be absolutely clear: ... supported in |mime_type_lower_case|. IsNotSupported is returned if |mime_type_lower_case| is not supported or at least one is not supported in |mime_type_lower_case|. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.h:107: const std::string& mime_type_lower_case, consistency nit: Elsewhere, the mime_type parameter is first. The fact that this function name is asking about "Codecs" makes it less clear that this would be a better ordering, especially since we also have |supported_codecs|, which is inconsistent with the others. Perhaps that should be just before |is_encrypted|. Anyway, up to you. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.h:124: // |allow_proprietary_codecs_| is set to false. |is_encrypted| means the codec You dropped part of the text from the Note. It will return false for proprietary codes regardless of platform codec support. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.h:127: const std::string& mime_type_lower_case, ditto on order consistency https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.h:148: const std::string& mime_type_lower_case, ditto on order consistency https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... File media/base/mime_util_unittest.cc (right): https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:120: TEST(MimeUtilInternalTest, EncryptedCodecsFailWithoutPlatformSupport) { These test names should include/refer to IsCodecSupportedOnAndroidForTests. i.e. IsCodecSupportedOnAndroidForTests_EncryptedCodecsFailWithoutPlatformSupport, though that is too long. Maybe drop "ForTests". https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:125: const bool kTestStates[] = {true, false}; remove https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:137: ASSERT_FALSE(mime_util.IsCodecSupportedOnAndroidForTests( EXPECT_ throughout. We want to run all cases even if one fails, right? https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:138: static_cast<MimeUtil::Codec>(codec), "", true)); It's interesting that the function allows an empty mime type string. Should that be DCHECKed (and "foo" used here)? https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:161: const MimeUtil::Codec codec_type = nit: codec type sounds like there are types of codecs. Perhaps s/codec/codec_number/ or s/codec_type/codec_value/. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:171: ASSERT_FALSE(mime_util.IsCodecSupportedOnAndroidForTests( If one of these fails, it's going to be difficult to know which combination failed. Perhaps we should add "<< PrintInfo(info)" and implement the print function. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:207: break; ? Can we check the ifdef? We should at least call for this type to make sure it doesn't DCHECK or crash. If !BUILDFLAG(ENABLE_HEVC_DEMUXING), it will always be false - that's something we can check. If we wanted to, we could check the Android version in the other case too. Another option would be to handle this similar to VP9. However, I'm not sure we want to do that since it is not used in most cases. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:216: TEST(MimeUtilInternalTest, ClearCodecBehaviorWithStandardPipeline) { s/Standard/Android/? https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:218: PlatformCodecInfo info = {0}; has_unified_media_pipeline is never set to false (per the test). https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:218: PlatformCodecInfo info = {0}; has_platform_decoder and has_vp8 are not set. (Currently, their value is non-deterministic.) https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:265: break; ditto https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:274: PlatformCodecInfo info = {0}; Why aren't has_<codec> looped through? https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:317: break; ditto, though this also requires has_platform_decoder https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:326: info.has_platform_decoder = false; Should this matter? Can we loop through it? https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:327: info.has_opus = true; Might as well loop throught his too? https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:329: const bool kTestStates[] = {true, false}; remove https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:340: #endif This is a long block. Add // defined(OS_ANDROID)? https://codereview.chromium.org/1690063002/diff/170001/media/filters/stream_p... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/1690063002/diff/170001/media/filters/stream_p... media/filters/stream_parser_factory.cc:329: if (audio_codecs) Note: This code is also inaccurate for when IsUnifiedMediaPipelineEnabledForMse() and ffmpeg_branding!=Chrome*. https://codereview.chromium.org/1690063002/diff/170001/media/filters/stream_p... media/filters/stream_parser_factory.cc:338: if (codec_info->tag == CodecInfo::HISTOGRAM_VP9 && Do we need to check VP8 blacklisting as well?
Patchset #6 (id:190001) has been deleted
Thanks for the thorough review. All comments should be addressed! https://codereview.chromium.org/1690063002/diff/170001/media/base/android/med... File media/base/android/media_codec_util.h (right): https://codereview.chromium.org/1690063002/diff/170001/media/base/android/med... media/base/android/media_codec_util.h:70: // MediaCodec is available or can successfully be configured. On 2016/02/17 21:18:38, ddorwin wrote: > Is it confusing to have these in this class then rather than in media.h or > similar? Good point. Moved, though I think we'll want a better location still. https://codereview.chromium.org/1690063002/diff/170001/media/base/media.h File media/base/media.h (right): https://codereview.chromium.org/1690063002/diff/170001/media/base/media.h#new... media/base/media.h:25: // Tells the media library it has support for OS level decoders. On 2016/02/17 21:18:38, ddorwin wrote: > This only applies to MediaCodec, not MediaPlayer. :) Done. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:326: platform_info_.has_vp8 = MediaCodecUtil::IsVp8Blacklisted(); On 2016/02/17 21:18:39, ddorwin wrote: > VP8 is different from Opus and VP9. The VP8 blacklisting only applies to > MediaCodec, right? Whereas the Opus and VP9 checks apply to MediaPlayer too. The > functions are on MediaCodecUtil, which makes the relevance to MediaPlayer > confusing... > > The difference - without being communicated by the names - also confused me when > reviewing the code below. Done and changed to has_encrypted_vp8 since that's the only context it's used in. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:477: void MimeUtil::SetPlatformCodecInfoForTests(const PlatformCodecInfo& info) { On 2016/02/17 21:18:38, ddorwin wrote: > #ifdef See previous comment. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:563: bool MimeUtil::IsDefaultCodecSupportedLowerCase( On 2016/02/17 21:18:39, ddorwin wrote: > Unrelated to this CL: > These ...DefaultCodec...LowerCase functions seem poorly named. It's the MIME > type that is lower case, but it's never mentioned. > > ...DefaultCodec...{In|For}MimeTypeLowerCase? > > Also, the implementation(s) should probably DCHECK that the string is lower case > [ASCII]. Acknowledged. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:573: bool MimeUtil::IsCodecSupportedOnAndroid( On 2016/02/17 21:18:38, ddorwin wrote: > Much simpler now. Thanks! Acknowledged. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:593: // The following codecs may be supported depending on platform abilities. On 2016/02/17 21:18:38, ddorwin wrote: > nit: This comment appears to apply to these 6 codecs, but I think it applies to > all. (Actually, this is not strictly true for the first 6.) > > Perhaps: > // The remaining... > <empty line> Done. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:595: case MP3: On 2016/02/17 21:18:38, ddorwin wrote: > Note: The results for proprietary audio codecs are incorrect when > has_unified_media_pipeline && ffmpeg_branding!=Chrome* > If there was a preprocessor define for the latter, this would be easy to > address, but I don't believe there is. (proprietary_codecs is always set for > Android, so we can't check that here.) There are various other bugs that might > make this easier to address. This is correct, but should be a temporary issue once sorted out with legal, so I won't bother muddling this code any further. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:606: case H264_BASELINE: On 2016/02/17 21:18:39, ddorwin wrote: > We should try to keep audio and video (especially VPx) together. > > These were out of order because we had a big block that all returned true. Now > that we have logic, I think we should group them and try to keep the order as > consistent with the enum definition as possible. Done. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:618: if (is_encrypted) On 2016/02/17 21:18:39, ddorwin wrote: > I wonder if we should swap the logic here to be more consistent with VP9, > especially if we simplify VP9 per my comments there. This logic is not quite the same since MediaPlayer can always play vp8, but we want to excluded encrypted vp8 if MediaCodec/PlatformDecoders are not available. I've reworked it to be consistent with the others though and renamed has_vp8 to supports_encrypted_vp8. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:621: // MediaPlayer or the unified pipeline can always play vp8 via software. On 2016/02/17 21:18:39, ddorwin wrote: > nit: This wording is ambiguous. MP does not use SW. Done. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:622: return true; On 2016/02/17 21:18:39, ddorwin wrote: > Note: This is not true for MSE and !has_unified_media_pipeline, but MSE doesn't > currently use this code. Done. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:640: // Android does not support opus in ogg containers. On 2016/02/17 21:18:38, ddorwin wrote: > "The Android platform..."? > "Android MediaPlayer..."? (Not sure if MediaCodec would support Opus extracted > from Ogg, but I think that's irrelevant here since MSE and EME do not support > it.) Done. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:646: DCHECK(!is_encrypted || platform_info_.has_platform_decoder); On 2016/02/17 21:18:39, ddorwin wrote: > This is one place where the differences in the has_<codec> values is confusing. > has_opus does not imply has_platform_decoder (but we would have bailed on this > condition above). Hopefully has_xxx vs supports_xxx helps this confusion. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.cc:664: if (!platform_info_.has_vp9) On 2016/02/17 21:18:39, ddorwin wrote: > At this point, can we just return the value of this? Done. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... File media/base/mime_util_internal.h (right): https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.h:21: // Platform configuration structure. Controls which codecs are supported at On 2016/02/17 21:18:39, ddorwin wrote: > Is there a reason this isn't nested in the class? Done. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.h:24: bool has_platform_decoder; On 2016/02/17 21:18:39, ddorwin wrote: > nit: decoder*s*? > Since it's Android, should we just say has_mediacodec? > Also, "platform" is redundant here and makes it look like the last three are > missing it. I'd like to keep the name consistent with what we have in media.h, which does not refer to MediaCodec. Ditto for keeping "platform" in the name. I'm opening to alternative names though. To appease your concerns I renamed has_{opus,vp8,vp9} to supports_() which I think makes the distinction clearer. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.h:25: bool has_unified_media_pipeline; On 2016/02/17 21:18:39, ddorwin wrote: > nit: This isn't really a platform property. Perhaps change it to use_... and > move it to the top or bottom. Done. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.h:28: bool has_vp9; On 2016/02/17 21:18:39, ddorwin wrote: > define default values? Done. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.h:73: void SetPlatformCodecInfoForTests(const PlatformCodecInfo& info); On 2016/02/17 21:18:39, ddorwin wrote: > Move inside #ifdef. Actually instead I removed all of the #ifdefs from here since this is code that can be tested on any platform; which makes it easier to debug and find issues. I think we should always strive for less-ifdefs when possible especially when it comes to testing. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.h:102: // codecs are supported. IsNotSupported is returned if at least one codec ID On 2016/02/17 21:18:39, ddorwin wrote: > To be absolutely clear: > ... supported in |mime_type_lower_case|. IsNotSupported is returned if > |mime_type_lower_case| is not supported or at least one is not supported in > |mime_type_lower_case|. Done. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.h:107: const std::string& mime_type_lower_case, On 2016/02/17 21:18:39, ddorwin wrote: > consistency nit: Elsewhere, the mime_type parameter is first. > > The fact that this function name is asking about "Codecs" makes it less clear > that this would be a better ordering, especially since we also have > |supported_codecs|, which is inconsistent with the others. Perhaps that should > be just before |is_encrypted|. > > Anyway, up to you. I intentionally left codecs first here since that's nominally what the method is about. Per your previous comment I've made |is_encrypted| the least important :) https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.h:124: // |allow_proprietary_codecs_| is set to false. |is_encrypted| means the codec On 2016/02/17 21:18:39, ddorwin wrote: > You dropped part of the text from the Note. It will return false for proprietary > codes regardless of platform codec support. Yes, I intentionally dropped that since the new wording is clearer. There's no need to talk about platforms if |allow_proprietary_codecs_|=false will always force it to return false :) https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.h:127: const std::string& mime_type_lower_case, On 2016/02/17 21:18:39, ddorwin wrote: > ditto on order consistency Acknowledged. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.h:148: const std::string& mime_type_lower_case, On 2016/02/17 21:18:39, ddorwin wrote: > ditto on order consistency Acknowledged. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... File media/base/mime_util_unittest.cc (right): https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:120: TEST(MimeUtilInternalTest, EncryptedCodecsFailWithoutPlatformSupport) { On 2016/02/17 21:18:40, ddorwin wrote: > These test names should include/refer to IsCodecSupportedOnAndroidForTests. i.e. > IsCodecSupportedOnAndroidForTests_EncryptedCodecsFailWithoutPlatformSupport, > though that is too long. Maybe drop "ForTests". Done. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:125: const bool kTestStates[] = {true, false}; On 2016/02/17 21:18:40, ddorwin wrote: > remove Done. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:137: ASSERT_FALSE(mime_util.IsCodecSupportedOnAndroidForTests( On 2016/02/17 21:18:39, ddorwin wrote: > EXPECT_ throughout. We want to run all cases even if one fails, right? Done, but can be obnoxious in local runs if you fail 2^5 test cases :) https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:138: static_cast<MimeUtil::Codec>(codec), "", true)); On 2016/02/17 21:18:40, ddorwin wrote: > It's interesting that the function allows an empty mime type string. Should that > be DCHECKed (and "foo" used here)? I don't think it should since only one portion of the code actually cares about this and that's verified below in the ogg / opus test. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:161: const MimeUtil::Codec codec_type = On 2016/02/17 21:18:40, ddorwin wrote: > nit: codec type sounds like there are types of codecs. Perhaps > s/codec/codec_number/ or s/codec_type/codec_value/. New format removes this. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:171: ASSERT_FALSE(mime_util.IsCodecSupportedOnAndroidForTests( On 2016/02/17 21:18:40, ddorwin wrote: > If one of these fails, it's going to be difficult to know which combination > failed. Perhaps we should add "<< PrintInfo(info)" and implement the print > function. Done. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:207: break; On 2016/02/17 21:18:39, ddorwin wrote: > ? Can we check the ifdef? We should at least call for this type to make sure it > doesn't DCHECK or crash. > > If !BUILDFLAG(ENABLE_HEVC_DEMUXING), it will always be false - that's something > we can check. If we wanted to, we could check the Android version in the other > case too. > > Another option would be to handle this similar to VP9. However, I'm not sure we > want to do that since it is not used in most cases. Done. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:216: TEST(MimeUtilInternalTest, ClearCodecBehaviorWithStandardPipeline) { On 2016/02/17 21:18:40, ddorwin wrote: > s/Standard/Android/? Done. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:218: PlatformCodecInfo info = {0}; On 2016/02/17 21:18:40, ddorwin wrote: > has_platform_decoder and has_vp8 are not set. (Currently, their value is > non-deterministic.) = {0} zero-initializes the structure, but that's removed now and the default values are false. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:265: break; On 2016/02/17 21:18:40, ddorwin wrote: > ditto Done. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:274: PlatformCodecInfo info = {0}; On 2016/02/17 21:18:40, ddorwin wrote: > Why aren't has_<codec> looped through? Done. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:317: break; On 2016/02/17 21:18:40, ddorwin wrote: > ditto, though this also requires has_platform_decoder Done. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:326: info.has_platform_decoder = false; On 2016/02/17 21:18:40, ddorwin wrote: > Should this matter? Can we loop through it? Done. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:327: info.has_opus = true; On 2016/02/17 21:18:40, ddorwin wrote: > Might as well loop throught his too? Done. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:329: const bool kTestStates[] = {true, false}; On 2016/02/17 21:18:40, ddorwin wrote: > remove Done. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:340: #endif On 2016/02/17 21:18:40, ddorwin wrote: > This is a long block. Add // defined(OS_ANDROID)? Done. https://codereview.chromium.org/1690063002/diff/170001/media/filters/stream_p... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/1690063002/diff/170001/media/filters/stream_p... media/filters/stream_parser_factory.cc:329: if (audio_codecs) On 2016/02/17 21:18:40, ddorwin wrote: > Note: This code is also inaccurate for when > IsUnifiedMediaPipelineEnabledForMse() and ffmpeg_branding!=Chrome*. Acknowledged. https://codereview.chromium.org/1690063002/diff/170001/media/filters/stream_p... media/filters/stream_parser_factory.cc:338: if (codec_info->tag == CodecInfo::HISTOGRAM_VP9 && On 2016/02/17 21:18:40, ddorwin wrote: > Do we need to check VP8 blacklisting as well? Again, good catch. Done.
The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1690063002/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1690063002/210001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Thanks. Getting closer. :) One refactoring suggestion. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... File media/base/mime_util_internal.h (right): https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_i... media/base/mime_util_internal.h:73: void SetPlatformCodecInfoForTests(const PlatformCodecInfo& info); On 2016/02/18 03:58:09, DaleCurtis wrote: > On 2016/02/17 21:18:39, ddorwin wrote: > > Move inside #ifdef. > > Actually instead I removed all of the #ifdefs from here since this is code that > can be tested on any platform; which makes it easier to debug and find issues. I > think we should always strive for less-ifdefs when possible especially when it > comes to testing. See my comment in the next PS. https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... File media/base/mime_util_unittest.cc (right): https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:138: static_cast<MimeUtil::Codec>(codec), "", true)); On 2016/02/18 03:58:10, DaleCurtis wrote: > On 2016/02/17 21:18:40, ddorwin wrote: > > It's interesting that the function allows an empty mime type string. Should > that > > be DCHECKed (and "foo" used here)? > > I don't think it should since only one portion of the code actually cares about > this and that's verified below in the ogg / opus test. Right, but isn't it a bug if the caller does not pass a valid string? https://codereview.chromium.org/1690063002/diff/170001/media/base/mime_util_u... media/base/mime_util_unittest.cc:218: PlatformCodecInfo info = {0}; On 2016/02/18 03:58:10, DaleCurtis wrote: > On 2016/02/17 21:18:40, ddorwin wrote: > > has_platform_decoder and has_vp8 are not set. (Currently, their value is > > non-deterministic.) > > = {0} zero-initializes the structure, but that's removed now and the default > values are false. Oops, I missed that. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.cc:276: : MediaCodecUtil::IsMediaCodecAvailable(); Why doesn't HasPlatformDecoderSupport() - namely the flags - apply to the old pipeline? If those flags are to do what they say (i.e. if a user selected them), even WMPA should be disabled. I understand that one case is there is no GPU process, and this would make some sense there, but that's not what the flags indicate to the user. Perhaps you really want the previous line to ask whether GPU process-based decoders are available. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.cc:277: platform_info_.supports_encrypted_vp8 = !MediaCodecUtil::IsVp8Blacklisted(); Yes, VP8 only applies in the encrypted case, but that seems overly specific and to expose implementation details to the caller of the function. Also, if MSE was to start using this, we would need more general information. Perhaps has_vp8_platform_decoder. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.cc:446: // If |codec_id| is not in |string_to_codec_map_|, then we assume that it is Does the new line below not allow us to indent these comments as intended (clang formatter unindented them)? https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.cc:556: case INVALID_CODEC: This should be first. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.cc:563: case PCM: WDYT of an empty line before this to avoid linking the comment to this specific block? https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.cc:612: return platform_info_.has_platform_decoders; This skips the API level check below. I think you need if (A && !B) return false; instead. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.cc:616: return base::android::BuildInfo::GetInstance()->sdk_int() >= 21; This line will only compile on Android. This is the only such line in this function. Either add guards or refactor to remove this check from the function. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.cc:629: // MediaPlayer can always play vp8. Note: This is incorrect for MSE, but nit: "VP8" for consistency. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... File media/base/mime_util_internal.h (right): https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.h:58: bool using_unified_media_pipeline = false; nit: We don't actually know whether the unified pipeline will be used because there are reasons we may fall back at load time. Thus, _enabled or something like that might be better. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.h:97: // supported in |mime_type_lower_case|. IsNotSupported is returned if Did you mean to drop the MayBeSupported sentence? https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.h:120: // |allow_proprietary_codecs_| is set to false. |is_encrypted| means the codec I still think you are missing a qualification in this sentence: "... false for proprietary codecs if..." https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.h:142: bool IsCodecSupportedOnAndroid(Codec codec, It is weird that we have multiple "OnAndroid" functions being built on all platforms. I understand the testing aspect, but it's probably just as much cognitive burden as the ifdefs. Also, platform_info_ is compiled on all platforms but never populated or used except on Android. Shouldn't it either be used for all platforms, which would add unnecessary complexity to most platforms, or not present at all? After looking at the implementation, I propose a hybrid: * Rename IsCodecSupportedOnAndroid IsCodecSupportedOnPlatform. * Add a const PlatformInfo& parameter. * Remove it from the class or make it static. * #ifdef platform_info_ so it is only present on Android - the only platform that populates or uses it. * Inside the Android guards in the .cc file, pass platform_info_ to IsCodecSupportedOnPlatform. * Remove SetPlatformInfoForTests as it is no longer necessary. Win-win-win: You get to test your code on all platforms, we don't have confusion on non-Android platforms, and we remove test-only function(s). Still, has_platform_decoders and using_unified_media_pipeline and the way they are used are very Android-specific. :( Perhaps the struct should be PlatformInfo. Note: PlatformInfo might need to be moved back outside the class. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... File media/base/mime_util_unittest.cc (right): https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:23: static std::vector<bool> CreateTestVector(bool use_default, s/use_default/check_all_values/? https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:24: bool initial_value) { s/initial_value/single_value/ or something like that? It's not really an initial value - it's the only value and only when the first param is false. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:30: // Helper method for running IsCodecSupportedOnAndroid() tests which will nit:s/which/that/ https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:35: // |initial_state|. ditto on "initial" https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:44: std::vector<bool> has_platform_decoders_states = Not that we should, but it seems that using a macro would be easier to read and less error-prone for these statements. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:104: return base::android::BuildInfo::GetInstance()->sdk_int() >= 21; Needs OS_ANDROID guard. Also, there are non-Android platforms that do support it, so #else return true; https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:242: // These codecs are never supported on Android. ... by the Android platform. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:245: case MimeUtil::INVALID_CODEC: This should be first. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:255: case MimeUtil::H264: video after audio https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:309: // These codecs are never supported on Android. ditto (at least similar) https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:312: case MimeUtil::INVALID_CODEC: ditto https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:321: // These codecs are always available. ... via MediaPlayer. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:322: case MimeUtil::H264: ditto
https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.cc:276: : MediaCodecUtil::IsMediaCodecAvailable(); On 2016/02/18 20:37:44, ddorwin wrote: > Why doesn't HasPlatformDecoderSupport() - namely the flags - apply to the old > pipeline? > > If those flags are to do what they say (i.e. if a user selected them), even WMPA > should be disabled. I understand that one case is there is no GPU process, and > this would make some sense there, but that's not what the flags indicate to the > user. > > Perhaps you really want the previous line to ask whether GPU process-based > decoders are available. As discussed via chat, comment added. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.cc:277: platform_info_.supports_encrypted_vp8 = !MediaCodecUtil::IsVp8Blacklisted(); On 2016/02/18 20:37:44, ddorwin wrote: > Yes, VP8 only applies in the encrypted case, but that seems overly specific and > to expose implementation details to the caller of the function. Also, if MSE was > to start using this, we would need more general information. > > Perhaps has_vp8_platform_decoder. Done. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.cc:446: // If |codec_id| is not in |string_to_codec_map_|, then we assume that it is On 2016/02/18 20:37:44, ddorwin wrote: > Does the new line below not allow us to indent these comments as intended (clang > formatter unindented them)? Apparently not. It does when I format a section manually, but git cl format relocates it. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.cc:556: case INVALID_CODEC: On 2016/02/18 20:37:44, ddorwin wrote: > This should be first. Done. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.cc:563: case PCM: On 2016/02/18 20:37:44, ddorwin wrote: > WDYT of an empty line before this to avoid linking the comment to this specific > block? Done. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.cc:612: return platform_info_.has_platform_decoders; On 2016/02/18 20:37:44, ddorwin wrote: > This skips the API level check below. I think you need if (A && !B) return > false; instead. Done. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.cc:616: return base::android::BuildInfo::GetInstance()->sdk_int() >= 21; On 2016/02/18 20:37:44, ddorwin wrote: > This line will only compile on Android. This is the only such line in this > function. Either add guards or refactor to remove this check from the function. Done. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.cc:629: // MediaPlayer can always play vp8. Note: This is incorrect for MSE, but On 2016/02/18 20:37:44, ddorwin wrote: > nit: "VP8" for consistency. Done. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... File media/base/mime_util_internal.h (right): https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.h:58: bool using_unified_media_pipeline = false; On 2016/02/18 20:37:45, ddorwin wrote: > nit: We don't actually know whether the unified pipeline will be used because > there are reasons we may fall back at load time. Thus, _enabled or something > like that might be better. Went with |is_unified_media_pipeline_enabled| https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.h:97: // supported in |mime_type_lower_case|. IsNotSupported is returned if On 2016/02/18 20:37:45, ddorwin wrote: > Did you mean to drop the MayBeSupported sentence? Nope, restored. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.h:120: // |allow_proprietary_codecs_| is set to false. |is_encrypted| means the codec On 2016/02/18 20:37:44, ddorwin wrote: > I still think you are missing a qualification in this sentence: "... false for > proprietary codecs if..." Ah, I see what you were saying. Done. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_i... media/base/mime_util_internal.h:142: bool IsCodecSupportedOnAndroid(Codec codec, On 2016/02/18 20:37:45, ddorwin wrote: > It is weird that we have multiple "OnAndroid" functions being built on all > platforms. I understand the testing aspect, but it's probably just as much > cognitive burden as the ifdefs. > > Also, platform_info_ is compiled on all platforms but never populated or used > except on Android. Shouldn't it either be used for all platforms, which would > add unnecessary complexity to most platforms, or not present at all? > > After looking at the implementation, I propose a hybrid: > * Rename IsCodecSupportedOnAndroid IsCodecSupportedOnPlatform. > * Add a const PlatformInfo& parameter. > * Remove it from the class or make it static. > * #ifdef platform_info_ so it is only present on Android - the only platform > that populates or uses it. > * Inside the Android guards in the .cc file, pass platform_info_ to > IsCodecSupportedOnPlatform. > * Remove SetPlatformInfoForTests as it is no longer necessary. > > Win-win-win: You get to test your code on all platforms, we don't have confusion > on non-Android platforms, and we remove test-only function(s). > > Still, has_platform_decoders and using_unified_media_pipeline and the way they > are used are very Android-specific. :( Perhaps the struct should be > PlatformInfo. > > Note: PlatformInfo might need to be moved back outside the class. I had a similar idea this morning. Done. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... File media/base/mime_util_unittest.cc (right): https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:23: static std::vector<bool> CreateTestVector(bool use_default, On 2016/02/18 20:37:45, ddorwin wrote: > s/use_default/check_all_values/? Done. |test_all_values| https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:24: bool initial_value) { On 2016/02/18 20:37:45, ddorwin wrote: > s/initial_value/single_value/ or something like that? It's not really an initial > value - it's the only value and only when the first param is false. Done. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:30: // Helper method for running IsCodecSupportedOnAndroid() tests which will On 2016/02/18 20:37:45, ddorwin wrote: > nit:s/which/that/ Done. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:35: // |initial_state|. On 2016/02/18 20:37:45, ddorwin wrote: > ditto on "initial" Went with |test_state| https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:44: std::vector<bool> has_platform_decoders_states = On 2016/02/18 20:37:45, ddorwin wrote: > Not that we should, but it seems that using a macro would be easier to read and > less error-prone for these statements. Helps a lot, done. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:104: return base::android::BuildInfo::GetInstance()->sdk_int() >= 21; On 2016/02/18 20:37:45, ddorwin wrote: > Needs OS_ANDROID guard. > Also, there are non-Android platforms that do support it, so #else return true; Done. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:242: // These codecs are never supported on Android. On 2016/02/18 20:37:45, ddorwin wrote: > ... by the Android platform. Done. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:245: case MimeUtil::INVALID_CODEC: On 2016/02/18 20:37:45, ddorwin wrote: > This should be first. Done. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:255: case MimeUtil::H264: On 2016/02/18 20:37:45, ddorwin wrote: > video after audio Done. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:309: // These codecs are never supported on Android. On 2016/02/18 20:37:45, ddorwin wrote: > ditto (at least similar) Done. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:312: case MimeUtil::INVALID_CODEC: On 2016/02/18 20:37:45, ddorwin wrote: > ditto Done. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:321: // These codecs are always available. On 2016/02/18 20:37:45, ddorwin wrote: > ... via MediaPlayer. Done. https://codereview.chromium.org/1690063002/diff/210001/media/base/mime_util_u... media/base/mime_util_unittest.cc:322: case MimeUtil::H264: On 2016/02/18 20:37:45, ddorwin wrote: > ditto Done.
dalecurtis@chromium.org changed reviewers: + sandersd@chromium.org
+sandersd to double check my logic changes in RenderFrameImpl. They seem correct :) nick@ I've also added a couple minor lines to RenderThreadImpl if you want to take another look.
The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1690063002/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1690063002/230001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
Whoops, inverted vp8 checks. Going to rename the method to IsVp8DecoderAvailable(). Will upload after the next round of comments.
LG other than logic inversion and a few comments. https://codereview.chromium.org/1690063002/diff/230001/media/base/android/med... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/1690063002/diff/230001/media/base/android/med... media/base/android/media_codec_util.cc:250: if (!IsMediaCodecAvailable()) This probably isn't relevant anymore. Don't call this if you don't have MediaCodec. You could DCHECK instead. https://codereview.chromium.org/1690063002/diff/230001/media/base/android/med... media/base/android/media_codec_util.cc:255: return Java_MediaCodecUtil_isDecoderSupportedForDevice(env, j_mime.obj()); Invert. https://codereview.chromium.org/1690063002/diff/230001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1690063002/diff/230001/media/base/mime_util_i... media/base/mime_util_internal.cc:507: // When MediaPlayer is used, h264 is always supported. For EME, MediaCodec too. https://codereview.chromium.org/1690063002/diff/230001/media/base/mime_util_u... File media/base/mime_util_unittest.cc (right): https://codereview.chromium.org/1690063002/diff/230001/media/base/mime_util_u... media/base/mime_util_unittest.cc:23: const char kTestMimeType[] = "foo/foo"; Suggest: // Type is ignored in nearly all cases. https://codereview.chromium.org/1690063002/diff/230001/media/base/mime_util_u... media/base/mime_util_unittest.cc:46: static void RunCodecSupportTest(const MimeUtil::PlatformInfo& test_state, state*s* https://codereview.chromium.org/1690063002/diff/230001/media/base/mime_util_u... media/base/mime_util_unittest.cc:47: const MimeUtil::PlatformInfo& states_to_vary, It's strange and potentially error-prone that the control is the second parameter and the optionally-used value is first, especially since this is the opposite of CreateTestVector(). https://codereview.chromium.org/1690063002/diff/230001/media/base/mime_util_u... media/base/mime_util_unittest.cc:256: EXPECT_FALSE(MimeUtil::IsCodecSupportedOnPlatform( Every call is identical for each test. Should we instead make a single call before the switch then compare the result at each location?
Description was changed from ========== Fix mime type mappings when the unified media pipeline is enabled. Enables codecs which are supported by the unified media pipeline when enabled. Removes unnecessary compile time restrictions on the codecs supported on Android. Additional notes: - Fixes VP8 codecs being included when they should be blacklisted. - Fixes MSE continuing to be disabled when supported by the unified pipeline. BUG=559236 TEST=new mime util tests. ========== to ========== Fix mime type mappings when the unified media pipeline is enabled. Enables codecs which are supported by the unified media pipeline when enabled. Removes unnecessary compile time restrictions on the codecs supported on Android. Additional notes: - Fixes VP8 codecs being included when they should be blacklisted. - Fixes VP9 codecs being included when the platform doesn't support them. - Fixes MSE continuing to be disabled when supported by the unified pipeline. BUG=559236 TEST=new mime util tests. ==========
Thanks for the review, this passes all tests locally! https://codereview.chromium.org/1690063002/diff/230001/media/base/android/med... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/1690063002/diff/230001/media/base/android/med... media/base/android/media_codec_util.cc:250: if (!IsMediaCodecAvailable()) On 2016/02/19 at 19:53:23, ddorwin wrote: > This probably isn't relevant anymore. Don't call this if you don't have MediaCodec. You could DCHECK instead. Perhaps, but the rest of the methods in the file do this, so I'd like to keep it for consistency. It also simplifies some of the checks elsewhere (e.g. StreamParserFactory). https://codereview.chromium.org/1690063002/diff/230001/media/base/android/med... media/base/android/media_codec_util.cc:255: return Java_MediaCodecUtil_isDecoderSupportedForDevice(env, j_mime.obj()); On 2016/02/19 at 19:53:23, ddorwin wrote: > Invert. Done. https://codereview.chromium.org/1690063002/diff/230001/media/base/mime_util_i... File media/base/mime_util_internal.cc (right): https://codereview.chromium.org/1690063002/diff/230001/media/base/mime_util_i... media/base/mime_util_internal.cc:507: // When MediaPlayer is used, h264 is always supported. On 2016/02/19 at 19:53:23, ddorwin wrote: > For EME, MediaCodec too. Done. https://codereview.chromium.org/1690063002/diff/230001/media/base/mime_util_u... File media/base/mime_util_unittest.cc (right): https://codereview.chromium.org/1690063002/diff/230001/media/base/mime_util_u... media/base/mime_util_unittest.cc:23: const char kTestMimeType[] = "foo/foo"; On 2016/02/19 at 19:53:23, ddorwin wrote: > Suggest: > // Type is ignored in nearly all cases. Done with some further elaboration. https://codereview.chromium.org/1690063002/diff/230001/media/base/mime_util_u... media/base/mime_util_unittest.cc:46: static void RunCodecSupportTest(const MimeUtil::PlatformInfo& test_state, On 2016/02/19 at 19:53:23, ddorwin wrote: > state*s* Done. https://codereview.chromium.org/1690063002/diff/230001/media/base/mime_util_u... media/base/mime_util_unittest.cc:47: const MimeUtil::PlatformInfo& states_to_vary, On 2016/02/19 at 19:53:23, ddorwin wrote: > It's strange and potentially error-prone that the control is the second parameter and the optionally-used value is first, especially since this is the opposite of CreateTestVector(). Flipped. https://codereview.chromium.org/1690063002/diff/230001/media/base/mime_util_u... media/base/mime_util_unittest.cc:256: EXPECT_FALSE(MimeUtil::IsCodecSupportedOnPlatform( On 2016/02/19 at 19:53:23, ddorwin wrote: > Every call is identical for each test. Should we instead make a single call before the switch then compare the result at each location? Good suggestion, done!
This is how I feel about this patch: http://i.imgur.com/k0p1C2q.gif
On 2016/02/19 at 20:29:15, DaleCurtis wrote: > This is how I feel about this patch: http://i.imgur.com/k0p1C2q.gif Wait that was a .gif, need to use .gifv!! http://i.imgur.com/k0p1C2q.gifv
I don't have a gifv, but 👍. LGTM. Thank you! https://codereview.chromium.org/1690063002/diff/230001/media/base/android/med... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/1690063002/diff/230001/media/base/android/med... media/base/android/media_codec_util.cc:250: if (!IsMediaCodecAvailable()) On 2016/02/19 20:28:35, DaleCurtis wrote: > On 2016/02/19 at 19:53:23, ddorwin wrote: > > This probably isn't relevant anymore. Don't call this if you don't have > MediaCodec. You could DCHECK instead. > > Perhaps, but the rest of the methods in the file do this, so I'd like to keep it > for consistency. It also simplifies some of the checks elsewhere (e.g. > StreamParserFactory). It makes more sense now that we're not asking about the blacklist. https://codereview.chromium.org/1690063002/diff/250001/media/base/mime_util_u... File media/base/mime_util_unittest.cc (right): https://codereview.chromium.org/1690063002/diff/250001/media/base/mime_util_u... media/base/mime_util_unittest.cc:66: size_t name##_index = 0; \ The contents of the macro are less readable than the previous for loop, but I guess that's necessary to put the '{' outside the macro. LG.
On 2016/02/19 20:29:59, DaleCurtis wrote: > On 2016/02/19 at 20:29:15, DaleCurtis wrote: > > This is how I feel about this patch: http://i.imgur.com/k0p1C2q.gif > > Wait that was a .gif, need to use .gifv!! http://i.imgur.com/k0p1C2q.gifv Don't you mean http://i.imgur.com/k0p1C2q.mp4 ?
The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1690063002/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1690063002/250001
https://codereview.chromium.org/1690063002/diff/250001/media/base/media.cc File media/base/media.cc (right): https://codereview.chromium.org/1690063002/diff/250001/media/base/media.cc#ne... media/base/media.cc:111: return IsUnifiedMediaPipelineEnabled() || We should probably rely on the command line flag until EME is fully supported.
https://codereview.chromium.org/1690063002/diff/250001/media/base/media.cc File media/base/media.cc (right): https://codereview.chromium.org/1690063002/diff/250001/media/base/media.cc#ne... media/base/media.cc:111: return IsUnifiedMediaPipelineEnabled() || On 2016/02/19 at 21:07:12, sandersd wrote: > We should probably rely on the command line flag until EME is fully supported. Good catch. This was what my comment last Friday was trying to tell me :)
The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1690063002/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1690063002/270001
Description was changed from ========== Fix mime type mappings when the unified media pipeline is enabled. Enables codecs which are supported by the unified media pipeline when enabled. Removes unnecessary compile time restrictions on the codecs supported on Android. Additional notes: - Fixes VP8 codecs being included when they should be blacklisted. - Fixes VP9 codecs being included when the platform doesn't support them. - Fixes MSE continuing to be disabled when supported by the unified pipeline. BUG=559236 TEST=new mime util tests. ========== to ========== Fix mime type mappings when the unified media pipeline is enabled. Enables codecs which are supported by the unified media pipeline when enabled. Removes unnecessary compile time restrictions on the codecs supported on Android. Additional notes: - Fixes VP8 codecs being included when they should be blacklisted. - Fixes VP9 codecs being included when the platform doesn't support them. - Fixes MSE continuing to be disabled when supported by the unified pipeline. BUG=559236,582548 TEST=new mime util tests. ==========
The CQ bit was checked by dalecurtis@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1690063002/290001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1690063002/290001
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dalecurtis@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org, ddorwin@chromium.org Link to the patchset: https://codereview.chromium.org/1690063002/#ps290001 (title: "Fix compile error.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1690063002/290001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1690063002/290001
Message was sent while issue was closed.
Committed patchset #10 (id:290001)
Message was sent while issue was closed.
Description was changed from ========== Fix mime type mappings when the unified media pipeline is enabled. Enables codecs which are supported by the unified media pipeline when enabled. Removes unnecessary compile time restrictions on the codecs supported on Android. Additional notes: - Fixes VP8 codecs being included when they should be blacklisted. - Fixes VP9 codecs being included when the platform doesn't support them. - Fixes MSE continuing to be disabled when supported by the unified pipeline. BUG=559236,582548 TEST=new mime util tests. ========== to ========== Fix mime type mappings when the unified media pipeline is enabled. Enables codecs which are supported by the unified media pipeline when enabled. Removes unnecessary compile time restrictions on the codecs supported on Android. Additional notes: - Fixes VP8 codecs being included when they should be blacklisted. - Fixes VP9 codecs being included when the platform doesn't support them. - Fixes MSE continuing to be disabled when supported by the unified pipeline. BUG=559236,582548 TEST=new mime util tests. Committed: https://crrev.com/88af393b8b7cc535978f95f32e8c83b5e83f4ebc Cr-Commit-Position: refs/heads/master@{#376571} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/88af393b8b7cc535978f95f32e8c83b5e83f4ebc Cr-Commit-Position: refs/heads/master@{#376571} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
