Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2559423002:
media/gpu: switch v4l2_jpeg_decode_accelerator to use multi-planar APIs (Closed)
Descriptionmedia/gpu: switch v4l2_jpeg_decode_accelerator to use multi-planar APIs
This patch switches v4l2 JDA to use multi-planar APIs and buffers.
BUG=655890
BUG=chrome-os-partner:43703
TEST=Deploy my self-built chrome on device and test with hangout.
TEST=Pass jpeg_decode_accelerator_unittest.
R=henryhsu@chromium.org, posciak@chromium.org, wuchengli@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Committed: https://crrev.com/b98a5568d356aea64ef47c4039836ff49faa1a5f
Cr-Commit-Position: refs/heads/master@{#440610}
Patch Set 1 #Patch Set 2 : add PIXEL_FORMAT_I422 in enum VideoPixelFormat #Patch Set 3 : fix CopyOutputImage #Patch Set 4 : add a new BUG tag in patch description #Patch Set 5 : remove PIXEL_FORMAT_I422 #
Total comments: 19
Patch Set 6 : address the review comments from round 1 #Patch Set 7 : let's see if I get the number of patches to upload right this time... #
Total comments: 21
Patch Set 8 : address the review comments from round 2 #Patch Set 9 : remove splane code paths #Patch Set 10 : fix a typo in comments #
Total comments: 4
Patch Set 11 : add static_assert for kMaxInputPlanes #
Total comments: 26
Patch Set 12 : address the review comments from round 3 #
Total comments: 20
Patch Set 13 : address the review comments #
Messages
Total messages: 39 (14 generated)
Description was changed from ========== media/gpu: switch v4l2 jpeg decode accelerator to mplane buffer This patch switches v4l2 JDA to use mplane buffer by default. Support for splane buffer is kept for backward compatibility. BUG=chrome-os-partner:43703 TEST=Deploy my self-built chrome on device and test with hangout. R=henryhsu@chromium.org, posciak@chromium.org, wuchengli@chromium.org ========== to ========== media/gpu: switch v4l2 jpeg decode accelerator to mplane buffer This patch switches v4l2 JDA to use mplane buffer by default. Support for splane buffer is kept for backward compatibility. BUG=chrome-os-partner:43703 TEST=Deploy my self-built chrome on device and test with hangout. R=henryhsu@chromium.org, posciak@chromium.org, wuchengli@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== media/gpu: switch v4l2 jpeg decode accelerator to mplane buffer This patch switches v4l2 JDA to use mplane buffer by default. Support for splane buffer is kept for backward compatibility. BUG=chrome-os-partner:43703 TEST=Deploy my self-built chrome on device and test with hangout. R=henryhsu@chromium.org, posciak@chromium.org, wuchengli@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== media/gpu: switch v4l2 jpeg decode accelerator to mplane buffer This patch switches v4l2 JDA to use mplane buffer by default. Support for splane buffer is kept for backward compatibility. Also adds a new pixel format enumeration PIXEL_FORMAT_I422 which is used by the MediaTek JPEG decode accelerator. BUG=chrome-os-partner:43703 TEST=Deploy my self-built chrome on device and test with hangout. R=henryhsu@chromium.org, posciak@chromium.org, wuchengli@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by jcliang@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: No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Description was changed from ========== media/gpu: switch v4l2 jpeg decode accelerator to mplane buffer This patch switches v4l2 JDA to use mplane buffer by default. Support for splane buffer is kept for backward compatibility. Also adds a new pixel format enumeration PIXEL_FORMAT_I422 which is used by the MediaTek JPEG decode accelerator. BUG=chrome-os-partner:43703 TEST=Deploy my self-built chrome on device and test with hangout. R=henryhsu@chromium.org, posciak@chromium.org, wuchengli@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== media/gpu: switch v4l2 jpeg decode accelerator to mplane buffer This patch switches v4l2 JDA to use mplane buffer by default. Support for splane buffer is kept for backward compatibility. Also adds a new pixel format enumeration PIXEL_FORMAT_I422 which is used by the MediaTek JPEG decode accelerator. BUG=655890 BUG=chrome-os-partner:43703 TEST=Deploy my self-built chrome on device and test with hangout. R=henryhsu@chromium.org, posciak@chromium.org, wuchengli@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Can we separate adding PIXEL_FORMAT_I422 and adding mplane for jda to two patches? They are independent.
See https://codereview.chromium.org/1326153002. Do we need to modify media_types.mojom, histograms.xml, media_type_converters.cc, video_frame_compositor.cc, and other files in that CL?
On 2016/12/14 07:21:20, wuchengli wrote: > See https://codereview.chromium.org/1326153002. Do we need to modify > media_types.mojom, histograms.xml, media_type_converters.cc, > video_frame_compositor.cc, and other files in that CL? Done: https://codereview.chromium.org/2571163002 We have to modify them all or chrome does not compile.
Can you remove PIXEL_FORMAT_I422 from this patch?
On 2016/12/14 07:59:29, wuchengli wrote: > Can you remove PIXEL_FORMAT_I422 from this patch? I can but the CL won't compile before https://codereview.chromium.org/2571163002 is merged. Can you review the files under media/gpu/ on the current patch set for the time being? I can address your comments in the new patch set together with removing PIXEL_FORMAT_I422.
https://codereview.chromium.org/2559423002/diff/80001/media/gpu/jpeg_decode_a... File media/gpu/jpeg_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2559423002/diff/80001/media/gpu/jpeg_decode_a... media/gpu/jpeg_decode_accelerator_unittest.cc:140: scoped_refptr<V4L2Device> device = V4L2Device::Create(); Hmm. This means JDA is not run by autotest... I need to file a bug. https://codereview.chromium.org/2559423002/diff/80001/media/gpu/jpeg_decode_a... media/gpu/jpeg_decode_accelerator_unittest.cc:564: LOG(ERROR) << "Unexpected switch: " << it->first << ":" << it->second; What's this for? https://codereview.chromium.org/2559423002/diff/80001/media/gpu/v4l2_jpeg_dec... File media/gpu/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/2559423002/diff/80001/media/gpu/v4l2_jpeg_dec... media/gpu/v4l2_jpeg_decode_accelerator.cc:718: if (src_buffer.num_planes == 1) { Can we remove num_planes == 1 case and use mplane code only (line 733-759)? Is the code simpler? What input formats does this function support? https://codereview.chromium.org/2559423002/diff/80001/media/gpu/v4l2_jpeg_dec... File media/gpu/v4l2_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/2559423002/diff/80001/media/gpu/v4l2_jpeg_dec... media/gpu/v4l2_jpeg_decode_accelerator.h:47: // Ouput buffer can have as many as three planes. It looks like the output is always three planes, not one or two. s/can have as many as/have/ s/three planes/three physical planes/ https://codereview.chromium.org/2559423002/diff/80001/media/gpu/v4l2_jpeg_dec... media/gpu/v4l2_jpeg_decode_accelerator.h:50: static const uint32_t kMaxBufferPlanes = 3; We are hard-coding the number of planes to be always 3 (kOutputPlanes). kMaxBufferPlanes can be removed. https://codereview.chromium.org/2559423002/diff/80001/media/gpu/v4l2_jpeg_dec... media/gpu/v4l2_jpeg_decode_accelerator.h:92: Document this function. What's the output format? What's the supported input formats? This is not copy anymore. Use a better name. ConvertOutputImage?
https://codereview.chromium.org/2559423002/diff/80001/media/gpu/v4l2_jpeg_dec... File media/gpu/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/2559423002/diff/80001/media/gpu/v4l2_jpeg_dec... media/gpu/v4l2_jpeg_decode_accelerator.cc:433: format.fmt.pix_mp.num_planes = 1; s/1/kInputPlanes/ https://codereview.chromium.org/2559423002/diff/80001/media/gpu/v4l2_jpeg_dec... media/gpu/v4l2_jpeg_decode_accelerator.cc:492: gfx::Size(format.fmt.pix.width, format.fmt.pix.height))); format.fmt.pix_mp.width and format.fmt.pix_mp.height https://codereview.chromium.org/2559423002/diff/80001/media/gpu/v4l2_jpeg_dec... media/gpu/v4l2_jpeg_decode_accelerator.cc:781: dqbuf.length = 1; s/1/kInputPlanes/ https://codereview.chromium.org/2559423002/diff/80001/media/gpu/v4l2_jpeg_dec... media/gpu/v4l2_jpeg_decode_accelerator.cc:782: dqbuf.m.planes = planes; |planes| has kOutputPlanes elements. It assumes kOutputPlanes is no less than kInputPlanes.
https://codereview.chromium.org/2559423002/diff/80001/media/gpu/jpeg_decode_a... File media/gpu/jpeg_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2559423002/diff/80001/media/gpu/jpeg_decode_a... media/gpu/jpeg_decode_accelerator_unittest.cc:564: LOG(ERROR) << "Unexpected switch: " << it->first << ":" << it->second; On 2016/12/15 09:18:25, wuchengli wrote: > What's this for? LOG(FATAL) generates core dumps and it wasted disk space on my DUT. There's no reason to generate core dump for this error. https://codereview.chromium.org/2559423002/diff/80001/media/gpu/v4l2_jpeg_dec... File media/gpu/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/2559423002/diff/80001/media/gpu/v4l2_jpeg_dec... media/gpu/v4l2_jpeg_decode_accelerator.cc:433: format.fmt.pix_mp.num_planes = 1; On 2016/12/15 09:56:18, henryhsu wrote: > s/1/kInputPlanes/ Technically the 1 here means differently from kInputPlanes. Here it's referring to the splane V4L2_PIX_FMT_YUV420, and kInputPlanes is 1 because the input is alway V4L2_PIX_FMT_JPEG. I'd prefer not to mix them up. https://codereview.chromium.org/2559423002/diff/80001/media/gpu/v4l2_jpeg_dec... media/gpu/v4l2_jpeg_decode_accelerator.cc:492: gfx::Size(format.fmt.pix.width, format.fmt.pix.height))); On 2016/12/15 09:56:17, henryhsu wrote: > format.fmt.pix_mp.width and format.fmt.pix_mp.height Done. https://codereview.chromium.org/2559423002/diff/80001/media/gpu/v4l2_jpeg_dec... media/gpu/v4l2_jpeg_decode_accelerator.cc:718: if (src_buffer.num_planes == 1) { On 2016/12/15 09:18:25, wuchengli wrote: > Can we remove num_planes == 1 case and use mplane code only (line 733-759)? Is > the code simpler? What input formats does this function support? We have to keep the num_planes == 1 case to handle splane buffers (e.g. for exynos s5p jpeg decoder). libyuv::ConvertToI420 accepts a large variety of input pixel formats, so it's much simpler if we just pass the buffer to libyuv:ConvertToI420 and let it handle the rest. Currently this function's input format can be whatever ConvertToI420 accepts, plus V4L2_PIX_FMT_YUV420M and V4L2_PIX_FMT_YUV422M. https://codereview.chromium.org/2559423002/diff/80001/media/gpu/v4l2_jpeg_dec... media/gpu/v4l2_jpeg_decode_accelerator.cc:781: dqbuf.length = 1; On 2016/12/15 09:56:17, henryhsu wrote: > s/1/kInputPlanes/ Done. https://codereview.chromium.org/2559423002/diff/80001/media/gpu/v4l2_jpeg_dec... media/gpu/v4l2_jpeg_decode_accelerator.cc:782: dqbuf.m.planes = planes; On 2016/12/15 09:56:18, henryhsu wrote: > |planes| has kOutputPlanes elements. It assumes kOutputPlanes is no less than > kInputPlanes. Done. https://codereview.chromium.org/2559423002/diff/80001/media/gpu/v4l2_jpeg_dec... File media/gpu/v4l2_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/2559423002/diff/80001/media/gpu/v4l2_jpeg_dec... media/gpu/v4l2_jpeg_decode_accelerator.h:47: // Ouput buffer can have as many as three planes. On 2016/12/15 09:18:25, wuchengli wrote: > It looks like the output is always three planes, not one or two. s/can have as > many as/have/ > > s/three planes/three physical planes/ Done. I was thinking there are 2-plane and 4-plane pixel format as well, but for now it's always 3-plane. https://codereview.chromium.org/2559423002/diff/80001/media/gpu/v4l2_jpeg_dec... media/gpu/v4l2_jpeg_decode_accelerator.h:50: static const uint32_t kMaxBufferPlanes = 3; On 2016/12/15 09:18:25, wuchengli wrote: > We are hard-coding the number of planes to be always 3 (kOutputPlanes). > kMaxBufferPlanes can be removed. Done. https://codereview.chromium.org/2559423002/diff/80001/media/gpu/v4l2_jpeg_dec... media/gpu/v4l2_jpeg_decode_accelerator.h:92: On 2016/12/15 09:18:25, wuchengli wrote: > Document this function. What's the output format? What's the supported input > formats? > > This is not copy anymore. Use a better name. ConvertOutputImage? Done.
https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... File media/gpu/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:773: struct v4l2_plane input_plane; use input_planes[kInputPlanes]; We don't know the value of kInputPlanes here. But line 781 and 782 assume kInputPlanes is 1.
https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... File media/gpu/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:362: format.fmt.pix_mp.field = V4L2_FIELD_ANY; format.fmt.pix_mp.num_planes = kInputPlanes; https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:480: buffer.length = kOutputPlanes; Use format.fmt.pix_mp.num_planes returned from S_FMT. The output format can be V4L2_PIX_FMT_YUV420 (we request it in line 434) or V4L2_PIX_FMT_YUV420M. https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:486: for (uint32_t i = 0; i < kOutputPlanes; ++i) { s/kOutputPlanes/buffer.length/ https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:490: VideoFrame::AllocationSize( Use VideoFrame::PlaneSize to check each plane in the for loop. https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:500: if (V4L2_TYPE_IS_MULTIPLANAR(output_buf_type_)) { if and else clauses can be combined. if (V4L2_TYPE_IS_MULTIPLANAR(output_buf_type_)) output_buffer_map_[i].num_planes = buffer.length; else output_buffer_map_[i].num_planes = 1; for (size_t j = 0; j < output_buffer_map_[i].num_planes; ++j) { ... } https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:821: dqbuf.length = kOutputPlanes; I realized the number of planes could be 1 in mplane case (line 433-434). So kOutputPlanes should be changed to BufferRecord.num_planes. Right? https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:975: } let's also set bytesused for splane. } else { qbuf.bytesused = input_record.length[0]; } https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... File media/gpu/v4l2_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.h:47: // Ouput buffer has three physical planes. I found the number of planes can be 1 V4L2_PIX_FMT_YUV420 or 3 V4L2_PIX_FMT_YUV420M. So the number of output planes is not always 1. T his is the number of maximum output planes. Actually we can just use VIDEO_MAX_PLANES from linux/videodev2.h. kOutputPlanes can be removed. https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.h:56: size_t num_planes; // Number of mapped planes. s/planes/physical planes/. So it's clear this is not logical planes.
https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... File media/gpu/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:362: format.fmt.pix_mp.field = V4L2_FIELD_ANY; On 2016/12/16 03:15:15, wuchengli wrote: > format.fmt.pix_mp.num_planes = kInputPlanes; Done. https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:480: buffer.length = kOutputPlanes; On 2016/12/16 03:15:15, wuchengli wrote: > Use format.fmt.pix_mp.num_planes returned from S_FMT. The output format can be > V4L2_PIX_FMT_YUV420 (we request it in line 434) or V4L2_PIX_FMT_YUV420M. Done. https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:486: for (uint32_t i = 0; i < kOutputPlanes; ++i) { On 2016/12/16 03:15:15, wuchengli wrote: > s/kOutputPlanes/buffer.length/ Done. https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:490: VideoFrame::AllocationSize( On 2016/12/16 03:15:15, wuchengli wrote: > Use VideoFrame::PlaneSize to check each plane in the for loop. Done. https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:500: if (V4L2_TYPE_IS_MULTIPLANAR(output_buf_type_)) { On 2016/12/16 03:15:15, wuchengli wrote: > if and else clauses can be combined. > > if (V4L2_TYPE_IS_MULTIPLANAR(output_buf_type_)) > output_buffer_map_[i].num_planes = buffer.length; > else > output_buffer_map_[i].num_planes = 1; > for (size_t j = 0; j < output_buffer_map_[i].num_planes; ++j) { > ... > } We can't do this because the fields in |buffer| that Mmap() uses are different. For splane it needs buffer.length and buffer.m.offset; for mplane it needs buffer.m.planes[j].length and buffer.m.planes[j].m.mem_offset. https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:773: struct v4l2_plane input_plane; On 2016/12/16 03:05:27, henryhsu wrote: > use input_planes[kInputPlanes]; > We don't know the value of kInputPlanes here. But line 781 and 782 assume > kInputPlanes is 1. Done. https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:821: dqbuf.length = kOutputPlanes; On 2016/12/16 03:15:15, wuchengli wrote: > I realized the number of planes could be 1 in mplane case (line 433-434). So > kOutputPlanes should be changed to BufferRecord.num_planes. Right? I believe this works as long as kOutputPlanes >= kInputPlanes. Also we can't do what you proposed here because we don't know which BufferRecord we're going to get; we learn that by looking at the |dqbuf.index| the VIDIOC_DQBUF ioctl returns below. https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:975: } On 2016/12/16 03:15:15, wuchengli wrote: > let's also set bytesused for splane. > } else { > qbuf.bytesused = input_record.length[0]; > } Done.
https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... File media/gpu/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:500: if (V4L2_TYPE_IS_MULTIPLANAR(output_buf_type_)) { On 2016/12/16 04:19:53, jcliang wrote: > On 2016/12/16 03:15:15, wuchengli wrote: > > if and else clauses can be combined. > > > > if (V4L2_TYPE_IS_MULTIPLANAR(output_buf_type_)) > > output_buffer_map_[i].num_planes = buffer.length; > > else > > output_buffer_map_[i].num_planes = 1; > > for (size_t j = 0; j < output_buffer_map_[i].num_planes; ++j) { > > ... > > } > > We can't do this because the fields in |buffer| that Mmap() uses are different. > For splane it needs buffer.length and buffer.m.offset; for mplane it needs > buffer.m.planes[j].length and buffer.m.planes[j].m.mem_offset. You are right. https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:821: dqbuf.length = kOutputPlanes; On 2016/12/16 04:19:53, jcliang wrote: > On 2016/12/16 03:15:15, wuchengli wrote: > > I realized the number of planes could be 1 in mplane case (line 433-434). So > > kOutputPlanes should be changed to BufferRecord.num_planes. Right? > > I believe this works as long as kOutputPlanes >= kInputPlanes. > > Also we can't do what you proposed here because we don't know which BufferRecord > we're going to get; we learn that by looking at the |dqbuf.index| the > VIDIOC_DQBUF ioctl returns below. I see. Then we need to add a variable to store the number of planes. Maybe |output_buffer_num_planes_|. In that case, BufferRecord.num_planes doesn't seem to be required.
I'll be OOO for two weeks. Leaving this to other reviewers.
Description was changed from ========== media/gpu: switch v4l2 jpeg decode accelerator to mplane buffer This patch switches v4l2 JDA to use mplane buffer by default. Support for splane buffer is kept for backward compatibility. Also adds a new pixel format enumeration PIXEL_FORMAT_I422 which is used by the MediaTek JPEG decode accelerator. BUG=655890 BUG=chrome-os-partner:43703 TEST=Deploy my self-built chrome on device and test with hangout. R=henryhsu@chromium.org, posciak@chromium.org, wuchengli@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== media/gpu: switch v4l2_jpeg_decode_accelerator to use multi-planar APIs This patch switches v4l2 JDA to use multi-plane APIs and buffers. BUG=655890 BUG=chrome-os-partner:43703 TEST=Deploy my self-built chrome on device and test with hangout. TEST=Pass jpeg_decode_accelerator_unittest. R=henryhsu@chromium.org, posciak@chromium.org, wuchengli@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== media/gpu: switch v4l2_jpeg_decode_accelerator to use multi-planar APIs This patch switches v4l2 JDA to use multi-plane APIs and buffers. BUG=655890 BUG=chrome-os-partner:43703 TEST=Deploy my self-built chrome on device and test with hangout. TEST=Pass jpeg_decode_accelerator_unittest. R=henryhsu@chromium.org, posciak@chromium.org, wuchengli@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== media/gpu: switch v4l2_jpeg_decode_accelerator to use multi-planar APIs This patch switches v4l2 JDA to use multi-planar APIs and buffers. BUG=655890 BUG=chrome-os-partner:43703 TEST=Deploy my self-built chrome on device and test with hangout. TEST=Pass jpeg_decode_accelerator_unittest. R=henryhsu@chromium.org, posciak@chromium.org, wuchengli@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... File media/gpu/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/2559423002/diff/120001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:821: dqbuf.length = kOutputPlanes; On 2016/12/16 06:50:32, wuchengli wrote: > On 2016/12/16 04:19:53, jcliang wrote: > > On 2016/12/16 03:15:15, wuchengli wrote: > > > I realized the number of planes could be 1 in mplane case (line 433-434). So > > > kOutputPlanes should be changed to BufferRecord.num_planes. Right? > > > > I believe this works as long as kOutputPlanes >= kInputPlanes. > > > > Also we can't do what you proposed here because we don't know which > BufferRecord > > we're going to get; we learn that by looking at the |dqbuf.index| the > > VIDIOC_DQBUF ioctl returns below. > I see. Then we need to add a variable to store the number of planes. Maybe > |output_buffer_num_planes_|. In that case, BufferRecord.num_planes doesn't seem > to be required. Done.
https://codereview.chromium.org/2559423002/diff/180001/media/gpu/v4l2_jpeg_de... File media/gpu/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/2559423002/diff/180001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:383: input_buffer_map_[i].address[0] = address; Can we also change this like output buffer? Because current code assumes the kMaxInputPlanes must be 1. for (size_t j = 0; j < buffer.length; ++j) { input_buffer_map_[i].address[j] = address; input_buffer_map_[i].length[j] = plane.length; } https://codereview.chromium.org/2559423002/diff/180001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:889: input_record.address[0], input_record.length[0])) { This also assume the kMaxInputPlanes is 1. I'm starting to think whether it is easier to use static_assert(kMaxInputPlanes == 1). If we use static_assert, we can have the assumption and make code simple.
https://codereview.chromium.org/2559423002/diff/180001/media/gpu/v4l2_jpeg_de... File media/gpu/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/2559423002/diff/180001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:383: input_buffer_map_[i].address[0] = address; On 2016/12/19 07:07:27, henryhsu wrote: > Can we also change this like output buffer? > Because current code assumes the kMaxInputPlanes must be 1. > > for (size_t j = 0; j < buffer.length; ++j) { > input_buffer_map_[i].address[j] = address; > input_buffer_map_[i].length[j] = plane.length; > } It's a known fact that the input can only be V4L2_PIX_FMT_JPEG and this class is only designed to handle V4L2_PIX_FMT_JPEG. https://codereview.chromium.org/2559423002/diff/180001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:889: input_record.address[0], input_record.length[0])) { On 2016/12/19 07:07:27, henryhsu wrote: > This also assume the kMaxInputPlanes is 1. > I'm starting to think whether it is easier to use static_assert(kMaxInputPlanes > == 1). > > If we use static_assert, we can have the assumption and make code simple. Sure. Let's add a static_assert.
lgtm. But I'm not the owner. You should wait the approval from owner
https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... File media/gpu/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:375: struct v4l2_plane plane; Perhaps s/plane/planes[kMaxInputPlanes]/ for consistency. https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:381: buffer.length = kMaxInputPlanes; arraysize(planes) https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:384: DCHECK_EQ(1u, buffer.length); s/1u/kMaxInputPlanes/ s/DCHECK_EQ(...)/if (!...) return false/ https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:385: void* address = device_->Mmap(NULL, plane.length, PROT_READ | PROT_WRITE, for (i=0;i<kMaxInputPlanes;++i) https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:387: input_buffer_map_[i].address[0] = address; s/0/i/, etc. https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:408: format.fmt.pix_mp.num_planes = 1; Should we be using kMaxOutputPlanes? https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:435: struct v4l2_plane planes[output_buffer_num_planes_]; It might be good to not use a non-const here, perhaps VIDEO_MAX_PLANES instead? https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:445: DCHECK_EQ(output_buffer_num_planes_, buffer.length); This should preferably be an if() also. https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:482: for (const auto& input_record : input_buffer_map_) { kMaxInputPlanes for consistency? https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:671: } else if (output_buffer_pixelformat_ == V4L2_PIX_FMT_YUV420M || We are not setting the format to V4L2_PIX_FMT_YUV420M/V4L2_PIX_FMT_YUV422M, but using V4L2_PIX_FMT_YUV420 (which is ok) above. Do we need the ability to convert V4L2_PIX_FMT_YUV420M and V4L2_PIX_FMT_YUV422M? https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:713: struct v4l2_plane input_plane; We could use VIDEO_MAX_PLANES everywhere for v4l2_plane arrays and arraysize() for consistency and to simplify things perhaps. https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... File media/gpu/v4l2_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.h:94: // - V4L2_YUV_422M s/V4L2_YUV_420M/V4L2_PIX_FMT_YUV420M/ s/V4L2_YUV_422M/V4L2_PIX_FMT_YUV422M/ ? https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.h:144: uint32_t output_buffer_num_planes_; s/uint32_t/size_t/
https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... File media/gpu/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:375: struct v4l2_plane plane; On 2016/12/20 05:19:12, Pawel Osciak wrote: > Perhaps s/plane/planes[kMaxInputPlanes]/ for consistency. I'm replacing all the v4l2_plane arrays with planes[VIDEO_MAX_PLANES] for simplicity. https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:381: buffer.length = kMaxInputPlanes; On 2016/12/20 05:19:12, Pawel Osciak wrote: > arraysize(planes) Done. https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:384: DCHECK_EQ(1u, buffer.length); On 2016/12/20 05:19:12, Pawel Osciak wrote: > s/1u/kMaxInputPlanes/ > > s/DCHECK_EQ(...)/if (!...) return false/ Done. https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:385: void* address = device_->Mmap(NULL, plane.length, PROT_READ | PROT_WRITE, On 2016/12/20 05:19:12, Pawel Osciak wrote: > for (i=0;i<kMaxInputPlanes;++i) Done. https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:387: input_buffer_map_[i].address[0] = address; On 2016/12/20 05:19:12, Pawel Osciak wrote: > s/0/i/, etc. Done. https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:408: format.fmt.pix_mp.num_planes = 1; On 2016/12/20 05:19:12, Pawel Osciak wrote: > Should we be using kMaxOutputPlanes? I thought we should set a num_planes matching the pixelformat (V4L2_PIX_FMT_YUV420 below). Afaict the num_planes does not have real affects and should be set by the driver. What's the best practice for setting num_planes in S_FMT for multi-planar types? https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:435: struct v4l2_plane planes[output_buffer_num_planes_]; On 2016/12/20 05:19:12, Pawel Osciak wrote: > It might be good to not use a non-const here, perhaps VIDEO_MAX_PLANES instead? Done. https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:445: DCHECK_EQ(output_buffer_num_planes_, buffer.length); On 2016/12/20 05:19:12, Pawel Osciak wrote: > This should preferably be an if() also. Done. https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:482: for (const auto& input_record : input_buffer_map_) { On 2016/12/20 05:19:12, Pawel Osciak wrote: > kMaxInputPlanes for consistency? Done. https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:671: } else if (output_buffer_pixelformat_ == V4L2_PIX_FMT_YUV420M || On 2016/12/20 05:19:12, Pawel Osciak wrote: > We are not setting the format to V4L2_PIX_FMT_YUV420M/V4L2_PIX_FMT_YUV422M, but > using V4L2_PIX_FMT_YUV420 (which is ok) above. Do we need the ability to convert > V4L2_PIX_FMT_YUV420M and V4L2_PIX_FMT_YUV422M? The mtk-jpeg decoder only supports V4L2_PIX_FMT_YUV{422M/420M}. On mtk-jpeg in capture buffer S_FMT it'll negotiate to either 422M or 420M depending on the input JPEG image's sub-sampling. The s5p-jpeg driver simply fails if we ask for V4L2_PIX_FMT_YUV420M (which is a bug IMO), so for the time being we have to set the default pixel format to V4L2_PIX_FMT_YUV420 and rely on S_FMT to negotiate the final format on mtk-jpeg. https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:713: struct v4l2_plane input_plane; On 2016/12/20 05:19:12, Pawel Osciak wrote: > We could use VIDEO_MAX_PLANES everywhere for v4l2_plane arrays and arraysize() > for consistency and to simplify things perhaps. Done. https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... File media/gpu/v4l2_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.h:94: // - V4L2_YUV_422M On 2016/12/20 05:19:12, Pawel Osciak wrote: > s/V4L2_YUV_420M/V4L2_PIX_FMT_YUV420M/ > s/V4L2_YUV_422M/V4L2_PIX_FMT_YUV422M/ ? Done. https://codereview.chromium.org/2559423002/diff/200001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.h:144: uint32_t output_buffer_num_planes_; On 2016/12/20 05:19:12, Pawel Osciak wrote: > s/uint32_t/size_t/ Done.
https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... File media/gpu/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:389: device_->Mmap(NULL, planes[j].length, PROT_READ | PROT_WRITE, We should verify return value from Mmap(). https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:434: V4L2Device::V4L2PixFmtToVideoPixelFormat(output_buffer_pixelformat_); if (output_format == PIXEL_FORMAT_UNKNOWN) // error out We'd probably also prefer this at l.422. https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:454: if (static_cast<int64_t>(planes[j].length) < checked_cast<size_t> perhaps... https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:719: DCHECK_GE(output_buffer_num_planes_, 0u); Should this be GT? https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:932: DCHECK_GE(output_buffer_num_planes_, 0u); GT? https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... File media/gpu/v4l2_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.h:45: // Input pixel format (i.e. V4L2_PIX_FMT_JPEG) has only one physical plane. Nit: I'd suggest moving these two constants to an anonymous namespace in .cc since they are not being used here anymore. https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.h:46: static const uint32_t kMaxInputPlanes = 1; size_t https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.h:48: static const uint32_t kMaxOutputPlanes = 3; size_t https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.h:90: // Convert |src_buffer| to I420 and copy the result to |dst_frame|. s/src_buffer/output_buffer/ ? Please also document return value. https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.h:91: // The function currently accepts the following format as |src_pixelformat|: Please update this sentence as well.
https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... File media/gpu/v4l2_jpeg_decode_accelerator.cc (right): https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:389: device_->Mmap(NULL, planes[j].length, PROT_READ | PROT_WRITE, On 2016/12/23 at 06:05:52, Pawel Osciak wrote: > We should verify return value from Mmap(). Done. https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:434: V4L2Device::V4L2PixFmtToVideoPixelFormat(output_buffer_pixelformat_); On 2016/12/23 06:05:52, Pawel Osciak wrote: > if (output_format == PIXEL_FORMAT_UNKNOWN) > // error out > > We'd probably also prefer this at l.422. Done. https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:454: if (static_cast<int64_t>(planes[j].length) < On 2016/12/23 06:05:52, Pawel Osciak wrote: > checked_cast<size_t> perhaps... Using base::checked_cast<> is a good idea, but VideoFrame::PlaneSize(...).Area() returns int so we need signed integer here. I chose int64_t to make sure it does not overflow since planes[j].length is of type uint32_t. https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:719: DCHECK_GE(output_buffer_num_planes_, 0u); On 2016/12/23 at 06:05:52, Pawel Osciak wrote: > Should this be GT? Hmm, this check should be removed. The first call to Dequeue() before the first V4L2_EVENT_SRC_CH_RESOLUTION arrives may have output_buffer_num_planes_ set to 0. https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.cc:932: DCHECK_GE(output_buffer_num_planes_, 0u); On 2016/12/23 at 06:05:52, Pawel Osciak wrote: > GT? Done. https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... File media/gpu/v4l2_jpeg_decode_accelerator.h (right): https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.h:45: // Input pixel format (i.e. V4L2_PIX_FMT_JPEG) has only one physical plane. On 2016/12/23 06:05:52, Pawel Osciak wrote: > Nit: I'd suggest moving these two constants to an anonymous namespace in .cc > since they are not being used here anymore. Done. https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.h:46: static const uint32_t kMaxInputPlanes = 1; On 2016/12/23 06:05:52, Pawel Osciak wrote: > size_t Done. https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.h:48: static const uint32_t kMaxOutputPlanes = 3; On 2016/12/23 06:05:52, Pawel Osciak wrote: > size_t Done. https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.h:90: // Convert |src_buffer| to I420 and copy the result to |dst_frame|. On 2016/12/23 06:05:52, Pawel Osciak wrote: > s/src_buffer/output_buffer/ ? > > Please also document return value. Done. https://codereview.chromium.org/2559423002/diff/220001/media/gpu/v4l2_jpeg_de... media/gpu/v4l2_jpeg_decode_accelerator.h:91: // The function currently accepts the following format as |src_pixelformat|: On 2016/12/23 06:05:52, Pawel Osciak wrote: > Please update this sentence as well. Done.
lgtm
The CQ bit was checked by jcliang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from henryhsu@chromium.org Link to the patchset: https://codereview.chromium.org/2559423002/#ps240001 (title: "address the review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1482483653430050,
"parent_rev": "24290d32376c6abf4f2502887bd8fb9dbf8c1bff", "commit_rev":
"739cab52b2a44ac9fb1663ad2e2df9f8290b8c6d"}
Message was sent while issue was closed.
Description was changed from ========== media/gpu: switch v4l2_jpeg_decode_accelerator to use multi-planar APIs This patch switches v4l2 JDA to use multi-planar APIs and buffers. BUG=655890 BUG=chrome-os-partner:43703 TEST=Deploy my self-built chrome on device and test with hangout. TEST=Pass jpeg_decode_accelerator_unittest. R=henryhsu@chromium.org, posciak@chromium.org, wuchengli@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== media/gpu: switch v4l2_jpeg_decode_accelerator to use multi-planar APIs This patch switches v4l2 JDA to use multi-planar APIs and buffers. BUG=655890 BUG=chrome-os-partner:43703 TEST=Deploy my self-built chrome on device and test with hangout. TEST=Pass jpeg_decode_accelerator_unittest. R=henryhsu@chromium.org, posciak@chromium.org, wuchengli@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2559423002 ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== media/gpu: switch v4l2_jpeg_decode_accelerator to use multi-planar APIs This patch switches v4l2 JDA to use multi-planar APIs and buffers. BUG=655890 BUG=chrome-os-partner:43703 TEST=Deploy my self-built chrome on device and test with hangout. TEST=Pass jpeg_decode_accelerator_unittest. R=henryhsu@chromium.org, posciak@chromium.org, wuchengli@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2559423002 ========== to ========== media/gpu: switch v4l2_jpeg_decode_accelerator to use multi-planar APIs This patch switches v4l2 JDA to use multi-planar APIs and buffers. BUG=655890 BUG=chrome-os-partner:43703 TEST=Deploy my self-built chrome on device and test with hangout. TEST=Pass jpeg_decode_accelerator_unittest. R=henryhsu@chromium.org, posciak@chromium.org, wuchengli@chromium.org CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Committed: https://crrev.com/b98a5568d356aea64ef47c4039836ff49faa1a5f Cr-Commit-Position: refs/heads/master@{#440610} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/b98a5568d356aea64ef47c4039836ff49faa1a5f Cr-Commit-Position: refs/heads/master@{#440610} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
