Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2260123002:
V4L2VDA: use YV12 as output format if processor supports it. (Closed)
|
Created:
4 years, 4 months ago by wuchengli Modified:
4 years, 4 months ago CC:
chromium-reviews, posciak+watch_chromium.org, piman+watch_chromium.org, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionV4L2VDA: use YV12 as output format if processor supports it.
This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video
output format. ArcGpuVideoDecodeAccelerator only supports single
physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M.
We also prefer YVU420 over NV12 because chrome rendering supports
only YV12.
BUG=chrome-os-partner:56354
TEST=Run VDA unittest and play video on oak.
Committed: https://crrev.com/8dfde51b86938d8e64941cd584dfe736c14e6005
Cr-Commit-Position: refs/heads/master@{#413703}
Patch Set 1 #
Total comments: 4
Patch Set 2 : address kcwu's comment #
Total comments: 4
Patch Set 3 : address kcwu's comments #Patch Set 4 : fix a typo. s/NV21M/NV12M/ #Patch Set 5 : address Pawel's offline comment. Prefer YVU420>NV12>others #
Total comments: 4
Patch Set 6 : address kcwu's comment #Patch Set 7 : improve comments #
Total comments: 4
Patch Set 8 : address Pawel's comments #
Messages
Total messages: 38 (24 generated)
Description was changed from ========== V4L2VDA: use YV12 as output format if processor supports it. BUG=chrome-os-partner: TEST=Run VDA unittest and play video on oak. ========== to ========== V4L2VDA: use YV12 as output format if processor supports it. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ==========
wuchengli@chromium.org changed reviewers: + kcwu@chromium.org, posciak@chromium.org
PTAL.
https://codereview.chromium.org/2260123002/diff/1/media/gpu/v4l2_video_decode... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2260123002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:2044: image_processor.GetSupportedOutputFormats(); I'm wondering what if we simply push_front preferred format to processor_output_formats. Or move preferred format to the front. For example, (psuedo code) it = find(...); if (it != end) { swap(it, begin); } https://codereview.chromium.org/2260123002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:2051: if (it != processor_output_formats.end()) { swap these two if-conditions at line 2051 and 2047. It doesn't make sense to query device first and then found the format are not what we want.
https://codereview.chromium.org/2260123002/diff/1/media/gpu/v4l2_video_decode... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2260123002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:2044: image_processor.GetSupportedOutputFormats(); On 2016/08/19 05:50:59, kcwu wrote: > I'm wondering what if we simply push_front preferred format to > processor_output_formats. > > Or move preferred format to the front. For example, > (psuedo code) > it = find(...); > if (it != end) { > swap(it, begin); > } Done. Good idea. https://codereview.chromium.org/2260123002/diff/1/media/gpu/v4l2_video_decode... media/gpu/v4l2_video_decode_accelerator.cc:2051: if (it != processor_output_formats.end()) { On 2016/08/19 05:50:59, kcwu wrote: > swap these two if-conditions at line 2051 and 2047. > It doesn't make sense to query device first and then found the format are not > what we want. The code is removed.
https://codereview.chromium.org/2260123002/diff/20001/media/gpu/v4l2_video_de... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2260123002/diff/20001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:2041: const uint32_t kPreferredEGLImageFormat = V4L2_PIX_FMT_YVU420; could you add comment here or in commit log, why this format is preferred? https://codereview.chromium.org/2260123002/diff/20001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:2047: std::vector<uint32_t>::iterator it = how about use "auto" type?
All done. PTAL. https://codereview.chromium.org/2260123002/diff/20001/media/gpu/v4l2_video_de... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2260123002/diff/20001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:2041: const uint32_t kPreferredEGLImageFormat = V4L2_PIX_FMT_YVU420; On 2016/08/19 06:27:33, kcwu wrote: > could you add comment here or in commit log, why this format is preferred? Done. https://codereview.chromium.org/2260123002/diff/20001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:2047: std::vector<uint32_t>::iterator it = On 2016/08/19 06:27:33, kcwu wrote: > how about use "auto" type? Done.
Description was changed from ========== V4L2VDA: use YV12 as output format if processor supports it. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ========== to ========== V4L2VDA: use YV12 as output format if processor supports it. Prefer YVU420 over NV12M because ArcGpuVideoDecodeAccelerator and drm_gralloc only support single physical plane. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ==========
Description was changed from ========== V4L2VDA: use YV12 as output format if processor supports it. Prefer YVU420 over NV12M because ArcGpuVideoDecodeAccelerator and drm_gralloc only support single physical plane. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ========== to ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ARC only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ==========
Description was changed from ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ARC only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ========== to ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ARC only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ==========
Description was changed from ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ARC only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ========== to ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ARC only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ==========
Description was changed from ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ARC only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ========== to ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ARC only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ==========
Description was changed from ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ARC only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ========== to ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ARC only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. When we use NV12, chrome thinks it is RGBA and mali handles that transparently knowing it is really NV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ==========
Description was changed from ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ARC only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. When we use NV12, chrome thinks it is RGBA and mali handles that transparently knowing it is really NV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ========== to ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ARC only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. When we use NV12, chrome thinks it is RGBA and mali handles that transparently knowing it is really NV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ==========
Description was changed from ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ARC only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. When we use NV12, chrome thinks it is RGBA and mali handles that transparently knowing it is really NV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ========== to ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ARC only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. When we use NV12, chrome thinks it is RGBA and mali handles that transparently knowing it is really NV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ==========
Description was changed from ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ARC only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. When we use NV12, chrome thinks it is RGBA and mali handles that transparently knowing it is really NV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ========== to ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ARC only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. When we use NV12, chrome thinks it is RGBA and mali handles that transparently knowing it is really NV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ==========
Done. PTAL.
https://codereview.chromium.org/2260123002/diff/80001/media/gpu/v4l2_video_de... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2260123002/diff/80001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:2047: struct { I feel it's unnecessary to use struct. A normal function or even a lambda is enough. https://codereview.chromium.org/2260123002/diff/80001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:2062: std::sort(processor_output_formats.begin(), processor_output_formats.end(), Just curious, does the ordering of GetSupportedOutputFormats() output mean something? I'm wondering should we try to keep original order if not in our preference list? If so, use stable_sort.
Done. https://codereview.chromium.org/2260123002/diff/80001/media/gpu/v4l2_video_de... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2260123002/diff/80001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:2047: struct { On 2016/08/22 08:07:00, kcwu wrote: > I feel it's unnecessary to use struct. A normal function or even a lambda is > enough. Yeah. Lambda is simpler. Done. https://codereview.chromium.org/2260123002/diff/80001/media/gpu/v4l2_video_de... media/gpu/v4l2_video_decode_accelerator.cc:2062: std::sort(processor_output_formats.begin(), processor_output_formats.end(), On 2016/08/22 08:07:00, kcwu wrote: > Just curious, does the ordering of GetSupportedOutputFormats() output mean > something? I'm wondering should we try to keep original order if not in our > preference list? > If so, use stable_sort. The order of GetSupportedOutputFormats does not mean anything. It's just the order returned by the kernel driver. Also, the formats returned from GetSupportedOutputFormats should not have duplicates.
lgtm
Description was changed from ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ARC only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. When we use NV12, chrome thinks it is RGBA and mali handles that transparently knowing it is really NV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ========== to ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ARC only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ==========
Description was changed from ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ARC only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ========== to ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ArcGpuVideoDecodeAccelerator only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ==========
Description was changed from ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ArcGpuVideoDecodeAccelerator only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ========== to ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ArcGpuVideoDecodeAccelerator only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ==========
Description was changed from ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ArcGpuVideoDecodeAccelerator only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ========== to ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ArcGpuVideoDecodeAccelerator only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ==========
Description was changed from ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ArcGpuVideoDecodeAccelerator only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ========== to ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ArcGpuVideoDecodeAccelerator only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ==========
lgtm % nits. Please also test on other ARM platforms. https://codereview.chromium.org/2260123002/diff/120001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2260123002/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:2041: // Prefer YVU420 and NV12 because ArcGpuVideoDecodeAccelerator only support s/support/supports/ https://codereview.chromium.org/2260123002/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:2058: // Move the preferred format to the front. s/format/formats/ /./in the same order as in kPreferredFormats./
The CQ bit was checked by wuchengli@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
All done. Tested on veyron minnie, nyan big, and peach pit. https://codereview.chromium.org/2260123002/diff/120001/media/gpu/v4l2_video_d... File media/gpu/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/2260123002/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:2041: // Prefer YVU420 and NV12 because ArcGpuVideoDecodeAccelerator only support On 2016/08/23 05:22:00, Pawel Osciak wrote: > s/support/supports/ Done. https://codereview.chromium.org/2260123002/diff/120001/media/gpu/v4l2_video_d... media/gpu/v4l2_video_decode_accelerator.cc:2058: // Move the preferred format to the front. On 2016/08/23 05:22:00, Pawel Osciak wrote: > s/format/formats/ > /./in the same order as in kPreferredFormats./ Done.
The CQ bit was checked by wuchengli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kcwu@chromium.org, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/2260123002/#ps140001 (title: "address Pawel's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ArcGpuVideoDecodeAccelerator only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ========== to ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ArcGpuVideoDecodeAccelerator only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ArcGpuVideoDecodeAccelerator only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. ========== to ========== V4L2VDA: use YV12 as output format if processor supports it. This CL adds the support of YV12 (V4L2_PIX_FMT_YVU420) as video output format. ArcGpuVideoDecodeAccelerator only supports single physical plane. Prefer YVU420 and NV12 over YVU420M and NV12M. We also prefer YVU420 over NV12 because chrome rendering supports only YV12. BUG=chrome-os-partner:56354 TEST=Run VDA unittest and play video on oak. Committed: https://crrev.com/8dfde51b86938d8e64941cd584dfe736c14e6005 Cr-Commit-Position: refs/heads/master@{#413703} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/8dfde51b86938d8e64941cd584dfe736c14e6005 Cr-Commit-Position: refs/heads/master@{#413703} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
