Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1897003002:
Fix a bug that mime type isn't passed when checking Codec capabilities (Closed)
|
Created:
4 years, 8 months ago by qinmin Modified:
4 years, 8 months ago CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, avayvod+watch_chromium.org, mlamouri+watch-media_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix a bug that mime type isn't passed when checking Codec capabilities
Chrome is passing wrong MIME type when checking vp8/9 codec capability.
We should use video/x-vnd.on2.vp8, but using vp8 instead.
TBR=timav@chromium.org
BUG=603162
Committed: https://crrev.com/9bb6f369e41be690806ff02e69b8a1dc9b83cccb
Cr-Commit-Position: refs/heads/master@{#388947}
Patch Set 1 #
Total comments: 9
Patch Set 2 : nits #
Total comments: 24
Patch Set 3 : addressing comments #
Total comments: 2
Patch Set 4 : nits #Messages
Total messages: 24 (10 generated)
qinmin@chromium.org changed reviewers: + timav@chromium.org
PTAL
The CQ bit was checked by qinmin@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/1897003002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1897003002/1
Drive-by comments/questions about how we might avoid similar issues in the future. https://codereview.chromium.org/1897003002/diff/1/media/base/android/media_co... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/1897003002/diff/1/media/base/android/media_co... media/base/android/media_codec_util.cc:44: return std::string(); Should there be a NOTREACHED() here or at least a DLOG(WARNING)? https://codereview.chromium.org/1897003002/diff/1/media/base/android/media_co... media/base/android/media_codec_util.cc:57: static bool IsDecoderSupportedByDevice(const std::string& mime_type) { Would it be safer to have this function (or even the Java code) handle such conversion? I guess we want devs to use |mime_type| consistently and know what they are doing, though. Perhaps this and similar functions should DCHECK() on a funciton that checks that the value is one of those known in CodecTypeToAndroidMimeType(). It might also help to rename the variable to |android_mime_type| and perhaps reinforce this in a comment. https://codereview.chromium.org/1897003002/diff/1/media/base/android/media_co... media/base/android/media_codec_util.cc:119: bool MediaCodecUtil::IsKnownUnaccelerated(const std::string& mime_type, |android_mime_type|? https://codereview.chromium.org/1897003002/diff/1/media/base/android/media_co... media/base/android/media_codec_util.cc:134: if (mime_type == "video/x-vnd.on2.vp8") Should we make these constants. This one appears twice (and they could appear more if my suggestion above is followed). Also, it might be nice to have all such strings in one place.
Description was changed from ========== Fix a bug that mime type isn't passed when checking Codec capabilities Chrome is passing wrong MIME type when checking vp8/9 codec capability. We should use video/x-vnd.on2.vp8, but using vp8 instead. BUG=603162 ========== to ========== Fix a bug that mime type isn't passed when checking Codec capabilities Chrome is passing wrong MIME type when checking vp8/9 codec capability. We should use video/x-vnd.on2.vp8, but using vp8 instead. TBR=timav@chromium.org BUG=603162 ==========
https://codereview.chromium.org/1897003002/diff/1/media/base/android/media_co... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/1897003002/diff/1/media/base/android/media_co... media/base/android/media_codec_util.cc:44: return std::string(); On 2016/04/18 20:30:44, ddorwin wrote: > Should there be a NOTREACHED() here or at least a DLOG(WARNING)? Done. https://codereview.chromium.org/1897003002/diff/1/media/base/android/media_co... media/base/android/media_codec_util.cc:57: static bool IsDecoderSupportedByDevice(const std::string& mime_type) { On 2016/04/18 20:30:44, ddorwin wrote: > Would it be safer to have this function (or even the Java code) handle such > conversion? I guess we want devs to use |mime_type| consistently and know what > they are doing, though. > Perhaps this and similar functions should DCHECK() on a funciton that checks > that the value is one of those known in CodecTypeToAndroidMimeType(). > It might also help to rename the variable to |android_mime_type| and perhaps > reinforce this in a comment. Done. https://codereview.chromium.org/1897003002/diff/1/media/base/android/media_co... media/base/android/media_codec_util.cc:119: bool MediaCodecUtil::IsKnownUnaccelerated(const std::string& mime_type, On 2016/04/18 20:30:44, ddorwin wrote: > |android_mime_type|? Done. https://codereview.chromium.org/1897003002/diff/1/media/base/android/media_co... media/base/android/media_codec_util.cc:134: if (mime_type == "video/x-vnd.on2.vp8") On 2016/04/18 20:30:44, ddorwin wrote: > Should we make these constants. This one appears twice (and they could appear > more if my suggestion above is followed). Also, it might be nice to have all > such strings in one place. Done.
The CQ bit was checked by qinmin@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/1897003002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1897003002/20001
ddorwin@chromium.org changed reviewers: + ddorwin@chromium.org
Thanks. Looks good. Minor stuff. https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:26: namespace { nit: empty line at beginning and end of namespaces. https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:27: const char kMP4AMimeType[] = "audio/mp4a-latm"; I think we generally do kMp4a... (The MP4 examples don't support this, but I believe that would be correct. Vp9 is definitely done more that way. ditto for Avc, Vp8, Vp9 and the others I commented below. https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:28: const char kOPUSMimeType[] = "audio/opus"; Opus https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:29: const char kVORBISMimeType[] = "audio/vorbis"; Vorbis https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:31: const char kHVCMimeType[] = "video/hevc"; kHevc... I haven't seen HVC used as an acronym. https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:36: namespace media { nit: Put the anonymous namespace in this one. No impact here, but it's generally the preferred pattern I believe. https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:58: static bool IsMimeTypeSupported(const std::string& mime_type) { |android_mime_type| https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:59: std::vector<std::string> supported{ nit: space before '{' ? https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:61: kHVCMimeType, kVP8MimeType, kVP9MimeType}; Note: Initializer lists are in the process of being allowed [1]. Since this is Android only, this seems fine. [1] https://groups.google.com/a/chromium.org/forum/#!searchin/cxx/Initialization$... https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:62: return std::find(supported.begin(), supported.end(), mime_type) != This is a bit simpler if you use a base::hash_set above, though that might be more size and complexity in implementation. Up to you. https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:145: DCHECK(IsMimeTypeSupported(android_mime_type)); We can still do debug checks of input even regardless of 142. IOW, make this the first line. https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:156: if (android_mime_type.compare(kVP8MimeType) == 0) Is there a reason you changed from just using ==?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1897003002/diff/1/media/base/android/media_co... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/1897003002/diff/1/media/base/android/media_co... media/base/android/media_codec_util.cc:181: return IsMediaCodecAvailable() && I would have just replaced "vp8" and "vp9" down there with proper constants "video/x-vnd.on2.vp8" and "video/x-vnd.on2.vp9". lgtm
https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:26: namespace { On 2016/04/18 22:09:09, ddorwin wrote: > nit: empty line at beginning and end of namespaces. Done. https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:27: const char kMP4AMimeType[] = "audio/mp4a-latm"; On 2016/04/18 22:09:09, ddorwin wrote: > I think we generally do kMp4a... (The MP4 examples don't support this, but I > believe that would be correct. Vp9 is definitely done more that way. > > ditto for Avc, Vp8, Vp9 and the others I commented below. Done. https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:28: const char kOPUSMimeType[] = "audio/opus"; On 2016/04/18 22:09:09, ddorwin wrote: > Opus Done. https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:29: const char kVORBISMimeType[] = "audio/vorbis"; On 2016/04/18 22:09:08, ddorwin wrote: > Vorbis Done. https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:31: const char kHVCMimeType[] = "video/hevc"; On 2016/04/18 22:09:09, ddorwin wrote: > kHevc... I haven't seen HVC used as an acronym. Done. https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:36: namespace media { On 2016/04/18 22:09:09, ddorwin wrote: > nit: Put the anonymous namespace in this one. No impact here, but it's generally > the preferred pattern I believe. Done. https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:58: static bool IsMimeTypeSupported(const std::string& mime_type) { On 2016/04/18 22:09:09, ddorwin wrote: > |android_mime_type| Done. However, this function should checks whether the mime_type is an android supported mime_type, so using mime_type is more proper. https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:59: std::vector<std::string> supported{ On 2016/04/18 22:09:09, ddorwin wrote: > nit: space before '{' ? I added the space initially, but "git cl media format" removed that space on purpose. https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:61: kHVCMimeType, kVP8MimeType, kVP9MimeType}; On 2016/04/18 22:09:09, ddorwin wrote: > Note: Initializer lists are in the process of being allowed [1]. Since this is > Android only, this seems fine. > > [1] > https://groups.google.com/a/chromium.org/forum/#!searchin/cxx/Initialization$... good to know, thanks https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:62: return std::find(supported.begin(), supported.end(), mime_type) != On 2016/04/18 22:09:09, ddorwin wrote: > This is a bit simpler if you use a base::hash_set above, though that might be > more size and complexity in implementation. Up to you. Since this is only used in DCHECK, i will leave this as it is now. https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:145: DCHECK(IsMimeTypeSupported(android_mime_type)); On 2016/04/18 22:09:09, ddorwin wrote: > We can still do debug checks of input even regardless of 142. IOW, make this the > first line. Done. https://codereview.chromium.org/1897003002/diff/20001/media/base/android/medi... media/base/android/media_codec_util.cc:156: if (android_mime_type.compare(kVP8MimeType) == 0) On 2016/04/18 22:09:09, ddorwin wrote: > Is there a reason you changed from just using ==? They are essentially the same, personal preference thing. Anyway, reverted to use ==.
LGTM with renaming suggestion. Thanks. https://codereview.chromium.org/1897003002/diff/40001/media/base/android/medi... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/1897003002/diff/40001/media/base/android/medi... media/base/android/media_codec_util.cc:60: static bool IsMimeTypeSupported(const std::string& android_mime_type) { Since the point is to confirm that this is a supported Android MIME type, maybe we should rename to IsSupportedAndroidMimeType. This also may address your comment in the previous PS that the value passed in could be any MIME type. IOW, you could drop "android_" from the parameter name.
https://codereview.chromium.org/1897003002/diff/40001/media/base/android/medi... File media/base/android/media_codec_util.cc (right): https://codereview.chromium.org/1897003002/diff/40001/media/base/android/medi... media/base/android/media_codec_util.cc:60: static bool IsMimeTypeSupported(const std::string& android_mime_type) { On 2016/04/19 23:54:22, ddorwin wrote: > Since the point is to confirm that this is a supported Android MIME type, maybe > we should rename to IsSupportedAndroidMimeType. > > This also may address your comment in the previous PS that the value passed in > could be any MIME type. IOW, you could drop "android_" from the parameter name. Done.
The CQ bit was checked by qinmin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from timav@chromium.org, ddorwin@chromium.org Link to the patchset: https://codereview.chromium.org/1897003002/#ps60001 (title: "nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1897003002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1897003002/60001
Message was sent while issue was closed.
Description was changed from ========== Fix a bug that mime type isn't passed when checking Codec capabilities Chrome is passing wrong MIME type when checking vp8/9 codec capability. We should use video/x-vnd.on2.vp8, but using vp8 instead. TBR=timav@chromium.org BUG=603162 ========== to ========== Fix a bug that mime type isn't passed when checking Codec capabilities Chrome is passing wrong MIME type when checking vp8/9 codec capability. We should use video/x-vnd.on2.vp8, but using vp8 instead. TBR=timav@chromium.org BUG=603162 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix a bug that mime type isn't passed when checking Codec capabilities Chrome is passing wrong MIME type when checking vp8/9 codec capability. We should use video/x-vnd.on2.vp8, but using vp8 instead. TBR=timav@chromium.org BUG=603162 ========== to ========== Fix a bug that mime type isn't passed when checking Codec capabilities Chrome is passing wrong MIME type when checking vp8/9 codec capability. We should use video/x-vnd.on2.vp8, but using vp8 instead. TBR=timav@chromium.org BUG=603162 Committed: https://crrev.com/9bb6f369e41be690806ff02e69b8a1dc9b83cccb Cr-Commit-Position: refs/heads/master@{#388947} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/9bb6f369e41be690806ff02e69b8a1dc9b83cccb Cr-Commit-Position: refs/heads/master@{#388947} |
