cc: Support texcoord offsets in YUVVideoDrawQuad

cc: Support texcoord offsets in YUVVideoDrawQuad

YUVVideoDrawQuad currently supports stretching, but not offset, from its source
textures.  Add support for offset.

Also: fix VideoGLRendererPixelTest to upload subsampled chroma planes at the
correct (half) size.  Use patterned test textures to detect texcoord
misalignment.

BUG=349450
TEST=local build, run unittests on desktop Linux

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259831

[sheu, 2014-03-20 23:34:34]

perkj@: this is the offset support CL you'll be wanting :-)

danakj@: PTAL, thanks!  Also fixes existing test bug :-)

[perkj_chrome, 2014-03-21 14:48:34]

On 2014/03/20 23:34:34, sheu wrote:
> perkj@: this is the offset support CL you'll be wanting :-)
> 
> danakj@: PTAL, thanks!  Also fixes existing test bug :-)

Seems to work like a charm, thanks!

But it seems like you have forgot to remove the  asserts in 
bool VideoResourceUpdater::VerifyFrame(
DCHECK_EQ(video_frame->visible_rect().x(), 0);
DCHECK_EQ(video_frame->visible_rect().y(), 0);

[danakj, 2014-03-21 18:11:10]

https://chromiumcodereview.appspot.com/207233002/diff/20001/cc/output/rendere...
File cc/output/renderer_pixeltest.cc (right):

https://chromiumcodereview.appspot.com/207233002/diff/20001/cc/output/rendere...
cc/output/renderer_pixeltest.cc:411: const gfx::Rect uv_rect(0, 0, rect.width()
+ 1 / 2, rect.height() + 1 / 2);
a + 1 / 2 == a + (1/2) == a + 0

for integer division. what's going on here? is the test broken or is this
supposed to be a no-op change to rect.width and height?

https://chromiumcodereview.appspot.com/207233002/diff/20001/cc/output/rendere...
cc/output/renderer_pixeltest.cc:570: shared_state.get(), true, true,
gfx::RectF(0.0f, 0.0f, 1.0f, 1.0f));
nit: gfx::RectF(1.f, 1.f) is much shorter way to say the same thing.

https://chromiumcodereview.appspot.com/207233002/diff/20001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://chromiumcodereview.appspot.com/207233002/diff/20001/cc/output/shader....
cc/output/shader.cc:197: "texOFfset",
"texOffset"

again, is the test broken or not testing this thoroughly?

[sheu, 2014-03-21 20:44:50]

The test was indeed broken.  I just happened to fix the test in a way that was
broken in other ways :-(

I'm really sorry for the oversight here.  Fixed up.  The problem is that the UV
planes in the test were being created at full-height but only uploaded at
subsampled-height.  This just wasn't showing up until I used non-solid color
fill for the test texture.

https://codereview.chromium.org/207233002/diff/20001/cc/output/renderer_pixel...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/207233002/diff/20001/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:411: const gfx::Rect uv_rect(0, 0, rect.width()
+ 1 / 2, rect.height() + 1 / 2);
On 2014/03/21 18:11:11, danakj wrote:
> a + 1 / 2 == a + (1/2) == a + 0
> 
> for integer division. what's going on here? is the test broken or is this
> supposed to be a no-op change to rect.width and height?

:-(

Forgot parentheses.  Fixed.

https://codereview.chromium.org/207233002/diff/20001/cc/output/renderer_pixel...
cc/output/renderer_pixeltest.cc:570: shared_state.get(), true, true,
gfx::RectF(0.0f, 0.0f, 1.0f, 1.0f));
On 2014/03/21 18:11:11, danakj wrote:
> nit: gfx::RectF(1.f, 1.f) is much shorter way to say the same thing.

I'm adding offset support to the tests (see: new OffsetYUVRect test) so I
figured I'd make this explicit.

https://codereview.chromium.org/207233002/diff/20001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/207233002/diff/20001/cc/output/shader.cc#newc...
cc/output/shader.cc:197: "texOFfset",
On 2014/03/21 18:11:11, danakj wrote:
> "texOffset"
> 
> again, is the test broken or not testing this thoroughly?

D'oh.  I lucked out since the driver I'm testing on is not case sensitive to
uniform locations.

Fixed, but does not affect test results.

[danakj, 2014-03-21 20:48:27]

On Fri, Mar 21, 2014 at 4:44 PM, <sheu@chromium.org> wrote:

> The test was indeed broken.  I just happened to fix the test in a way that
> was
> broken in other ways :-(
>
> I'm really sorry for the oversight here.  Fixed up.  The problem is that
> the UV
> planes in the test were being created at full-height but only uploaded at
> subsampled-height.  This just wasn't showing up until I used non-solid
> color
> fill for the test texture.
>

Cool, thanks. So, did the png's change and rietveld just can't tell? Or can
we use different inputs to get pngs that would fail with the same mistake
made again?


>
>
> https://codereview.chromium.org/207233002/diff/20001/cc/
> output/renderer_pixeltest.cc
> File cc/output/renderer_pixeltest.cc (right):
>
> https://codereview.chromium.org/207233002/diff/20001/cc/
> output/renderer_pixeltest.cc#newcode411
>
> cc/output/renderer_pixeltest.cc:411: const gfx::Rect uv_rect(0, 0,
> rect.width() + 1 / 2, rect.height() + 1 / 2);
> On 2014/03/21 18:11:11, danakj wrote:
>
>> a + 1 / 2 == a + (1/2) == a + 0
>>
>
>  for integer division. what's going on here? is the test broken or is
>>
> this
>
>> supposed to be a no-op change to rect.width and height?
>>
>
> :-(
>
> Forgot parentheses.  Fixed.
>
> https://codereview.chromium.org/207233002/diff/20001/cc/
> output/renderer_pixeltest.cc#newcode570
>
> cc/output/renderer_pixeltest.cc:570: shared_state.get(), true, true,
> gfx::RectF(0.0f, 0.0f, 1.0f, 1.0f));
> On 2014/03/21 18:11:11, danakj wrote:
>
>> nit: gfx::RectF(1.f, 1.f) is much shorter way to say the same thing.
>>
>
> I'm adding offset support to the tests (see: new OffsetYUVRect test) so
> I figured I'd make this explicit.
>
> https://codereview.chromium.org/207233002/diff/20001/cc/output/shader.cc
> File cc/output/shader.cc (right):
>
> https://codereview.chromium.org/207233002/diff/20001/cc/
> output/shader.cc#newcode197
> cc/output/shader.cc:197: "texOFfset",
>
> On 2014/03/21 18:11:11, danakj wrote:
>
>> "texOffset"
>>
>
>  again, is the test broken or not testing this thoroughly?
>>
>
> D'oh.  I lucked out since the driver I'm testing on is not case
> sensitive to uniform locations.
>
> Fixed, but does not affect test results.
>
> https://codereview.chromium.org/207233002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sheu, 2014-03-21 21:40:49]

The test images didn't change between my patchsets.  I've modified them now to
make the test a little more specific.

PTAL

[sheu, 2014-03-21 21:42:45]

jschuh@: PTAL at content/common/cc_messages.h.  Trivial.

[danakj, 2014-03-21 21:44:55]

Thanks, LGTM

https://codereview.chromium.org/207233002/diff/100001/cc/output/renderer_pixe...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/207233002/diff/100001/cc/output/renderer_pixe...
cc/output/renderer_pixeltest.cc:450: // YUV values representing a striped
pattern of green.
Update this comment please

[danakj, 2014-03-21 21:45:12]

+piman for content/

[piman, 2014-03-21 22:12:09]

lgtm

[jschuh, 2014-03-21 22:22:58]

ipc security lgtm (note: change from int size to float rect)

[sheu, 2014-03-21 22:30:20]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-03-21 22:31:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/207233002/120001

[danakj, 2014-03-21 22:34:36]

On 2014/03/21 22:22:58, Justin Schuh wrote:
> ipc security lgtm (note: change from int size to float rect)

(float size to float rect)

[commit-bot: I haz the power, 2014-03-22 00:31:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-22 00:31:50]

Retried try job too often on win_rel for step(s) cc_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[sheu, 2014-03-22 00:47:11]

Looks like I got bit by off-by-one pixel differences.  Different filtering on
Windows, I guess.

I'm just going to run this through win_rel and then dcommit (since all the other
bots passed)

[danakj, 2014-03-24 16:06:07]

On Mar 21, 2014 8:52 PM, <sheu@chromium.org> wrote:
>
> Looks like I got bit by off-by-one pixel differences.  Different
filtering on
> Windows, I guess.
>
> I'm just going to run this through win_rel and then dcommit (since all
the other
> bots passed)

If it fails on windows it'll fail on the waterfall on windows. Take a look
at the blur filter pixel tests. On OS_WIN they allow for slight variance.
You can add just enough to make this pass there.

> https://codereview.chromium.org/207233002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[perkj_chrome, 2014-03-26 05:55:37]

On 2014/03/24 16:06:07, danakj wrote:
> On Mar 21, 2014 8:52 PM, <mailto:sheu@chromium.org> wrote:
> >
> > Looks like I got bit by off-by-one pixel differences.  Different
> filtering on
> > Windows, I guess.
> >
> > I'm just going to run this through win_rel and then dcommit (since all
> the other
> > bots passed)
> 
> If it fails on windows it'll fail on the waterfall on windows. Take a look
> at the blur filter pixel tests. On OS_WIN they allow for slight variance.
> You can add just enough to make this pass there.
> 
> > https://codereview.chromium.org/207233002/
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Hej- Any update on this?

[sheu, 2014-03-27 00:13:54]

I'm on it.  This is dependent on:

    https://chromiumcodereview.appspot.com/178133005/

which I just uploaded another CL for to CQ on.  (Fixing that required that I
figure out how to build/run/test for Android, hence the delay.)

[sheu, 2014-03-27 07:46:49]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-03-27 07:48:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/207233002/160001

[commit-bot: I haz the power, 2014-03-27 11:43:53]

Change committed as 259831