Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
406893002:
Fix DCHECKs in V4l2VideoDevice. (Closed)
|
Created:
6 years, 5 months ago by wuchengli Modified:
6 years, 4 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, kcwu1, henryhsu, Owen Lin Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionFix DCHECKs in V4l2VideoDevice.
- sizeimage DCHECK:
The video device can adjust the buffer size to be bigger
than we need as long as the data can fit. Theoretically
sizeimage should be bigger than PlaneAllocationSize. But
Exynos driver has a 256 byte padding for sizeimage. It is
rounded up when calculating coded_size. So sizeimage can
be smaller than PlaneAllocationSize. For example, say
format.fmt.pix_mp.width and height are 1920x1088.
format.fmt.pix_mp.plane_fmt[0].sizeimage is 2089216
(1920*1088+256). |sizeimage| is rounded up and |coded_size|
becomes 1920x1089. So format.fmt.pix_mp.plane_fmt[i].sizeimage
(2089216) is less than PlaneAllocationSize (1920*1090=2092800).
Also, we should not round down because the coded size is passed
VideoEncodeAccelerator::Client::RequireBitstreamBuffers.
The client will allocate buffers according to the coded size.
- bytesperline DCHECK:
bytesperline of different planes should be checked against
media::VideoFrame::row_bytes, not the width of coded size.
BUG=387701
TEST=Run Hangout in debug builds.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291374
Patch Set 1 #
Total comments: 6
Patch Set 2 : address comments #
Total comments: 1
Patch Set 3 : rebase #Patch Set 4 : add one DCHECK back #
Total comments: 6
Patch Set 5 : change DCHECK_LE to DCHECK_EQ #Patch Set 6 : update comments of IsValidPlane #
Total comments: 12
Patch Set 7 : address review comments #
Total comments: 7
Patch Set 8 : address Pawel's review comments #Patch Set 9 : rebase #
Messages
Total messages: 30 (0 generated)
PTAL
https://codereview.chromium.org/406893002/diff/1/content/common/gpu/media/v4l... File content/common/gpu/media/v4l2_video_device.cc (left): https://codereview.chromium.org/406893002/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_device.cc:129: format.fmt.pix_mp.plane_fmt[i].sizeimage, Perhaps we could keep the checks, but make them DCHECK_GE, i.e. sizeimage should be able to fit each plane. https://codereview.chromium.org/406893002/diff/1/content/common/gpu/media/v4l... File content/common/gpu/media/v4l2_video_device.cc (right): https://codereview.chromium.org/406893002/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_device.cc:111: << ", visible_size=" << visible_size.width() Nit: visible_size.ToString() https://codereview.chromium.org/406893002/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_device.cc:113: << ", frame_format=" << frame_format Nit: VideoFrame::FormatToString(frame_format)
PTAL https://codereview.chromium.org/406893002/diff/1/content/common/gpu/media/v4l... File content/common/gpu/media/v4l2_video_device.cc (left): https://codereview.chromium.org/406893002/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_device.cc:129: format.fmt.pix_mp.plane_fmt[i].sizeimage, On 2014/07/24 05:24:59, Pawel Osciak wrote: > Perhaps we could keep the checks, but make them DCHECK_GE, i.e. sizeimage should > be able to fit each plane. As mentioned in bug 387701, the extra 256-byte padding makes coded size bigger. So sizeimage will be less than allocation size. For the next DCHECK, bytesperline could be smaller than coded width for UV planes. https://codereview.chromium.org/406893002/diff/1/content/common/gpu/media/v4l... File content/common/gpu/media/v4l2_video_device.cc (right): https://codereview.chromium.org/406893002/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_device.cc:111: << ", visible_size=" << visible_size.width() On 2014/07/24 05:24:59, Pawel Osciak wrote: > Nit: visible_size.ToString() Done. https://codereview.chromium.org/406893002/diff/1/content/common/gpu/media/v4l... content/common/gpu/media/v4l2_video_device.cc:113: << ", frame_format=" << frame_format On 2014/07/24 05:24:59, Pawel Osciak wrote: > Nit: VideoFrame::FormatToString(frame_format) Done.
lgtm https://codereview.chromium.org/406893002/diff/20001/content/common/gpu/media... File content/common/gpu/media/v4l2_video_device.cc (right): https://codereview.chromium.org/406893002/diff/20001/content/common/gpu/media... content/common/gpu/media/v4l2_video_device.cc:127: DVLOG(3) << "coded_size=" << coded_size.width() << "x" << coded_size.height(); nit: coded_size.ToString()
PTAL. https://codereview.chromium.org/406893002/diff/60001/content/common/gpu/media... File content/common/gpu/media/v4l2_video_device.cc (right): https://codereview.chromium.org/406893002/diff/60001/content/common/gpu/media... content/common/gpu/media/v4l2_video_device.cc:134: DCHECK_LE(format.fmt.pix_mp.plane_fmt[i].bytesperline, Pawel. Do you think this should be DCHECK_EQ? If bytesperline is always even, they should be the same.
https://codereview.chromium.org/406893002/diff/60001/content/common/gpu/media... File content/common/gpu/media/v4l2_image_processor.cc (right): https://codereview.chromium.org/406893002/diff/60001/content/common/gpu/media... content/common/gpu/media/v4l2_image_processor.cc:217: DVLOG(3) << __func__; Is this intended for future use or just forgot to remove? (I don't mean to remove, just make sure your intention) https://codereview.chromium.org/406893002/diff/60001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/406893002/diff/60001/media/base/video_frame.h... media/base/video_frame.h:294: static bool IsValidPlane(size_t plane, VideoFrame::Format format); It is not always used to DCHECK(), for instance, HashFrameForTesting
All done. Pawel. PTAL. https://codereview.chromium.org/406893002/diff/60001/content/common/gpu/media... File content/common/gpu/media/v4l2_image_processor.cc (right): https://codereview.chromium.org/406893002/diff/60001/content/common/gpu/media... content/common/gpu/media/v4l2_image_processor.cc:217: DVLOG(3) << __func__; On 2014/08/15 10:19:17, kcwu wrote: > Is this intended for future use or just forgot to remove? (I don't mean to > remove, just make sure your intention) This is added intentional. We often want to know if CodedSizeFromV4L2Format is called by this or other functions. https://codereview.chromium.org/406893002/diff/60001/content/common/gpu/media... File content/common/gpu/media/v4l2_video_device.cc (right): https://codereview.chromium.org/406893002/diff/60001/content/common/gpu/media... content/common/gpu/media/v4l2_video_device.cc:134: DCHECK_LE(format.fmt.pix_mp.plane_fmt[i].bytesperline, On 2014/08/15 09:56:53, wuchengli wrote: > Pawel. Do you think this should be DCHECK_EQ? If bytesperline is always even, > they should be the same. Discussed with Pawel. This should be EQ. https://codereview.chromium.org/406893002/diff/60001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/406893002/diff/60001/media/base/video_frame.h... media/base/video_frame.h:294: static bool IsValidPlane(size_t plane, VideoFrame::Format format); On 2014/08/15 10:19:17, kcwu wrote: > It is not always used to DCHECK(), for instance, HashFrameForTesting Updated the comments.
https://codereview.chromium.org/406893002/diff/100001/content/common/gpu/medi... File content/common/gpu/media/v4l2_video_device.cc (right): https://codereview.chromium.org/406893002/diff/100001/content/common/gpu/medi... content/common/gpu/media/v4l2_video_device.cc:109: DVLOG(3) << "CodedSizeFromV4L2Format: bytesperline=" << bytesperline use __func__ or __FUNCTION__? https://codereview.chromium.org/406893002/diff/100001/content/common/gpu/medi... content/common/gpu/media/v4l2_video_device.cc:137: } I think we still wanted to check sizeimage >= PlaneAllocationSize? Maybe I'm forgetting what the final conclusion that we reached was... https://codereview.chromium.org/406893002/diff/100001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/406893002/diff/100001/media/base/video_frame.... media/base/video_frame.cc:758: NOTREACHED() << "Unsupported video frame format/plane: " << format << "/" We could do FormatToString() here while we are at it... https://codereview.chromium.org/406893002/diff/100001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/406893002/diff/100001/media/base/video_frame.... media/base/video_frame.h:223: // This refers to the bytes representing frame data scanlines without stride We do align the given width though in the impl...
https://codereview.chromium.org/406893002/diff/100001/media/base/video_frame.cc File media/base/video_frame.cc (left): https://codereview.chromium.org/406893002/diff/100001/media/base/video_frame.... media/base/video_frame.cc:762: int VideoFrame::rows(size_t plane) const { Can we make rows function as static also?
PTAL. https://codereview.chromium.org/406893002/diff/100001/content/common/gpu/medi... File content/common/gpu/media/v4l2_video_device.cc (right): https://codereview.chromium.org/406893002/diff/100001/content/common/gpu/medi... content/common/gpu/media/v4l2_video_device.cc:109: DVLOG(3) << "CodedSizeFromV4L2Format: bytesperline=" << bytesperline On 2014/08/18 07:52:03, Pawel Osciak wrote: > use __func__ or __FUNCTION__? Done. https://codereview.chromium.org/406893002/diff/100001/content/common/gpu/medi... content/common/gpu/media/v4l2_video_device.cc:137: } On 2014/08/18 07:52:03, Pawel Osciak wrote: > I think we still wanted to check sizeimage >= PlaneAllocationSize? Maybe I'm > forgetting what the final conclusion that we reached was... The conclusion was this should be removed. The problem is the driver has an extra 256-byte padding. sizeimage is rounded up at line 123. So the |coded_size| at is bigger than format.fmt.pix_mp.width and height. The result is sizeimage < PlaneAllocationSize. For example, format.fmt.pix_mp.width and height are 1920x1088. format.fmt.pix_mp.plane_fmt[0].sizeimage is 2089216 (1920*1088+256). |sizeimage| is rounded up and |coded_size| becomes 1920x1089. So format.fmt.pix_mp.plane_fmt[i].sizeimage (2089216) is less than PlaneAllocationSize (1920*1090=2092800). We cannot round down |sizeimage| at line 123 because the client of VEA uses the returned coded size to allocate the buffers. If we round down, the client will allocate buffers smaller than needed. https://codereview.chromium.org/406893002/diff/100001/media/base/video_frame.cc File media/base/video_frame.cc (left): https://codereview.chromium.org/406893002/diff/100001/media/base/video_frame.... media/base/video_frame.cc:762: int VideoFrame::rows(size_t plane) const { On 2014/08/18 08:18:57, henryhsu wrote: > Can we make rows function as static also? That is not related to this CL. You'll need to create your own CL. :) https://codereview.chromium.org/406893002/diff/100001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/406893002/diff/100001/media/base/video_frame.... media/base/video_frame.cc:758: NOTREACHED() << "Unsupported video frame format/plane: " << format << "/" On 2014/08/18 07:52:03, Pawel Osciak wrote: > We could do FormatToString() here while we are at it... If we reach here, the format is invalid and FormatToString won't be able to convert it to a string. https://codereview.chromium.org/406893002/diff/100001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/406893002/diff/100001/media/base/video_frame.... media/base/video_frame.h:223: // This refers to the bytes representing frame data scanlines without stride On 2014/08/18 07:52:03, Pawel Osciak wrote: > We do align the given width though in the impl... It refers only to the stride. The original function comments is at line 237. I'll leave it as is...
Pawel@ PTAL. scherkus@ Owner review for video_frame.cc/h. Thanks.
lgtm % comment nits https://chromiumcodereview.appspot.com/406893002/diff/100001/media/base/video... File media/base/video_frame.h (right): https://chromiumcodereview.appspot.com/406893002/diff/100001/media/base/video... media/base/video_frame.h:223: // This refers to the bytes representing frame data scanlines without stride On 2014/08/18 09:09:00, wuchengli wrote: > On 2014/08/18 07:52:03, Pawel Osciak wrote: > > We do align the given width though in the impl... > It refers only to the stride. The original function comments is at line 237. > I'll leave it as is... The issue is that it used to not take width, so it returned the result for row bytes of actual instance of VF. This method however takes width and aligns it if needed. This is unexpected, especially since this doc says "without padding". So it suggests width is unchanged. Please instead say that width may be aligned to format requirements. https://chromiumcodereview.appspot.com/406893002/diff/120001/media/base/video... File media/base/video_frame.h (right): https://chromiumcodereview.appspot.com/406893002/diff/120001/media/base/video... media/base/video_frame.h:293: // Returns true if |plane| is less than the number of planes for the given I'd say: "Returns true if |plane| is a valid plane number for the given format."
https://codereview.chromium.org/406893002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/v4l2_video_device.cc (right): https://codereview.chromium.org/406893002/diff/120001/content/common/gpu/medi... content/common/gpu/media/v4l2_video_device.cc:84: media::VideoFrame::Format frame_format = media::VideoFrame::UNKNOWN; are we using actual VideoFrames in V4L2 code? it seems like we're adding functions to VideoFrame such as RowBytes() and PlaneHorizontalBitsPerPixel() out of convenience, but I wonder whether we'd be better off having V4L2 code do the calculations themselves instead of having a dependency on VideoFrame code
https://codereview.chromium.org/406893002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/v4l2_video_device.cc (right): https://codereview.chromium.org/406893002/diff/120001/content/common/gpu/medi... content/common/gpu/media/v4l2_video_device.cc:84: media::VideoFrame::Format frame_format = media::VideoFrame::UNKNOWN; On 2014/08/20 17:13:53, scherkus wrote: > are we using actual VideoFrames in V4L2 code? > > it seems like we're adding functions to VideoFrame such as RowBytes() and > PlaneHorizontalBitsPerPixel() out of convenience, but I wonder whether we'd be > better off having V4L2 code do the calculations themselves instead of having a > dependency on VideoFrame code V4L2 code use actual VideoFrames like v4l2_image_process.cc and v4l2_video_encode_accelerator.cc. Other files in content/common/gpu/media also use actual VideoFrame. So the dependency already exists. RowBytes() uses the code of existing row_bytes(), which is used by several different places. row_bytes() is not a new function. Duplicating it here is not so good. As for PlaneHorizontalBitsPerPixel, its implementation is like VideoFrame::PlaneAllocationSize, which is used by several other places. Putting it in VideoFrame looks right.
lgtm w/ nits https://codereview.chromium.org/406893002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/v4l2_video_device.cc (right): https://codereview.chromium.org/406893002/diff/120001/content/common/gpu/medi... content/common/gpu/media/v4l2_video_device.cc:84: media::VideoFrame::Format frame_format = media::VideoFrame::UNKNOWN; On 2014/08/21 01:03:57, wuchengli wrote: > On 2014/08/20 17:13:53, scherkus wrote: > > are we using actual VideoFrames in V4L2 code? > > > > it seems like we're adding functions to VideoFrame such as RowBytes() and > > PlaneHorizontalBitsPerPixel() out of convenience, but I wonder whether we'd be > > better off having V4L2 code do the calculations themselves instead of having a > > dependency on VideoFrame code > V4L2 code use actual VideoFrames like v4l2_image_process.cc and > v4l2_video_encode_accelerator.cc. Other files in content/common/gpu/media also > use actual VideoFrame. So the dependency already exists. > > RowBytes() uses the code of existing row_bytes(), which is used by several > different places. row_bytes() is not a new function. Duplicating it here is not > so good. > > As for PlaneHorizontalBitsPerPixel, its implementation is like > VideoFrame::PlaneAllocationSize, which is used by several other places. Putting > it in VideoFrame looks right. Thanks for the pointers. I'm a little wary of making VideoFrame a big dumping ground of utility functions ... it might be worth thinking about splitting things into a specific utility file vs. the class. We don't have to do this now, though. https://codereview.chromium.org/406893002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/v4l2_video_encode_accelerator.cc (right): https://codereview.chromium.org/406893002/diff/120001/content/common/gpu/medi... content/common/gpu/media/v4l2_video_encode_accelerator.cc:863: DVLOG(3) << "NegotiateInputFormat()"; use __func__ ?
On 2014/08/21 21:37:54, scherkus wrote: > lgtm w/ nits > > https://codereview.chromium.org/406893002/diff/120001/content/common/gpu/medi... > File content/common/gpu/media/v4l2_video_device.cc (right): > > https://codereview.chromium.org/406893002/diff/120001/content/common/gpu/medi... > content/common/gpu/media/v4l2_video_device.cc:84: media::VideoFrame::Format > frame_format = media::VideoFrame::UNKNOWN; > On 2014/08/21 01:03:57, wuchengli wrote: > > On 2014/08/20 17:13:53, scherkus wrote: > > > are we using actual VideoFrames in V4L2 code? > > > > > > it seems like we're adding functions to VideoFrame such as RowBytes() and > > > PlaneHorizontalBitsPerPixel() out of convenience, but I wonder whether we'd > be > > > better off having V4L2 code do the calculations themselves instead of having > a > > > dependency on VideoFrame code > > V4L2 code use actual VideoFrames like v4l2_image_process.cc and > > v4l2_video_encode_accelerator.cc. Other files in content/common/gpu/media also > > use actual VideoFrame. So the dependency already exists. > > > > RowBytes() uses the code of existing row_bytes(), which is used by several > > different places. row_bytes() is not a new function. Duplicating it here is > not > > so good. > > > > As for PlaneHorizontalBitsPerPixel, its implementation is like > > VideoFrame::PlaneAllocationSize, which is used by several other places. > Putting > > it in VideoFrame looks right. > > Thanks for the pointers. > > I'm a little wary of making VideoFrame a big dumping ground of utility functions > ... it might be worth thinking about splitting things into a specific utility > file vs. the class. We don't have to do this now, though. I'll keep this in mind. Thanks. > > https://codereview.chromium.org/406893002/diff/120001/content/common/gpu/medi... > File content/common/gpu/media/v4l2_video_encode_accelerator.cc (right): > > https://codereview.chromium.org/406893002/diff/120001/content/common/gpu/medi... > content/common/gpu/media/v4l2_video_encode_accelerator.cc:863: DVLOG(3) << > "NegotiateInputFormat()"; > use __func__ ?
All done. Submitting. https://codereview.chromium.org/406893002/diff/100001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/406893002/diff/100001/media/base/video_frame.... media/base/video_frame.h:223: // This refers to the bytes representing frame data scanlines without stride On 2014/08/20 10:46:06, Pawel Osciak wrote: > On 2014/08/18 09:09:00, wuchengli wrote: > > On 2014/08/18 07:52:03, Pawel Osciak wrote: > > > We do align the given width though in the impl... > > It refers only to the stride. The original function comments is at line 237. > > I'll leave it as is... > > The issue is that it used to not take width, so it returned the result for row > bytes of actual instance of VF. > This method however takes width and aligns it if needed. This is unexpected, > especially since this doc says "without padding". So it suggests width is > unchanged. Please instead say that width may be aligned to format requirements. Done. https://codereview.chromium.org/406893002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/v4l2_video_encode_accelerator.cc (right): https://codereview.chromium.org/406893002/diff/120001/content/common/gpu/medi... content/common/gpu/media/v4l2_video_encode_accelerator.cc:863: DVLOG(3) << "NegotiateInputFormat()"; On 2014/08/21 21:37:54, scherkus wrote: > use __func__ ? Most functions in this file use a plain string for DVLOG. I'll leave the clean up for someone else. https://codereview.chromium.org/406893002/diff/120001/media/base/video_frame.h File media/base/video_frame.h (right): https://codereview.chromium.org/406893002/diff/120001/media/base/video_frame.... media/base/video_frame.h:293: // Returns true if |plane| is less than the number of planes for the given On 2014/08/20 10:46:06, Pawel Osciak wrote: > I'd say: "Returns true if |plane| is a valid plane number for the given format." Done.
The CQ bit was checked by wuchengli@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wuchengli@chromium.org/406893002/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by wuchengli@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wuchengli@chromium.org/406893002/160001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by wuchengli@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wuchengli@chromium.org/406893002/160001
Message was sent while issue was closed.
Committed patchset #9 (160001) as 291374 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
