cc: UYVY video is not premultiplied rgba

cc: UYVY video is not premultiplied rgba

BUG=683347
TEST=cc_unittests VideoResourceUpdaterTest.CreateForHardwarePlanes_UYVYTexture
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

[dshwang, 2017-02-09 02:33:05]

Description was changed from

==========
cc: UYVY video is not premultiplied rgba

BUG=683347
TEST=cc_unittests VideoResourceUpdaterTest.CreateForHardwarePlanes_UYVYTexture
==========

to

==========
cc: UYVY video is not premultiplied rgba

BUG=683347
TEST=cc_unittests VideoResourceUpdaterTest.CreateForHardwarePlanes_UYVYTexture
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[inactive_dshwang_plz_cc_intel, 2017-02-09 02:33:15]

The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-09 02:33:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dshwang, 2017-02-09 02:35:59]

dongseong.hwang@intel.com changed reviewers:
+ ccameron@chromium.org, dcastagna@chromium.org

[dshwang, 2017-02-09 02:36:00]

ccameron@, dcastagna@, could you review? when dcastagna@ made UYVY support, this
bug was created but as only mac use UYVY, the code never be reached. To cover
the change, this CL adds new unittests for UYVY.

[commit-bot: I haz the power, 2017-02-09 06:03:00]

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

[commit-bot: I haz the power, 2017-02-09 06:03:01]

Dry run: This issue passed the CQ dry run.

[dshwang, 2017-02-28 02:16:06]

On 2017/02/09 02:36:00, dshwang wrote:
> ccameron@, dcastagna@, could you review? when dcastagna@ made UYVY support,
this
> bug was created but as only mac use UYVY, the code never be reached. To cover
> the change, this CL adds new unittests for UYVY.

ccameron@, dcastagna@, could you review? it's quite simple CL, and I think Mac
has bug about it.

[Daniele Castagna, 2017-02-28 03:17:53]

https://codereview.chromium.org/2684073006/diff/1/cc/resources/video_resource...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/2684073006/diff/1/cc/resources/video_resource...
cc/resources/video_resource_updater.cc:42: case GL_TEXTURE_2D:
Isn't CrOS using TEXTURE_EXTERNAL_OES?

[dshwang, 2017-02-28 04:18:45]

https://codereview.chromium.org/2684073006/diff/1/cc/resources/video_resource...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/2684073006/diff/1/cc/resources/video_resource...
cc/resources/video_resource_updater.cc:42: case GL_TEXTURE_2D:
On 2017/02/28 03:17:53, Daniele Castagna wrote:
> Isn't CrOS using TEXTURE_EXTERNAL_OES?

Not always TEXTURE_EXTERNAL_OES

GPU video decoder uses TEXTURE_EXTERNAL_OES
GpuMemoryBufferVideoFramePool with native GMB uses TEXTURE_EXTERNAL_OES
GpuMemoryBufferVideoFramePool with software-fallback GMB uses GL_TEXTURE_2D

[Daniele Castagna, 2017-02-28 04:20:12]

On 2017/02/28 at 04:18:45, dongseong.hwang wrote:
>
https://codereview.chromium.org/2684073006/diff/1/cc/resources/video_resource...
> File cc/resources/video_resource_updater.cc (right):
> 
>
https://codereview.chromium.org/2684073006/diff/1/cc/resources/video_resource...
> cc/resources/video_resource_updater.cc:42: case GL_TEXTURE_2D:
> On 2017/02/28 03:17:53, Daniele Castagna wrote:
> > Isn't CrOS using TEXTURE_EXTERNAL_OES?
> 
> Not always TEXTURE_EXTERNAL_OES
> 
> GPU video decoder uses TEXTURE_EXTERNAL_OES
> GpuMemoryBufferVideoFramePool with native GMB uses TEXTURE_EXTERNAL_OES
> GpuMemoryBufferVideoFramePool with software-fallback GMB uses GL_TEXTURE_2D

That seems correct.

There is no UYVY with software GMB though, so my point still stands. How did you
notice this problem working on UYVY?

[dshwang, 2017-02-28 04:31:52]

On 2017/02/28 04:20:12, Daniele Castagna wrote:
> On 2017/02/28 at 04:18:45, dongseong.hwang wrote:
> >
>
https://codereview.chromium.org/2684073006/diff/1/cc/resources/video_resource...
> > File cc/resources/video_resource_updater.cc (right):
> > 
> >
>
https://codereview.chromium.org/2684073006/diff/1/cc/resources/video_resource...
> > cc/resources/video_resource_updater.cc:42: case GL_TEXTURE_2D:
> > On 2017/02/28 03:17:53, Daniele Castagna wrote:
> > > Isn't CrOS using TEXTURE_EXTERNAL_OES?
> > 
> > Not always TEXTURE_EXTERNAL_OES
> > 
> > GPU video decoder uses TEXTURE_EXTERNAL_OES
> > GpuMemoryBufferVideoFramePool with native GMB uses TEXTURE_EXTERNAL_OES
> > GpuMemoryBufferVideoFramePool with software-fallback GMB uses GL_TEXTURE_2D
> 
> That seems correct.
> 
> There is no UYVY with software GMB though, so my point still stands. How did
you
> notice this problem working on UYVY?

ah, I never meet issue. I just read code and submit the patch in theory. because
when I add YUYV, premultiplied sounds wrong.
There is no UYVY and YUYV with software GMB though. By the way, I think the
changed code looks more logical :)

[dshwang, 2017-03-07 21:34:30]

On 2017/02/28 04:31:52, dshwang wrote:
> On 2017/02/28 04:20:12, Daniele Castagna wrote:
> > On 2017/02/28 at 04:18:45, dongseong.hwang wrote:
> > >
> >
>
https://codereview.chromium.org/2684073006/diff/1/cc/resources/video_resource...
> > > File cc/resources/video_resource_updater.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2684073006/diff/1/cc/resources/video_resource...
> > > cc/resources/video_resource_updater.cc:42: case GL_TEXTURE_2D:
> > > On 2017/02/28 03:17:53, Daniele Castagna wrote:
> > > > Isn't CrOS using TEXTURE_EXTERNAL_OES?
> > > 
> > > Not always TEXTURE_EXTERNAL_OES
> > > 
> > > GPU video decoder uses TEXTURE_EXTERNAL_OES
> > > GpuMemoryBufferVideoFramePool with native GMB uses TEXTURE_EXTERNAL_OES
> > > GpuMemoryBufferVideoFramePool with software-fallback GMB uses
GL_TEXTURE_2D
> > 
> > That seems correct.
> > 
> > There is no UYVY with software GMB though, so my point still stands. How did
> you
> > notice this problem working on UYVY?
> 
> ah, I never meet issue. I just read code and submit the patch in theory.
because
> when I add YUYV, premultiplied sounds wrong.
> There is no UYVY and YUYV with software GMB though. By the way, I think the
> changed code looks more logical :)

Daniele, could you review again? It's minor change, and looks more logical,
although any code path doesn't come to here yet.

[Daniele Castagna, 2017-03-10 02:54:50]

On 2017/03/07 at 21:34:30, dongseong.hwang wrote:
> On 2017/02/28 04:31:52, dshwang wrote:
> > On 2017/02/28 04:20:12, Daniele Castagna wrote:
> > > On 2017/02/28 at 04:18:45, dongseong.hwang wrote:
> > > >
> > >
> >
https://codereview.chromium.org/2684073006/diff/1/cc/resources/video_resource...
> > > > File cc/resources/video_resource_updater.cc (right):
> > > > 
> > > >
> > >
> >
https://codereview.chromium.org/2684073006/diff/1/cc/resources/video_resource...
> > > > cc/resources/video_resource_updater.cc:42: case GL_TEXTURE_2D:
> > > > On 2017/02/28 03:17:53, Daniele Castagna wrote:
> > > > > Isn't CrOS using TEXTURE_EXTERNAL_OES?
> > > > 
> > > > Not always TEXTURE_EXTERNAL_OES
> > > > 
> > > > GPU video decoder uses TEXTURE_EXTERNAL_OES
> > > > GpuMemoryBufferVideoFramePool with native GMB uses TEXTURE_EXTERNAL_OES
> > > > GpuMemoryBufferVideoFramePool with software-fallback GMB uses
GL_TEXTURE_2D
> > > 
> > > That seems correct.
> > > 
> > > There is no UYVY with software GMB though, so my point still stands. How
did
> > you
> > > notice this problem working on UYVY?
> > 
> > ah, I never meet issue. I just read code and submit the patch in theory.
because
> > when I add YUYV, premultiplied sounds wrong.
> > There is no UYVY and YUYV with software GMB though. By the way, I think the
> > changed code looks more logical :)
> 
> Daniele, could you review again? It's minor change, and looks more logical,
although any code path doesn't come to here yet.

I still don't understand why you want this, it's not going to change anything
but adding a multiplication by 1 in the shader.

If I understand correctly, the UYVY texture will behave like an RGBX texture
when sampled in the shader.

The only difference that PREMULTIPLIED alpha makes vs NON PREMULTIPLIED is if
the color should be multiplied by the alpha in the fragment shader.

Look at
https://cs.chromium.org/chromium/src/cc/output/shader.cc?rcl=1fd0ec64195b3be2...

In your case, I'm assuming texColor.a will be 1.0. So why would you want to
multiply every color sampled by 1 when you can avoid that and the result should
be exactly the same?

[dshwang, 2017-03-23 20:07:30]

On 2017/03/10 02:54:50, Daniele Castagna wrote:
> On 2017/03/07 at 21:34:30, dongseong.hwang wrote:
> > On 2017/02/28 04:31:52, dshwang wrote:
> > > On 2017/02/28 04:20:12, Daniele Castagna wrote:
> > > > On 2017/02/28 at 04:18:45, dongseong.hwang wrote:
> > > > >
> > > >
> > >
>
https://codereview.chromium.org/2684073006/diff/1/cc/resources/video_resource...
> > > > > File cc/resources/video_resource_updater.cc (right):
> > > > > 
> > > > >
> > > >
> > >
>
https://codereview.chromium.org/2684073006/diff/1/cc/resources/video_resource...
> > > > > cc/resources/video_resource_updater.cc:42: case GL_TEXTURE_2D:
> > > > > On 2017/02/28 03:17:53, Daniele Castagna wrote:
> > > > > > Isn't CrOS using TEXTURE_EXTERNAL_OES?
> > > > > 
> > > > > Not always TEXTURE_EXTERNAL_OES
> > > > > 
> > > > > GPU video decoder uses TEXTURE_EXTERNAL_OES
> > > > > GpuMemoryBufferVideoFramePool with native GMB uses
TEXTURE_EXTERNAL_OES
> > > > > GpuMemoryBufferVideoFramePool with software-fallback GMB uses
> GL_TEXTURE_2D
> > > > 
> > > > That seems correct.
> > > > 
> > > > There is no UYVY with software GMB though, so my point still stands. How
> did
> > > you
> > > > notice this problem working on UYVY?
> > > 
> > > ah, I never meet issue. I just read code and submit the patch in theory.
> because
> > > when I add YUYV, premultiplied sounds wrong.
> > > There is no UYVY and YUYV with software GMB though. By the way, I think
the
> > > changed code looks more logical :)
> > 
> > Daniele, could you review again? It's minor change, and looks more logical,
> although any code path doesn't come to here yet.
> 
> I still don't understand why you want this, it's not going to change anything
> but adding a multiplication by 1 in the shader.
> 
> If I understand correctly, the UYVY texture will behave like an RGBX texture
> when sampled in the shader.
> 
> The only difference that PREMULTIPLIED alpha makes vs NON PREMULTIPLIED is if
> the color should be multiplied by the alpha in the fragment shader.
> 
> Look at
>
https://cs.chromium.org/chromium/src/cc/output/shader.cc?rcl=1fd0ec64195b3be2...
> 
> In your case, I'm assuming texColor.a will be 1.0. So why would you want to
> multiply every color sampled by 1 when you can avoid that and the result
should
> be exactly the same?


Yes, it doesn't change any behavior. I just want to make it more logically make
sense. It's not big deal, tho.
premultiplied is RGBA concept, but XRGB, YUYV, UYVY doesn't contain alpha
channel, so doesn't make sense with premultiplied. It's fine, if gpu driver
fills tex.a to 1.0 when texture lookup on top of EGL image, which is on top of
GEM prime FD. but who know what gpu driver do for this unclear behavior.