DCHECK failures with odd video frame size |
|||||||
Issue descriptionVersion: master, #411947 OS: goobuntu What steps will reproduce the problem? (1) Open https://webrtc.github.io/samples/src/content/peerconnection/constraints/ (2) Move the "Max height" slider to 239px. (3) Press Get media What is the expected output? Display of an 640 x 239 video. What do you see instead? A crash, with [1:12:0818/134332:FATAL:gpu_memory_buffer_video_frame_pool.cc(322)] Check failed: (video_frame->visible_rect().y() & 1) == 0. I have seen two different problems, one is the DCHECK above, and the corresponding .x() & 1. I tried simply deleting them, and that improves the situation. I haven't found the code that uses visible_rect().origin(), but I think the right thing to do is simply to round the x and y values down to even when used. Second problem is if input frames are 640 x 239 already at arrival to the WebRtcVideoCapturerAdapter (this seems to depend on in which order GetUserMedia with different constraints are used). In this case, we get frames where coded_size().width() and visible_rect().width are both 239. Then we get a different DCHECK failure in GpuMemoryBufferVideoFramePool::PoolImpl::CreateHardwareFrame, in the call to CodedSize. There it computes an output size, where dimensions are rounded up to even, so we get output.width == 240. And then the check DCHECK(gfx::Rect(video_frame->coded_size()).Contains(gfx::Rect(output))); fails. Not sure what to do in this case, maybe round down to even rather than up? Or somehow enforce an even coded_size for all i420 video frames.
,
Aug 18 2016
emircan - is this for you or know who best can take a look? Thanks
,
Aug 24 2016
dcastagna@ would be the best to address this within GpuMemoryBufferVideoFramePool. I remember from this CL that we wanted to round up when there is odd: https://codereview.chromium.org/1420943002 I am also confused about DCHECK at l.330
,
Aug 29 2016
,
Oct 3 2016
,
Oct 13 2016
Issue 655362 has been merged into this issue.
,
Jul 7 2017
,
Mar 14 2018
GMBVFP rounds up the output given odd sized frames, which seems to be the convention in libvpx as well. Why is DCHECK necessary there though? Odd sized input is a legit case. https://cs.chromium.org/chromium/src/media/video/gpu_memory_buffer_video_frame_pool.cc?q=gpu_memory_buffer_video_frame_pool.cc&sq=package:chromium&dr&l=511
,
Mar 15 2018
Rounding up to even sounds reasonable to me, it's "just" a matter of doing it consistently everywhere it's needed. E.g., rounding up the output size without also rounding up the coded_size seems to break other assumptions. And note that the DCHECK you point to DCHECK((video_frame->visible_rect().y() & 1) == 0); is not about the size, but the position of the visible rectangle. That might need rounding (down?) as well.
,
Mar 19 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5f46dab1ca16acd2ec18b0c80b5ce0f312279485 commit 5f46dab1ca16acd2ec18b0c80b5ce0f312279485 Author: Emircan Uysaler <emircan@chromium.org> Date: Mon Mar 19 23:06:45 2018 Skip creating hardware frames for odd sized and positioned frames This CL skips HW frame creation for two input cases: - Frames with odd coded_size - Odd positioned visible_rect Bug: 638906 , webrtc:9033 Change-Id: Ie945c4626d236c1c97f2ec55cd67bc00d1d9c909 Reviewed-on: https://chromium-review.googlesource.com/965322 Reviewed-by: Dale Curtis <dalecurtis@chromium.org> Reviewed-by: Daniele Castagna <dcastagna@chromium.org> Commit-Queue: Emircan Uysaler <emircan@chromium.org> Cr-Commit-Position: refs/heads/master@{#544199} [modify] https://crrev.com/5f46dab1ca16acd2ec18b0c80b5ce0f312279485/media/video/gpu_memory_buffer_video_frame_pool.cc [modify] https://crrev.com/5f46dab1ca16acd2ec18b0c80b5ce0f312279485/media/video/gpu_memory_buffer_video_frame_pool_unittest.cc
,
Apr 2 2018
With Emircan CL we're skipping odd coded_size frames and odd positioned visible_rect, so the DCHECK is not being hit anymore. We discussed if it's worth to handle those cases and we decided that they happen so rarely that we can just use VideoResourceUpdater in cc in those cases.
,
Apr 2 2018
I thought the goal was to get rid of VideoResourceUpdater eventually?
,
Apr 2 2018
That was the original goal, when we were hoping to have native GMBs on Windows at one point. It's not clear if we'll ever be able to have native GMBs on Windows with the properties we need though, and/or if we'll switch to GMBs upload on all the platforms. When and if we'll be in a situation where we can get rid of VideoResourceUpdater, we can take care of dealing with the odd positioned visible_rect. The odd coded_sized shouldn't happen in the first pace, and Emircan already filed a bug about that. |
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by nisse@chromium.org
, Aug 18 2016