Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2369093002:
Fixing repeated pixels when drawing HTML5 video to canvas. (Closed)
|
Created:
4 years, 2 months ago by zakerinasab1 Modified:
4 years, 2 months ago CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixing repeated pixels when drawing HTML5 video to canvas.
When the coded size of the video frame is different from the visible
rect, SkCanvasVideoRenderer::Paint tries to scale the video while it
must ignore the pixels outside visible rect. This is addressed by
clipping the SkImage object created from VideoFrame prior to painting
the frame on the canvas.
BUG=611330
Committed: https://crrev.com/feb2e37e78f375218f842448db94eae13254b536
Cr-Commit-Position: refs/heads/master@{#423688}
Patch Set 1 #Patch Set 2 #Patch Set 3 #Patch Set 4 : Minor corrections in unit test. #Patch Set 5 : Addressing comments. #Patch Set 6 : Minor corrections in unit test. #
Messages
Total messages: 53 (26 generated)
Description was changed from ========== Fixing repeated pixels when drawing HTML5 video to canvas. BUG=611330 ========== to ========== When the coded size of the video frame is different from the visible rect, SkCanvasVideoRenderer::Paint tries to scale the video while it must just ignore the pixels outside visible rect. This is addressed by clipping the SkImage object created from VideoFrame prior to painting the frame on the canvas. BUG=611330 ==========
zakerinasab@google.com changed reviewers: + hubbe@chromium.org, junov@chromium.org
Description was changed from ========== When the coded size of the video frame is different from the visible rect, SkCanvasVideoRenderer::Paint tries to scale the video while it must just ignore the pixels outside visible rect. This is addressed by clipping the SkImage object created from VideoFrame prior to painting the frame on the canvas. BUG=611330 ========== to ========== When the coded size of the video frame is different from the visible rect, SkCanvasVideoRenderer::Paint tries to scale the video while it must ignore the pixels outside visible rect. This is addressed by clipping the SkImage object created from VideoFrame prior to painting the frame on the canvas. BUG=611330 ==========
zakerinasab@google.com changed reviewers: - hubbe@chromium.org
Description was changed from ========== When the coded size of the video frame is different from the visible rect, SkCanvasVideoRenderer::Paint tries to scale the video while it must ignore the pixels outside visible rect. This is addressed by clipping the SkImage object created from VideoFrame prior to painting the frame on the canvas. BUG=611330 ========== to ========== When the coded size of the video frame is different from the visible rect, SkCanvasVideoRenderer::Paint tries to scale the video while it must ignore the pixels outside visible rect. This is addressed by clipping the SkImage object created from VideoFrame prior to painting the frame on the canvas. BUG=611330 P.S. This CL does not have a layout test or unit test. The reason is that the error happens when the container is mp4. Other containers like ogv work just fine. Mpeg-4 container is not supported in debug builds and layout tests are tested only in debug builds. Hence, we cannot use a layout test. Also to write a unit test we can create a fake HTMLVideoElement but then again we need the video source to go through the code path to check if the code is working correctly. ==========
Description was changed from ========== When the coded size of the video frame is different from the visible rect, SkCanvasVideoRenderer::Paint tries to scale the video while it must ignore the pixels outside visible rect. This is addressed by clipping the SkImage object created from VideoFrame prior to painting the frame on the canvas. BUG=611330 P.S. This CL does not have a layout test or unit test. The reason is that the error happens when the container is mp4. Other containers like ogv work just fine. Mpeg-4 container is not supported in debug builds and layout tests are tested only in debug builds. Hence, we cannot use a layout test. Also to write a unit test we can create a fake HTMLVideoElement but then again we need the video source to go through the code path to check if the code is working correctly. ========== to ========== When the coded size of the video frame is different from the visible rect, SkCanvasVideoRenderer::Paint tries to scale the video while it must ignore the pixels outside visible rect. This is addressed by clipping the SkImage object created from VideoFrame prior to painting the frame on the canvas. BUG=611330 P.S. This CL does not have a layout test or unit test. The reason is that the error happens when the container is mp4. Other containers like ogv work just fine. Mpeg-4 container is not supported in debug builds and layout tests are tested only in debug builds. Hence, we cannot use a layout test. Also to write a unit test we can create a fake HTMLVideoElement but then again we need the video source to go through the code path to check if the code is working correctly. ==========
Patch submitted.
Description was changed from ========== When the coded size of the video frame is different from the visible rect, SkCanvasVideoRenderer::Paint tries to scale the video while it must ignore the pixels outside visible rect. This is addressed by clipping the SkImage object created from VideoFrame prior to painting the frame on the canvas. BUG=611330 P.S. This CL does not have a layout test or unit test. The reason is that the error happens when the container is mp4. Other containers like ogv work just fine. Mpeg-4 container is not supported in debug builds and layout tests are tested only in debug builds. Hence, we cannot use a layout test. Also to write a unit test we can create a fake HTMLVideoElement but then again we need the video source to go through the code path to check if the code is working correctly. ========== to ========== When the coded size of the video frame is different from the visible rect, SkCanvasVideoRenderer::Paint tries to scale the video while it must ignore the pixels outside visible rect. This is addressed by clipping the SkImage object created from VideoFrame prior to painting the frame on the canvas. BUG=611330 ==========
zakerinasab@chromium.org changed reviewers: + hubbe@chromium.org
@hubbe How we should write a layout test for this CL? The error happens when the container is mp4. Other containers like ogv work fine. Mpeg-4 container is not supported in chromium and AFAIK trybots run chromium.
On 2016/09/27 14:55:12, zakerinasab wrote: > @hubbe > > How we should write a layout test for this CL? The error happens when the > container is mp4. Other containers like ogv work fine. Mpeg-4 container is not > supported in chromium and AFAIK trybots run chromium. Just make a unit test where you inject frames that you constructed yourself?
On 2016/09/27 15:43:55, hubbe wrote: > On 2016/09/27 14:55:12, zakerinasab wrote: > > @hubbe > > > > How we should write a layout test for this CL? The error happens when the > > container is mp4. Other containers like ogv work fine. Mpeg-4 container is not > > supported in chromium and AFAIK trybots run chromium. > > Just make a unit test where you inject frames that you constructed yourself? hm. Let me try that. I was thinking about using the video inside the unit test that had the same MP4 container support issue. But I think I can use fake frames.
I tried to add a unit test to create a fake hardware backed video frame and then pass it to the SkCanvasVideoRenderer::Paint. It seems to be hard to do as we don't have access to header files from platform/. Please see the unit test file. The code added to SkCanvasRenderer works fine and resolves the bug and also does not cause any problem for other unit tests.
You need to line-wrap the description. The first line of the description should be the summary. Don't be afraid to modify skcanvas_video_renderer.cc (to handle null cases, for example) to work with your unit test.
Description was changed from ========== When the coded size of the video frame is different from the visible rect, SkCanvasVideoRenderer::Paint tries to scale the video while it must ignore the pixels outside visible rect. This is addressed by clipping the SkImage object created from VideoFrame prior to painting the frame on the canvas. BUG=611330 ========== to ========== Fixing repeated pixels when drawing HTML5 video to canvas. When the coded size of the video frame is different from the visible rect, SkCanvasVideoRenderer::Paint tries to scale the video while it must ignore the pixels outside visible rect. This is addressed by clipping the SkImage object created from VideoFrame prior to painting the frame on the canvas. BUG=611330 ==========
New patch submitted.
Not an owner, but this looks reasonable to me.
You still need to limit line length to 72 characters in the description.
Description was changed from ========== Fixing repeated pixels when drawing HTML5 video to canvas. When the coded size of the video frame is different from the visible rect, SkCanvasVideoRenderer::Paint tries to scale the video while it must ignore the pixels outside visible rect. This is addressed by clipping the SkImage object created from VideoFrame prior to painting the frame on the canvas. BUG=611330 ========== to ========== Fixing repeated pixels when drawing HTML5 video to canvas. When the coded size of the video frame is different from the visible rect, SkCanvasVideoRenderer::Paint tries to scale the video while it must ignore the pixels outside visible rect. This is addressed by clipping the SkImage object created from VideoFrame prior to painting the frame on the canvas. BUG=611330 ==========
On 2016/10/03 18:05:11, Justin Novosad wrote: > You still need to limit line length to 72 characters in the description. done.
non-owner lgtm
zakerinasab@chromium.org changed reviewers: + sandersd@chromium.org
zakerinasab@chromium.org changed reviewers: + zakerinasab@chromium.org
@hubbe @sandersd Can you look at this CL? Thanks.
On 2016/10/05 18:35:38, zakerinasab wrote: > @hubbe > @sandersd > > Can you look at this CL? Thanks. Why does this CL explicitly throw away the origin of the visible rect? If there is no way to crop correctly, then the next best option is to use the bounds of the crop instead (x + width, y + height).
On 2016/10/05 19:12:26, sandersd wrote: > On 2016/10/05 18:35:38, zakerinasab wrote: > > @hubbe > > @sandersd > > > > Can you look at this CL? Thanks. > > Why does this CL explicitly throw away the origin of the visible rect? > > If there is no way to crop correctly, then the next best option is to use the > bounds of the crop instead (x + width, y + height). Fixed and new CL uploaded.
On 2016/10/05 19:47:11, zakerinasab1 wrote: > On 2016/10/05 19:12:26, sandersd wrote: > > On 2016/10/05 18:35:38, zakerinasab wrote: > > > @hubbe > > > @sandersd > > > > > > Can you look at this CL? Thanks. > > > > Why does this CL explicitly throw away the origin of the visible rect? > > > > If there is no way to crop correctly, then the next best option is to use the > > bounds of the crop instead (x + width, y + height). > > Fixed and new CL uploaded. lgtm. FYI I started documenting some of these things (specifically for H.264) a few weeks ago in https://docs.google.com/document/d/189waiorF7xvbk41hM5XCG1FKk3ocPWJDbyFobEC3f... Let me know if you want any test files in particular.
On 2016/10/05 20:29:07, sandersd wrote: > On 2016/10/05 19:47:11, zakerinasab1 wrote: > > On 2016/10/05 19:12:26, sandersd wrote: > > > On 2016/10/05 18:35:38, zakerinasab wrote: > > > > @hubbe > > > > @sandersd > > > > > > > > Can you look at this CL? Thanks. > > > > > > Why does this CL explicitly throw away the origin of the visible rect? > > > > > > If there is no way to crop correctly, then the next best option is to use > the > > > bounds of the crop instead (x + width, y + height). > > > > Fixed and new CL uploaded. > > lgtm. > > FYI I started documenting some of these things (specifically for H.264) a few > weeks ago in > https://docs.google.com/document/d/189waiorF7xvbk41hM5XCG1FKk3ocPWJDbyFobEC3f... > > Let me know if you want any test files in particular. Interesting. Can you give me access to the test files for frame cropping? I'll check how this CL works on them and post the result here before trying to land it.
> Interesting. Can you give me access to the test files for frame cropping? I'll > check how this CL works on them and post the result here before trying to land > it. I went ahead and just made an index page: https://storage.googleapis.com/videostack/testdata/index.html
On 2016/10/05 21:04:55, sandersd wrote: > > Interesting. Can you give me access to the test files for frame cropping? I'll > > check how this CL works on them and post the result here before trying to land > > it. > > I went ahead and just made an index page: > https://storage.googleapis.com/videostack/testdata/index.html I examined the frame cropping videos. It seems that the current patch resolves the problem for 3 out of 9 and reduces the problem for other 3 out of 9. I'll go ahead and try to land this one. Is there any bug filed for this cropping problem? Chromium: Chromium 55.0.2880.0 + patch + hw acceleration Player 1: ffplay version N-81696-gd38dff8e Player 2: MPC-HC 10.0 Chromium Player 1 Player 2 ------------------------------------------------------------- bottom_left.mp4 L L L bottom_right.mp4 --- --- --- bottom_right_cc.mp4 BR --- --- bottom_right_cc_top_right.mp4 T --- T bottom_right_huge.mp4 --- --- --- right_huge.mp4 --- --- --- top_bottom_left_right.mp4 TL L TL top_left.mp4 TL L TL top_right.mp4 T --- T
The CQ bit was checked by zakerinasab@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org Link to the patchset: https://codereview.chromium.org/2369093002/#ps80001 (title: "Addressing comments.")
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
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by zakerinasab@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org, sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/2369093002/#ps100001 (title: "Fixing minor error in unittest.")
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
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> Chromium: Chromium 55.0.2880.0 + patch + hw acceleration > Player 1: ffplay version N-81696-gd38dff8e > Player 2: MPC-HC 10.0 > Chromium Player 1 Player 2 > ------------------------------------------------------------- > bottom_left.mp4 L L L > bottom_right.mp4 --- --- --- > bottom_right_cc.mp4 BR --- --- > bottom_right_cc_top_right.mp4 T --- T > bottom_right_huge.mp4 --- --- --- > right_huge.mp4 --- --- --- > top_bottom_left_right.mp4 TL L TL > top_left.mp4 TL L TL > top_right.mp4 T --- T The next FFmpeg roll will be move us from FFmpeg 2.x to 3.0; this will resolve about half of the issues (left crops will still not work correctly). There is a second compositing bug that results in RGB textures using the (x + w, y + h) size, which affects hardware decoded frames. I will be fixing that within the next few weeks (didn't file a bug yet though). The important thing is that your new code is ready for those change :-)
Patchset #9 (id:160001) has been deleted
Patchset #8 (id:140001) has been deleted
Patchset #7 (id:120001) has been deleted
Patchset #6 (id:100001) has been deleted
The CQ bit was checked by zakerinasab@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from junov@chromium.org, sandersd@chromium.org Link to the patchset: https://codereview.chromium.org/2369093002/#ps180001 (title: "Minor corrections in unit test.")
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 ========== Fixing repeated pixels when drawing HTML5 video to canvas. When the coded size of the video frame is different from the visible rect, SkCanvasVideoRenderer::Paint tries to scale the video while it must ignore the pixels outside visible rect. This is addressed by clipping the SkImage object created from VideoFrame prior to painting the frame on the canvas. BUG=611330 ========== to ========== Fixing repeated pixels when drawing HTML5 video to canvas. When the coded size of the video frame is different from the visible rect, SkCanvasVideoRenderer::Paint tries to scale the video while it must ignore the pixels outside visible rect. This is addressed by clipping the SkImage object created from VideoFrame prior to painting the frame on the canvas. BUG=611330 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Fixing repeated pixels when drawing HTML5 video to canvas. When the coded size of the video frame is different from the visible rect, SkCanvasVideoRenderer::Paint tries to scale the video while it must ignore the pixels outside visible rect. This is addressed by clipping the SkImage object created from VideoFrame prior to painting the frame on the canvas. BUG=611330 ========== to ========== Fixing repeated pixels when drawing HTML5 video to canvas. When the coded size of the video frame is different from the visible rect, SkCanvasVideoRenderer::Paint tries to scale the video while it must ignore the pixels outside visible rect. This is addressed by clipping the SkImage object created from VideoFrame prior to painting the frame on the canvas. BUG=611330 Committed: https://crrev.com/feb2e37e78f375218f842448db94eae13254b536 Cr-Commit-Position: refs/heads/master@{#423688} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/feb2e37e78f375218f842448db94eae13254b536 Cr-Commit-Position: refs/heads/master@{#423688} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
