Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
874083005:
Fix visible size for V4L2 VDA (Closed)
|
Created:
5 years, 10 months ago by kcwu Modified:
5 years, 10 months ago CC:
chromium-reviews, posciak+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 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix visible size for V4L2 VDA
BUG=chromium:449920
TEST=apprtc loopback with resolution 640x354 on peach pit
Committed: https://crrev.com/ad3526512104b5a7352f356b62f8cea3427a58cb
Cr-Commit-Position: refs/heads/master@{#314138}
Patch Set 1 #
Total comments: 18
Patch Set 2 : #
Total comments: 7
Patch Set 3 : indent #Patch Set 4 : rebase #Patch Set 5 : rebase #
Messages
Total messages: 24 (6 generated)
kcwu@chromium.org changed reviewers: + owenlin@chromium.org, posciak@chromium.org, wuchengli@chromium.org
lgtm % one nit https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_decode_accelerator.cc:1654: frame_buffer_size_.SetSize( Since we set |frame_buffer_size_| here, how about getting the |frame_visible_size_| at the same time?
https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_decode_accelerator.cc:1654: frame_buffer_size_.SetSize( On 2015/01/29 02:42:09, Owen Lin wrote: > Since we set |frame_buffer_size_| here, how about getting the > |frame_visible_size_| at the same time? I'm not sure ... since this function is named "CreateBuffersForFormat". Getting crop size here seems weird.
https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_decode_accelerator.cc:1654: frame_buffer_size_.SetSize( On 2015/01/29 02:58:38, kcwu wrote: > On 2015/01/29 02:42:09, Owen Lin wrote: > > Since we set |frame_buffer_size_| here, how about getting the > > |frame_visible_size_| at the same time? > > I'm not sure ... since this function is named "CreateBuffersForFormat". > Getting crop size here seems weird. But we have already set |frame_buffer_size_| inside the function. How about considering both |frame_buffer_size_| and |frame_visible_size_| as attributes of the OutputBuffers. So we set them when we create the buffers.
https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_decode_accelerator.cc:1551: if (!GetCropSize(frame_buffer_size_, &frame_visible_size_)) Could we call this from GetFormatInfo()? https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_decode_accelerator.cc:1669: struct v4l2_crop crop_arg; memset to 0 please. https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_decode_accelerator.cc:1676: DVLOG(3) << "Crop rectangle is " << rect.ToString(); Let's stop calling this crop and say visible size everywhere instead please. https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_decode_accelerator.cc:1677: if (!gfx::Rect(buffer_size).Contains(rect)) { Please also check if it's !Empty() https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_decode_accelerator.cc:1683: if (!rect.origin().IsOrigin()) { Please add a comment why. https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_decode_accelerator.cc:2017: DVLOG(3) << "IsResolutionChangeNecessary(): crop size change detected"; Crop size does not influence resolution change. If coded size didn't change, a new crop size does not require reallocating buffers. https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... File content/common/gpu/media/v4l2_video_decode_accelerator.h (right): https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_decode_accelerator.h:426: gfx::Size frame_visible_size_; Please use visible_size_ and rename frame_buffer_size to coded_size_ to match the general Chrome conventions.
https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right): https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_decode_accelerator.cc:1551: if (!GetCropSize(frame_buffer_size_, &frame_visible_size_)) On 2015/01/29 05:02:04, Pawel Osciak wrote: > Could we call this from GetFormatInfo()? Done. https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_decode_accelerator.cc:1654: frame_buffer_size_.SetSize( On 2015/01/29 03:40:11, Owen Lin wrote: > On 2015/01/29 02:58:38, kcwu wrote: > > On 2015/01/29 02:42:09, Owen Lin wrote: > > > Since we set |frame_buffer_size_| here, how about getting the > > > |frame_visible_size_| at the same time? > > > > I'm not sure ... since this function is named "CreateBuffersForFormat". > > Getting crop size here seems weird. > > But we have already set |frame_buffer_size_| inside the function. How about > considering both |frame_buffer_size_| and |frame_visible_size_| as attributes of > the OutputBuffers. So we set them when we create the buffers. Moved to GetFormatInfo() https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_decode_accelerator.cc:1669: struct v4l2_crop crop_arg; On 2015/01/29 05:02:04, Pawel Osciak wrote: > memset to 0 please. Done. https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_decode_accelerator.cc:1676: DVLOG(3) << "Crop rectangle is " << rect.ToString(); On 2015/01/29 05:02:04, Pawel Osciak wrote: > Let's stop calling this crop and say visible size everywhere instead please. Done. https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_decode_accelerator.cc:1677: if (!gfx::Rect(buffer_size).Contains(rect)) { On 2015/01/29 05:02:04, Pawel Osciak wrote: > Please also check if it's !Empty() Done. https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_decode_accelerator.cc:1683: if (!rect.origin().IsOrigin()) { On 2015/01/29 05:02:04, Pawel Osciak wrote: > Please add a comment why. Done. https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_decode_accelerator.cc:2017: DVLOG(3) << "IsResolutionChangeNecessary(): crop size change detected"; On 2015/01/29 05:02:04, Pawel Osciak wrote: > Crop size does not influence resolution change. If coded size didn't change, a > new crop size does not require reallocating buffers. Done. https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... File content/common/gpu/media/v4l2_video_decode_accelerator.h (right): https://codereview.chromium.org/874083005/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_decode_accelerator.h:426: gfx::Size frame_visible_size_; On 2015/01/29 05:02:04, Pawel Osciak wrote: > Please use visible_size_ and rename frame_buffer_size to coded_size_ to match > the general Chrome conventions. Done.
lgtm % nits https://chromiumcodereview.appspot.com/874083005/diff/20001/content/common/gp... File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/874083005/diff/20001/content/common/gp... content/common/gpu/media/v4l2_video_decode_accelerator.cc:343: DCHECK(buffers[i].size() == coded_size_); I have a feeling there was a reason why we didn't use DCHECK_EQ here? https://chromiumcodereview.appspot.com/874083005/diff/20001/content/common/gp... content/common/gpu/media/v4l2_video_decode_accelerator.cc:353: EGLImageKHR egl_image = device_->CreateEGLImage( To be honest I really preferred the previous indent, more readable. https://chromiumcodereview.appspot.com/874083005/diff/20001/content/common/gp... content/common/gpu/media/v4l2_video_decode_accelerator.cc:1830: FROM_HERE, base::Bind(&Client::ProvidePictureBuffers, client_, Ditto preferred the old indent...
https://chromiumcodereview.appspot.com/874083005/diff/20001/content/common/gp... File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/874083005/diff/20001/content/common/gp... content/common/gpu/media/v4l2_video_decode_accelerator.cc:343: DCHECK(buffers[i].size() == coded_size_); On 2015/02/02 08:54:16, Pawel Osciak wrote: > I have a feeling there was a reason why we didn't use DCHECK_EQ here? DCHECK_EQ doesn't like objects unable send to output stream. gfx::Size needs to be ToString() for output explicitly. https://chromiumcodereview.appspot.com/874083005/diff/20001/content/common/gp... content/common/gpu/media/v4l2_video_decode_accelerator.cc:353: EGLImageKHR egl_image = device_->CreateEGLImage( On 2015/02/02 08:54:16, Pawel Osciak wrote: > To be honest I really preferred the previous indent, more readable. It's reindented by git-cl format. Do you still prefer old indent? https://chromiumcodereview.appspot.com/874083005/diff/20001/content/common/gp... content/common/gpu/media/v4l2_video_decode_accelerator.cc:1830: FROM_HERE, base::Bind(&Client::ProvidePictureBuffers, client_, On 2015/02/02 08:54:16, Pawel Osciak wrote: > Ditto preferred the old indent... It's reindented by git-cl format. Do you still prefer old indent?
https://chromiumcodereview.appspot.com/874083005/diff/20001/content/common/gp... File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right): https://chromiumcodereview.appspot.com/874083005/diff/20001/content/common/gp... content/common/gpu/media/v4l2_video_decode_accelerator.cc:353: EGLImageKHR egl_image = device_->CreateEGLImage( On 2015/02/02 09:01:47, kcwu wrote: > On 2015/02/02 08:54:16, Pawel Osciak wrote: > > To be honest I really preferred the previous indent, more readable. > > It's reindented by git-cl format. Do you still prefer old indent? Let's follow git cl format. If the intent looks bad, we should fix git cl format. If we manually change this code, the next person who uses git cl format will still face the same problem.
The CQ bit was checked by kcwu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/874083005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...) ios_rel_device_ninja_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2015/02/02 09:05:15, wuchengli wrote: > https://chromiumcodereview.appspot.com/874083005/diff/20001/content/common/gp... > File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right): > > https://chromiumcodereview.appspot.com/874083005/diff/20001/content/common/gp... > content/common/gpu/media/v4l2_video_decode_accelerator.cc:353: EGLImageKHR > egl_image = device_->CreateEGLImage( > On 2015/02/02 09:01:47, kcwu wrote: > > On 2015/02/02 08:54:16, Pawel Osciak wrote: > > > To be honest I really preferred the previous indent, more readable. > > > > It's reindented by git-cl format. Do you still prefer old indent? > Let's follow git cl format. If the intent looks bad, we should fix git cl > format. If we manually change this code, the next person who uses git cl format > will still face the same problem. To be honest I still prefer the old indent. It's much easier to read. Git cl format does not have intelligence about what is clearer. Both indents are correct.
On 2015/02/02 10:44:04, Pawel Osciak wrote: > On 2015/02/02 09:05:15, wuchengli wrote: > > > https://chromiumcodereview.appspot.com/874083005/diff/20001/content/common/gp... > > File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right): > > > > > https://chromiumcodereview.appspot.com/874083005/diff/20001/content/common/gp... > > content/common/gpu/media/v4l2_video_decode_accelerator.cc:353: EGLImageKHR > > egl_image = device_->CreateEGLImage( > > On 2015/02/02 09:01:47, kcwu wrote: > > > On 2015/02/02 08:54:16, Pawel Osciak wrote: > > > > To be honest I really preferred the previous indent, more readable. > > > > > > It's reindented by git-cl format. Do you still prefer old indent? > > Let's follow git cl format. If the intent looks bad, we should fix git cl > > format. If we manually change this code, the next person who uses git cl > format > > will still face the same problem. > > To be honest I still prefer the old indent. It's much easier to read. Git cl > format does not have intelligence about what is clearer. Both indents are > correct. Done
The CQ bit was checked by kcwu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/874083005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ng...) ios_rel_device_ninja_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by kcwu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/874083005/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ad3526512104b5a7352f356b62f8cea3427a58cb Cr-Commit-Position: refs/heads/master@{#314138} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
