Compare the full rect in SkCanvasVideoRenderer::Paint for transform.

Compare the full rect in SkCanvasVideoRenderer::Paint for transform.

When the rects are not the same, transform the output.

R=dalecurtis@chromium.org

Committed: https://crrev.com/0786ff643d5e17434c668c4dab04d264b57dd41d
Cr-Commit-Position: refs/heads/master@{#356159}

[danakj, 2015-09-24 18:10:53]

I'm not sure this is really right tho. The code in needs_transform does not use
the visible_rect's origin at all, so why do we need to compare it? Meanwhile it
already checks if the dest_rect origin is 0,0 or not, and just translates by
that inside the needs_transform block. WDYT?

[DaleCurtis, 2015-09-24 18:52:13]

dalecurtis@chromium.org changed reviewers:
+ dcastagna@chromium.org

[DaleCurtis, 2015-09-24 18:52:13]

+dcastagna who's worked on this recently :)

I believe it's using the visible rect implicitly through last_image_ (see how
VideoImageGenerator is passed into SkImage::NewFromGenerator()). Provided this
code is actually doing anything (delete and see if tests fail?) -- I think the
code should just be "dest_rect != video_frame->visible_rect()"

[danakj, 2015-09-24 19:03:46]

I deleted the check entirely and media tests fail. But they pass with rect
compares.

2 tests failed:
    SkCanvasVideoRendererTest.CroppedFrame
(../../media/blink/skcanvas_video_renderer_unittest.cc:321)
    SkCanvasVideoRendererTest.CroppedFrame_NoScaling
(../../media/blink/skcanvas_video_renderer_unittest.cc:342)

[Daniele Castagna, 2015-09-24 19:13:00]

On 2015/09/24 at 19:03:46, danakj wrote:
> I deleted the check entirely and media tests fail. But they pass with rect
compares.
> 
> 2 tests failed:
>     SkCanvasVideoRendererTest.CroppedFrame
(../../media/blink/skcanvas_video_renderer_unittest.cc:321)
>     SkCanvasVideoRendererTest.CroppedFrame_NoScaling
(../../media/blink/skcanvas_video_renderer_unittest.cc:342)

VideoFrames coming from VpxDecoder and FfmpegDecoder seem to always have
visible_rect().origin() set to (0,0). When is that set differently?

[danakj, 2015-09-24 20:30:51]

On 2015/09/24 19:13:00, Daniele Castagna wrote:
> On 2015/09/24 at 19:03:46, danakj wrote:
> > I deleted the check entirely and media tests fail. But they pass with rect
> compares.
> > 
> > 2 tests failed:
> >     SkCanvasVideoRendererTest.CroppedFrame
> (../../media/blink/skcanvas_video_renderer_unittest.cc:321)
> >     SkCanvasVideoRendererTest.CroppedFrame_NoScaling
> (../../media/blink/skcanvas_video_renderer_unittest.cc:342)
> 
> VideoFrames coming from VpxDecoder and FfmpegDecoder seem to always have
> visible_rect().origin() set to (0,0). When is that set differently?

Then visible_rect() should probably be a visible_size(). In media tests its
non-origin tho.

[ RUN      ] SkCanvasVideoRendererTest.CroppedFrame
[20656:20656:0924/132935:1217356822193:FATAL:video_frame.cc(898)] Check failed:
visible_rect.origin().IsOrigin(). 
#0 0x7fd6ff19c17e base::debug::StackTrace::StackTrace()
#1 0x7fd6ff0b311f logging::LogMessage::~LogMessage()
#2 0x7fd6fffacf94 media::VideoFrame::CreateFrameInternal()
#3 0x7fd6ffface4b media::VideoFrame::CreateFrame()
#4 0x0000004d2d0a media::SkCanvasVideoRendererTest::SkCanvasVideoRendererTest()
#5 0x0000004da94f
media::SkCanvasVideoRendererTest_CroppedFrame_Test::SkCanvasVideoRendererTest_CroppedFrame_Test()

for example. The test makes frames with non-0,0 visible rects.

[Daniele Castagna, 2015-09-25 00:39:59]

On 2015/09/24 at 20:30:51, danakj wrote:
> On 2015/09/24 19:13:00, Daniele Castagna wrote:
> > On 2015/09/24 at 19:03:46, danakj wrote:
> > > I deleted the check entirely and media tests fail. But they pass with rect
> > compares.
> > > 
> > > 2 tests failed:
> > >     SkCanvasVideoRendererTest.CroppedFrame
> > (../../media/blink/skcanvas_video_renderer_unittest.cc:321)
> > >     SkCanvasVideoRendererTest.CroppedFrame_NoScaling
> > (../../media/blink/skcanvas_video_renderer_unittest.cc:342)
> > 
> > VideoFrames coming from VpxDecoder and FfmpegDecoder seem to always have
> > visible_rect().origin() set to (0,0). When is that set differently?
> 
> Then visible_rect() should probably be a visible_size(). In media tests its
non-origin tho.
> 
> [ RUN      ] SkCanvasVideoRendererTest.CroppedFrame
> [20656:20656:0924/132935:1217356822193:FATAL:video_frame.cc(898)] Check
failed: visible_rect.origin().IsOrigin(). 
> #0 0x7fd6ff19c17e base::debug::StackTrace::StackTrace()
> #1 0x7fd6ff0b311f logging::LogMessage::~LogMessage()
> #2 0x7fd6fffacf94 media::VideoFrame::CreateFrameInternal()
> #3 0x7fd6ffface4b media::VideoFrame::CreateFrame()
> #4 0x0000004d2d0a
media::SkCanvasVideoRendererTest::SkCanvasVideoRendererTest()
> #5 0x0000004da94f
media::SkCanvasVideoRendererTest_CroppedFrame_Test::SkCanvasVideoRendererTest_CroppedFrame_Test()
> 
> for example. The test makes frames with non-0,0 visible rects.

After further investigation, origin of video_frame.visible_rect can be non-0,0.
This can happen when we capture with an aspect ratio different than the camera
capture aspect ratio (look at the end of
VideoTrackAdapter::VideoFrameResolutionAdapter::DeliverFrame).

Said so, the check was correct, even though confusing, as it was before.

last_image_ is already cropped using visible_rect when we do the check, so
visible_rect.origin is irrelevant at that point.

Maybe a less confusing check could be:

const bool need_transform =
      video_rotation != VIDEO_ROTATION_0 ||  // Need to be rotated.
      dest_rect.size() != last_image_->dimensions() ||  // Need to be scaled.
      !dest_rect.origin().IsOrigin();  // Need to be translated.

[danakj, 2015-10-26 20:43:22]

On 2015/09/25 00:39:59, Daniele Castagna wrote:
> On 2015/09/24 at 20:30:51, danakj wrote:
> > On 2015/09/24 19:13:00, Daniele Castagna wrote:
> > > On 2015/09/24 at 19:03:46, danakj wrote:
> > > > I deleted the check entirely and media tests fail. But they pass with
rect
> > > compares.
> > > > 
> > > > 2 tests failed:
> > > >     SkCanvasVideoRendererTest.CroppedFrame
> > > (../../media/blink/skcanvas_video_renderer_unittest.cc:321)
> > > >     SkCanvasVideoRendererTest.CroppedFrame_NoScaling
> > > (../../media/blink/skcanvas_video_renderer_unittest.cc:342)
> > > 
> > > VideoFrames coming from VpxDecoder and FfmpegDecoder seem to always have
> > > visible_rect().origin() set to (0,0). When is that set differently?
> > 
> > Then visible_rect() should probably be a visible_size(). In media tests its
> non-origin tho.
> > 
> > [ RUN      ] SkCanvasVideoRendererTest.CroppedFrame
> > [20656:20656:0924/132935:1217356822193:FATAL:video_frame.cc(898)] Check
> failed: visible_rect.origin().IsOrigin(). 
> > #0 0x7fd6ff19c17e base::debug::StackTrace::StackTrace()
> > #1 0x7fd6ff0b311f logging::LogMessage::~LogMessage()
> > #2 0x7fd6fffacf94 media::VideoFrame::CreateFrameInternal()
> > #3 0x7fd6ffface4b media::VideoFrame::CreateFrame()
> > #4 0x0000004d2d0a
> media::SkCanvasVideoRendererTest::SkCanvasVideoRendererTest()
> > #5 0x0000004da94f
>
media::SkCanvasVideoRendererTest_CroppedFrame_Test::SkCanvasVideoRendererTest_CroppedFrame_Test()
> > 
> > for example. The test makes frames with non-0,0 visible rects.
> 
> After further investigation, origin of video_frame.visible_rect can be
non-0,0.
> This can happen when we capture with an aspect ratio different than the camera
> capture aspect ratio (look at the end of
> VideoTrackAdapter::VideoFrameResolutionAdapter::DeliverFrame).
> 
> Said so, the check was correct, even though confusing, as it was before.
> 
> last_image_ is already cropped using visible_rect when we do the check, so
> visible_rect.origin is irrelevant at that point.
> 
> Maybe a less confusing check could be:
> 
> const bool need_transform =
>       video_rotation != VIDEO_ROTATION_0 ||  // Need to be rotated.
>       dest_rect.size() != last_image_->dimensions() ||  // Need to be scaled.

This is a SizeF on the left and a Size on the right. Since this came up in the
context of converting ints to floats, I thought I should point that out.

>       !dest_rect.origin().IsOrigin();  // Need to be translated.

[danakj, 2015-10-26 20:44:29]

PTAL

[DaleCurtis, 2015-10-26 20:45:55]

lgtm

[DaleCurtis, 2015-10-26 20:45:55]

lgtm

[Daniele Castagna, 2015-10-26 21:02:39]

lgtm

[danakj, 2015-10-26 21:03:23]

The CQ bit was checked by danakj@chromium.org

[danakj, 2015-10-26 21:03:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
dcastagna@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1369563002/#ps40001
(title: "sizemedia: rebase")

[commit-bot: I haz the power, 2015-10-26 21:04:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1369563002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1369563002/40001

[commit-bot: I haz the power, 2015-10-26 22:48:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-26 22:49:12]

Patchset 3 (id:??) landed as
https://crrev.com/0786ff643d5e17434c668c4dab04d264b57dd41d
Cr-Commit-Position: refs/heads/master@{#356159}