Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
648613003:
Add support for color formats negotiation. (Closed)
|
Created:
6 years, 2 months ago by changbin Modified:
6 years, 2 months ago CC:
chromium-reviews, posciak+watch_chromium.org, avayvod+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd support for color formats negotiation.
The color format parameter is hard coded for AndroidVideoEncodeAccelerator, we'd
better add negotiation with the codec for supported color formats.
BUG=423602
Committed: https://crrev.com/3b7fe737be7b85badc8829bca2e3806c0f7a2a5f
Cr-Commit-Position: refs/heads/master@{#300651}
Patch Set 1 #
Total comments: 12
Patch Set 2 : Update per mcasas's comments. #
Total comments: 7
Patch Set 3 : Update #Patch Set 4 : update. #
Total comments: 2
Patch Set 5 : Remove COLOR_FORMAT_UNKNOWN. #
Total comments: 2
Patch Set 6 : Update per Pawel's comment, ptal. #
Messages
Total messages: 22 (7 generated)
changbin.shao@intel.com changed reviewers: + posciak@chromium.org, xhwang@chromium.org
Hi, please take a look. thanks!
mcasas@chromium.org changed reviewers: + mcasas@chromium.org
Drive-by. https://codereview.chromium.org/648613003/diff/1/content/common/gpu/media/and... File content/common/gpu/media/android_video_encode_accelerator.cc (right): https://codereview.chromium.org/648613003/diff/1/content/common/gpu/media/and... content/common/gpu/media/android_video_encode_accelerator.cc:32: enum ColorFormat { Please consider calling this PixelFormat. https://codereview.chromium.org/648613003/diff/1/content/common/gpu/media/and... content/common/gpu/media/android_video_encode_accelerator.cc:74: ColorFormat GetSupportedColorFormatForMime(const std::string& mime) { static https://codereview.chromium.org/648613003/diff/1/content/common/gpu/media/and... content/common/gpu/media/android_video_encode_accelerator.cc:76: for (std::vector<int>::iterator it = formats.begin(); it != formats.end(); Please use range-based for loop and const auto [1]. [1] http://chromium-cpp.appspot.com/ https://codereview.chromium.org/648613003/diff/1/content/common/gpu/media/and... content/common/gpu/media/android_video_encode_accelerator.cc:81: nit: remove empty line. https://codereview.chromium.org/648613003/diff/1/media/base/android/java/src/... File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java (right): https://codereview.chromium.org/648613003/diff/1/media/base/android/java/src/... media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:215: for (int i = 0; i < count; ++i) { Range-based for loop? Same below on l.222 https://codereview.chromium.org/648613003/diff/1/media/base/android/java/src/... media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:217: if (!info.isEncoder()) { Put in a single line?
@msasas, thanks for your review. Patch is updated, ptal. https://codereview.chromium.org/648613003/diff/1/content/common/gpu/media/and... File content/common/gpu/media/android_video_encode_accelerator.cc (right): https://codereview.chromium.org/648613003/diff/1/content/common/gpu/media/and... content/common/gpu/media/android_video_encode_accelerator.cc:32: enum ColorFormat { On 2014/10/10 08:16:16, mcasas wrote: > Please consider calling this PixelFormat. Done. https://codereview.chromium.org/648613003/diff/1/content/common/gpu/media/and... content/common/gpu/media/android_video_encode_accelerator.cc:74: ColorFormat GetSupportedColorFormatForMime(const std::string& mime) { On 2014/10/10 08:16:16, mcasas wrote: > static Done. https://codereview.chromium.org/648613003/diff/1/content/common/gpu/media/and... content/common/gpu/media/android_video_encode_accelerator.cc:76: for (std::vector<int>::iterator it = formats.begin(); it != formats.end(); Thanks for suggestion. Looks the loop can be removed here, my fault. On 2014/10/10 08:16:16, mcasas wrote: > Please use range-based for loop and const auto [1]. > > [1] http://chromium-cpp.appspot.com/ https://codereview.chromium.org/648613003/diff/1/content/common/gpu/media/and... content/common/gpu/media/android_video_encode_accelerator.cc:81: On 2014/10/10 08:16:16, mcasas wrote: > nit: remove empty line. Done. https://codereview.chromium.org/648613003/diff/1/media/base/android/java/src/... File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java (right): https://codereview.chromium.org/648613003/diff/1/media/base/android/java/src/... media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:215: for (int i = 0; i < count; ++i) { It's java code. We may use 'for each' loop, while i prefer not to change here in order to align with other parts of code of this file. On 2014/10/10 08:16:16, mcasas wrote: > Range-based for loop? Same below on l.222 https://codereview.chromium.org/648613003/diff/1/media/base/android/java/src/... media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:217: if (!info.isEncoder()) { On 2014/10/10 08:16:16, mcasas wrote: > Put in a single line? Done.
Hi all, any review comments for this patch? Thanks!
Generally looking pretty good. I only have a few comments. Can you file a bug for this? https://codereview.chromium.org/648613003/diff/110001/content/common/gpu/medi... File content/common/gpu/media/android_video_encode_accelerator.cc (right): https://codereview.chromium.org/648613003/diff/110001/content/common/gpu/medi... content/common/gpu/media/android_video_encode_accelerator.cc:77: formats.end()) nit: Seems like GetEncoderColorFormats() should return a "set" instead of a "vector". You can do it easily in GetEncoderColorFormats() by return std::set(formats.begin(), formats.end()); Then the find() here would be super simple. https://codereview.chromium.org/648613003/diff/110001/content/common/gpu/medi... content/common/gpu/media/android_video_encode_accelerator.cc:79: return COLOR_FORMAT_YUV420_SEMIPLANAR; Is COLOR_FORMAT_YUV420_SEMIPLANAR always supported? If so, is it part of |formats| here? If so, check it? https://codereview.chromium.org/648613003/diff/110001/media/base/android/java... File media/base/android/java/src/org/chromium/media/MediaCodecBridge.java (right): https://codereview.chromium.org/648613003/diff/110001/media/base/android/java... media/base/android/java/src/org/chromium/media/MediaCodecBridge.java:222: if (supportedTypes[j].equalsIgnoreCase(mime)) { nit: we like to return early: if (!...) continue; // handle normal case. https://codereview.chromium.org/648613003/diff/110001/media/base/android/medi... File media/base/android/media_codec_bridge.h (right): https://codereview.chromium.org/648613003/diff/110001/media/base/android/medi... media/base/android/media_codec_bridge.h:84: // Get a list of encoder supported color formats for specified mime type. nit: |mime_type| Can you add comments about how an "int" is mapped to a color format? For example, refer to MediaCodecInfo.CodecCapabilities...
xhwang@, thanks a lot for your review. ptal again:) https://codereview.chromium.org/648613003/diff/110001/content/common/gpu/medi... File content/common/gpu/media/android_video_encode_accelerator.cc (right): https://codereview.chromium.org/648613003/diff/110001/content/common/gpu/medi... content/common/gpu/media/android_video_encode_accelerator.cc:77: formats.end()) On 2014/10/13 17:37:35, xhwang wrote: > nit: Seems like GetEncoderColorFormats() should return a "set" instead of a > "vector". You can do it easily in GetEncoderColorFormats() by > > return std::set(formats.begin(), formats.end()); > > Then the find() here would be super simple. Done. https://codereview.chromium.org/648613003/diff/110001/content/common/gpu/medi... content/common/gpu/media/android_video_encode_accelerator.cc:79: return COLOR_FORMAT_YUV420_SEMIPLANAR; On 2014/10/13 17:37:35, xhwang wrote: > Is COLOR_FORMAT_YUV420_SEMIPLANAR always supported? If so, is it part of > |formats| here? If so, check it? Thanks for pointing this out:) I think COLOR_FORMAT_YUV420_SEMIPLANAR isn't always supported, i.e, omx.MTK. The new logic of this function would be: if the MediaCodecInfo.CodecCapabilities contains COLOR_FORMAT_YUV420_SEMIPLANAR or COLOR_FORMAT_YUV420_PLANAR , then they are the chosen color format. Otherwise we consider it unknown color format and won't go through the VEA path. An uncertainty of this change is whether it can return COLOR_FORMAT_YUV420_SEMIPLANAR for previous supported device, i guess Google Nexus? See it gets MediaCodecInfo first per mime type, whereas the MediaCodecInfo could be null for vp8 mime. https://codereview.chromium.org/648613003/diff/110001/media/base/android/medi... File media/base/android/media_codec_bridge.h (right): https://codereview.chromium.org/648613003/diff/110001/media/base/android/medi... media/base/android/media_codec_bridge.h:84: // Get a list of encoder supported color formats for specified mime type. On 2014/10/13 17:37:35, xhwang wrote: > nit: |mime_type| > > Can you add comments about how an "int" is mapped to a color format? For > example, refer to MediaCodecInfo.CodecCapabilities... Done.
lgtm % one more comment. Please make sure you get Pawel's review before landing. https://codereview.chromium.org/648613003/diff/210001/content/common/gpu/medi... File content/common/gpu/media/android_video_encode_accelerator.cc (right): https://codereview.chromium.org/648613003/diff/210001/content/common/gpu/medi... content/common/gpu/media/android_video_encode_accelerator.cc:35: COLOR_FORMAT_UNKNOWN, This is confusing because value "22" is a valid one in: http://developer.android.com/reference/android/media/MediaCodecInfo.CodecCapa... How about let GetSupportedColorFormatForMime return a boolean, and provide an output param for PixelFormat?
Thanks a lot for your review, xhwang@! Hi Pawel, could you owner review this change? thanks! https://codereview.chromium.org/648613003/diff/210001/content/common/gpu/medi... File content/common/gpu/media/android_video_encode_accelerator.cc (right): https://codereview.chromium.org/648613003/diff/210001/content/common/gpu/medi... content/common/gpu/media/android_video_encode_accelerator.cc:35: COLOR_FORMAT_UNKNOWN, On 2014/10/15 04:15:45, xhwang wrote: > This is confusing because value "22" is a valid one in: > > http://developer.android.com/reference/android/media/MediaCodecInfo.CodecCapa... > > How about let GetSupportedColorFormatForMime return a boolean, and provide an > output param for PixelFormat? Done.
https://chromiumcodereview.appspot.com/648613003/diff/230001/content/common/g... File content/common/gpu/media/android_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/648613003/diff/230001/content/common/g... content/common/gpu/media/android_video_encode_accelerator.cc:82: : COLOR_FORMAT_YUV420_PLANAR; Perhaps: if (formats.count(COLOR_FORMAT_YUV420_SEMIPLANAR) > 0 *pixel_format = COLOR_FORMAT_YUV420_SEMIPLANAR; else if (formats.count(COLOR_FORMAT_YUV420_PLANAR) > 0) *pixel_format = COLOR_FORMAT_YUV420_PLANAR; else return false; return true;
Pawel@, thanks for your comments. ptal again. https://codereview.chromium.org/648613003/diff/230001/content/common/gpu/medi... File content/common/gpu/media/android_video_encode_accelerator.cc (right): https://codereview.chromium.org/648613003/diff/230001/content/common/gpu/medi... content/common/gpu/media/android_video_encode_accelerator.cc:82: : COLOR_FORMAT_YUV420_PLANAR; On 2014/10/20 08:10:24, Pawel Osciak wrote: > Perhaps: > > if (formats.count(COLOR_FORMAT_YUV420_SEMIPLANAR) > 0 > *pixel_format = COLOR_FORMAT_YUV420_SEMIPLANAR; > else if (formats.count(COLOR_FORMAT_YUV420_PLANAR) > 0) > *pixel_format = COLOR_FORMAT_YUV420_PLANAR; > else > return false; > > return true; Done.
On 2014/10/20 08:10:25, Pawel Osciak wrote: > https://chromiumcodereview.appspot.com/648613003/diff/230001/content/common/g... > File content/common/gpu/media/android_video_encode_accelerator.cc (right): > > https://chromiumcodereview.appspot.com/648613003/diff/230001/content/common/g... > content/common/gpu/media/android_video_encode_accelerator.cc:82: : > COLOR_FORMAT_YUV420_PLANAR; > Perhaps: > > if (formats.count(COLOR_FORMAT_YUV420_SEMIPLANAR) > 0 > *pixel_format = COLOR_FORMAT_YUV420_SEMIPLANAR; > else if (formats.count(COLOR_FORMAT_YUV420_PLANAR) > 0) > *pixel_format = COLOR_FORMAT_YUV420_PLANAR; > else > return false; > > return true; Hi, any comments for this CL? thanks!
The CQ bit was checked by changbin.shao@intel.com
The CQ bit was unchecked by changbin.shao@intel.com
The CQ bit was checked by changbin.shao@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/648613003/250001
The CQ bit was unchecked by changbin.shao@intel.com
The CQ bit was checked by changbin.shao@intel.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/648613003/250001
Message was sent while issue was closed.
Committed patchset #6 (id:250001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3b7fe737be7b85badc8829bca2e3806c0f7a2a5f Cr-Commit-Position: refs/heads/master@{#300651} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
