cc: make resource keep video buffer format for hardware overlay.

cc: make resource keep video buffer format for hardware overlay.

The below CL added gfx::BufferFormat to cc::ResourceProvider::Resource.
https://codereview.chromium.org/2748903002

This CL makes cc::ResourceProvider::Resource keep gfx::BufferFormat
for cc:StreamVideoDrawQuad as well.

BUG=683347
TEST=run chromeos with --enable-hardware-overlays on http://www.quirksmode.org/html5/tests/video.html
existing cc_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

[dshwang, 2017-02-08 18:50:39]

Description was changed from

==========
Make OverlayCandidate use gfx::BufferFormat instead of cc::ResourceFormat

The format of overlay plane is not very related to cc resource format. Currently
only Ozone backend requires the format variable and always convert it to
gfx::BufferFormat.

BUG=683347
TEST=run chromeos with --enable-hardware-overlays on
http://www.quirksmode.org/html5/tests/video.html
==========

to

==========
Make OverlayCandidate use gfx::BufferFormat instead of cc::ResourceFormat

The format of overlay plane is not very related to cc resource format. Currently
only Ozone backend requires the format variable and always convert it to
gfx::BufferFormat.

BUG=683347
TEST=run chromeos with --enable-hardware-overlays on
http://www.quirksmode.org/html5/tests/video.html
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[inactive_dshwang_plz_cc_intel, 2017-02-08 18:50:48]

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

[commit-bot: I haz the power, 2017-02-08 18:51:25]

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-08 18:54:36]

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

[dshwang, 2017-02-08 18:54:36]

ccameron@, could you review?
https://codereview.chromium.org/2648633005/ is waiting for this CL, because the
CL want to check YUV format for overlay but cc::ResourceFormat doesn't care of
YUV format.

[Daniele Castagna, 2017-02-08 18:59:48]

On 2017/02/08 at 18:54:36, dongseong.hwang wrote:
> ccameron@, could you review?
> https://codereview.chromium.org/2648633005/ is waiting for this CL, because
the CL want to check YUV format for overlay but cc::ResourceFormat doesn't care
of YUV format.

I'm still now quite sure why you want to push
https://codereview.chromium.org/2648633005 considering we won't have drm atomics
on skylake anytime soon.

https://codereview.chromium.org/2683763003/diff/1/components/display_composit...
File
components/display_compositor/compositor_overlay_candidate_validator_ozone.cc
(right):

https://codereview.chromium.org/2683763003/diff/1/components/display_composit...
components/display_compositor/compositor_overlay_candidate_validator_ozone.cc:94:
ozone_surface_list.at(i).format = surfaces->at(i).format;
This would now pass the format even if it's not an RGB format while we used to
accept only RGBA/BGRA.
Should we validate the format somewhere?

[Daniele Castagna, 2017-02-08 19:54:12]

On 2017/02/08 at 18:59:48, Daniele Castagna wrote:
> On 2017/02/08 at 18:54:36, dongseong.hwang wrote:
> > ccameron@, could you review?
> > https://codereview.chromium.org/2648633005/ is waiting for this CL, because
the CL want to check YUV format for overlay but cc::ResourceFormat doesn't care
of YUV format.
> 
> I'm still now quite sure why you want to push
https://codereview.chromium.org/2648633005 considering we won't have drm atomics
on skylake anytime soon.

s/now/not
> 
>
https://codereview.chromium.org/2683763003/diff/1/components/display_composit...
> File
components/display_compositor/compositor_overlay_candidate_validator_ozone.cc
(right):
> 
>
https://codereview.chromium.org/2683763003/diff/1/components/display_composit...
>
components/display_compositor/compositor_overlay_candidate_validator_ozone.cc:94:
ozone_surface_list.at(i).format = surfaces->at(i).format;
> This would now pass the format even if it's not an RGB format while we used to
accept only RGBA/BGRA.
> Should we validate the format somewhere?

[commit-bot: I haz the power, 2017-02-08 20:03:59]

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

[commit-bot: I haz the power, 2017-02-08 20:04:00]

Dry run: This issue passed the CQ dry run.

[inactive_dshwang_plz_cc_intel, 2017-02-08 23:48:45]

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

[commit-bot: I haz the power, 2017-02-08 23:49:15]

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-08 23:52:40]

Description was changed from

==========
Make OverlayCandidate use gfx::BufferFormat instead of cc::ResourceFormat

The format of overlay plane is not very related to cc resource format. Currently
only Ozone backend requires the format variable and always convert it to
gfx::BufferFormat.

BUG=683347
TEST=run chromeos with --enable-hardware-overlays on
http://www.quirksmode.org/html5/tests/video.html
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Check real video format for hardware overlay, instead of hardcoded RGBA.

StreamVideoDrawQuad is overlayable but it can be YUV format. Currently
only Ozone backend requires the format variable and the plain in
StreamVideoDrawQuad
always RGBA, but it will support YUV format in near future.

BUG=683347
TEST=run chromeos with --enable-hardware-overlays on
http://www.quirksmode.org/html5/tests/video.html
existing cc_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[dshwang, 2017-02-09 00:03:55]

Now we don't hardcode RGBA for StreamVideoDrawQuad overlay.

On 2017/02/08 18:59:48, Daniele Castagna wrote:
> On 2017/02/08 at 18:54:36, dongseong.hwang wrote:
> > ccameron@, could you review?
> > https://codereview.chromium.org/2648633005/ is waiting for this CL, because
> the CL want to check YUV format for overlay but cc::ResourceFormat doesn't
care
> of YUV format.
> 
> I'm still NOT quite sure why you want to push
> https://codereview.chromium.org/2648633005 considering we won't have drm
atomics
> on skylake anytime soon.

https://codereview.chromium.org/2648633005 has two goals.
1. decode video in YUYV format (by both gpu and software decoder)
2. enable YUYV hardware overlay with nuclear page flip

The first goal is still valid for Skylake as well as all generation IA
Chromebook.
Changing decoding format from RGBA to YUYV itself reduces ~7% power consumption.

Sorry for delaying separating #1 and #2. I'll do it soon.

https://codereview.chromium.org/2683763003/diff/1/components/display_composit...
File
components/display_compositor/compositor_overlay_candidate_validator_ozone.cc
(right):

https://codereview.chromium.org/2683763003/diff/1/components/display_composit...
components/display_compositor/compositor_overlay_candidate_validator_ozone.cc:94:
ozone_surface_list.at(i).format = surfaces->at(i).format;
On 2017/02/08 18:59:48, Daniele Castagna wrote:
> This would now pass the format even if it's not an RGB format while we used to
> accept only RGBA/BGRA.
> Should we validate the format somewhere?

Yes, DrmOverlayManager::CheckOverlaySupport() checks whether the format is
supported or not.

Actually, DrmOverlayValidator::TestPageFlip() queries the plane list (with
format) is supported to drm device (by ioctl call).

Above hack was introduced when drm doesn't support RGBA format, but it's stale.

https://codereview.chromium.org/2683763003/diff/20001/cc/output/overlay_candi...
File cc/output/overlay_candidate.cc (right):

https://codereview.chromium.org/2683763003/diff/20001/cc/output/overlay_candi...
cc/output/overlay_candidate.cc:287: candidate->format = quad->format;
To query this actuall format, all boilerplate plumbing code is added.

[dshwang, 2017-02-09 00:07:08]

Description was changed from

==========
Check real video format for hardware overlay, instead of hardcoded RGBA.

StreamVideoDrawQuad is overlayable but it can be YUV format. Currently
only Ozone backend requires the format variable and the plain in
StreamVideoDrawQuad
always RGBA, but it will support YUV format in near future.

BUG=683347
TEST=run chromeos with --enable-hardware-overlays on
http://www.quirksmode.org/html5/tests/video.html
existing cc_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Check real video format for hardware overlay, instead of hardcoded RGBA.

StreamVideoDrawQuad is overlayable. Only Ozone backend requires the format
variable
to check if overlay is possible. Currently the format of StreamVideoDrawQuad is
hardcoded to RGBA.
This CL makes it check real video format because YUV format will be supported in
near future.

BUG=683347
TEST=run chromeos with --enable-hardware-overlays on
http://www.quirksmode.org/html5/tests/video.html
existing cc_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

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

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

[commit-bot: I haz the power, 2017-02-09 01:13:07]

Dry run: This issue passed the CQ dry run.

[Daniele Castagna, 2017-02-09 04:01:08]

On 2017/02/09 at 00:03:55, dongseong.hwang wrote:
> Now we don't hardcode RGBA for StreamVideoDrawQuad overlay.
> 
> On 2017/02/08 18:59:48, Daniele Castagna wrote:
> > On 2017/02/08 at 18:54:36, dongseong.hwang wrote:
> > > ccameron@, could you review?
> > > https://codereview.chromium.org/2648633005/ is waiting for this CL,
because
> > the CL want to check YUV format for overlay but cc::ResourceFormat doesn't
care
> > of YUV format.
> > 
> > I'm still NOT quite sure why you want to push
> > https://codereview.chromium.org/2648633005 considering we won't have drm
atomics
> > on skylake anytime soon.
> 
> https://codereview.chromium.org/2648633005 has two goals.
> 1. decode video in YUYV format (by both gpu and software decoder)
> 2. enable YUYV hardware overlay with nuclear page flip
> 
> The first goal is still valid for Skylake as well as all generation IA
Chromebook.
> Changing decoding format from RGBA to YUYV itself reduces ~7% power
consumption.
> 
> Sorry for delaying separating #1 and #2. I'll do it soon.
> 
>
https://codereview.chromium.org/2683763003/diff/1/components/display_composit...
> File
components/display_compositor/compositor_overlay_candidate_validator_ozone.cc
(right):
> 
>
https://codereview.chromium.org/2683763003/diff/1/components/display_composit...
>
components/display_compositor/compositor_overlay_candidate_validator_ozone.cc:94:
ozone_surface_list.at(i).format = surfaces->at(i).format;
> On 2017/02/08 18:59:48, Daniele Castagna wrote:
> > This would now pass the format even if it's not an RGB format while we used
to
> > accept only RGBA/BGRA.
> > Should we validate the format somewhere?
> 
> Yes, DrmOverlayManager::CheckOverlaySupport() checks whether the format is
supported or not.
> 
> Actually, DrmOverlayValidator::TestPageFlip() queries the plane list (with
format) is supported to drm device (by ioctl call).
> 
Makes sense. This makes me wonder again how the first patch that you used to get
numbers was working in the first place since we were not letting anything but
RGB trough.

> Above hack was introduced when drm doesn't support RGBA format, but it's
stale.
> 
>
https://codereview.chromium.org/2683763003/diff/20001/cc/output/overlay_candi...
> File cc/output/overlay_candidate.cc (right):
> 
>
https://codereview.chromium.org/2683763003/diff/20001/cc/output/overlay_candi...
> cc/output/overlay_candidate.cc:287: candidate->format = quad->format;
> To query this actuall format, all boilerplate plumbing code is added.

Patch 2 is now significantly different than patch 1. I'll try to take a look at
it tomorrow.

CCing marcheu and posciak.

[dshwang, 2017-02-09 05:10:22]

On 2017/02/09 04:01:08, Daniele Castagna wrote:
>
https://codereview.chromium.org/2683763003/diff/1/components/display_composit...
> >
>
components/display_compositor/compositor_overlay_candidate_validator_ozone.cc:94:
> ozone_surface_list.at(i).format = surfaces->at(i).format;
> > On 2017/02/08 18:59:48, Daniele Castagna wrote:
> > > This would now pass the format even if it's not an RGB format while we
used
> to
> > > accept only RGBA/BGRA.
> > > Should we validate the format somewhere?
> > 
> > Yes, DrmOverlayManager::CheckOverlaySupport() checks whether the format is
> supported or not.
> > 
> > Actually, DrmOverlayValidator::TestPageFlip() queries the plane list (with
> format) is supported to drm device (by ioctl call).
> > 
> Makes sense. This makes me wonder again how the first patch that you used to
get
> numbers was working in the first place since we were not letting anything but
> RGB trough.

That's good question. I explained this existing inconsistence in
https://codereview.chromium.org/2648633005/#msg37
Let me explain again.

Imagine there are BGRA primary plane and RGBA video plane.
* Ozone checks BGRA primary plane and BGRA video plane.
* But Ozone overlays actually BGRA primary plane and RGBA video plane.

It's because
* Ozone is tracking actual format of the plane (GbmBuffer)
* drm supports both "BGRA primary plane and BGRA video plane" and "BGRA primary
plane and RGBA video plane".

In detail,

OverlayStrategySingleOnTop::TryOverlay queries to drm. note that it sends format
to gpu process.
bool OverlayStrategySingleOnTop::TryOverlay(
    QuadList* quad_list,
    OverlayCandidateList* candidate_list,
    const OverlayCandidate& candidate,
    QuadList::Iterator candidate_iterator) {
  ...
  capability_checker_->CheckOverlaySupport(&new_candidate_list);
  ...
}


GLRenderer::ScheduleOverlays kicks actual page flip. note that it doesn't sent
format to gpu
process, and gpu process knows actual format of the plane. lol
void GLRenderer::ScheduleOverlays(DrawingFrame* frame) {
    ....
    context_support_->ScheduleOverlayPlane(
        overlay.plane_z_order, overlay.transform, texture_id,
        ToNearestRect(overlay.display_rect), overlay.uv_rect);
  }
}

This CL fixes this inconsistency.

> > Above hack was introduced when drm doesn't support RGBA format, but it's
> stale.
> > 
> >
>
https://codereview.chromium.org/2683763003/diff/20001/cc/output/overlay_candi...
> > File cc/output/overlay_candidate.cc (right):
> > 
> >
>
https://codereview.chromium.org/2683763003/diff/20001/cc/output/overlay_candi...
> > cc/output/overlay_candidate.cc:287: candidate->format = quad->format;
> > To query this actuall format, all boilerplate plumbing code is added.
> 
> Patch 2 is now significantly different than patch 1. I'll try to take a look
at
> it tomorrow.
> 
> CCing marcheu and posciak.

Thank you in advance :)

[dshwang, 2017-03-07 21:32:23]

Daniele, can we make restart this review? It's rather refactorying than GMB
something.

[Daniele Castagna, 2017-03-09 02:45:09]

On 2017/03/07 at 21:32:23, dongseong.hwang wrote:
> Daniele, can we make restart this review? It's rather refactorying than GMB
something.

I like the idea of using gfx::BufferFormat for OverlayCandidate.

What I don't like is that now you have added a format in StreamVideoDrawQuad
that already has a resource that has a format too. This CL is way too specific
for what you're trying to do with videos instead of trying to get the format
across to the overlaycandidate for all the quads/resources.

For example, we also need a gfx::BufferFormat for TextuerDrawQuad and I don't
want to add it there too.
Should we try to replace cc::ResourceFormat with gfx::BufferFormat in the
ResourceProvider?

We could start landing the first patch you uploaded, without all the specific
plumbing you did for StreamVideoDrawQuad.

WDYT?

[Daniele Castagna, 2017-03-13 22:50:37]

On 2017/03/09 at 02:45:09, Daniele Castagna wrote:
> On 2017/03/07 at 21:32:23, dongseong.hwang wrote:
> > Daniele, can we make restart this review? It's rather refactorying than GMB
something.
> 
> I like the idea of using gfx::BufferFormat for OverlayCandidate.
> 
> What I don't like is that now you have added a format in StreamVideoDrawQuad
that already has a resource that has a format too. This CL is way too specific
for what you're trying to do with videos instead of trying to get the format
across to the overlaycandidate for all the quads/resources.
> 
> For example, we also need a gfx::BufferFormat for TextuerDrawQuad and I don't
want to add it there too.
> Should we try to replace cc::ResourceFormat with gfx::BufferFormat in the
ResourceProvider?
> 
> We could start landing the first patch you uploaded, without all the specific
plumbing you did for StreamVideoDrawQuad.
> 
> WDYT?

Just tried to put up a patch for the second part. Replacing the resource format
with BufferFormat is not doable, since they might be discordant. We decided to
just add a buffer_format to the Resource:
https://codereview.chromium.org/2748903002

[inactive_dshwang_plz_cc_intel, 2017-03-28 01:48:23]

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

[commit-bot: I haz the power, 2017-03-28 01:48:54]

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

[dshwang, 2017-03-28 01:51:58]

Description was changed from

==========
Check real video format for hardware overlay, instead of hardcoded RGBA.

StreamVideoDrawQuad is overlayable. Only Ozone backend requires the format
variable
to check if overlay is possible. Currently the format of StreamVideoDrawQuad is
hardcoded to RGBA.
This CL makes it check real video format because YUV format will be supported in
near future.

BUG=683347
TEST=run chromeos with --enable-hardware-overlays on
http://www.quirksmode.org/html5/tests/video.html
existing cc_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: make resource keep video buffer format for hardware overlay.

The below CL added gfx::BufferFormat to cc::ResourceProvider::Resource.
https://codereview.chromium.org/2748903002

This CL makes cc::ResourceProvider::Resource keep gfx::BufferFormat
for cc:StreamVideoDrawQuad as well.

BUG=683347
TEST=run chromeos with --enable-hardware-overlays on
http://www.quirksmode.org/html5/tests/video.html
existing cc_unittests
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[dshwang, 2017-03-28 01:52:42]

Hi Daniele, this CL plumbing video format to cc::resource for overlay. Could you
review?

[Daniele Castagna, 2017-03-28 02:01:39]

https://codereview.chromium.org/2683763003/diff/40001/cc/output/overlay_candi...
File cc/output/overlay_candidate.cc (right):

https://codereview.chromium.org/2683763003/diff/40001/cc/output/overlay_candi...
cc/output/overlay_candidate.cc:289: candidate->format =
resource_provider->GetBufferFormat(quad->resource_id());
We're not supposed to use the StreamVideoQuad on CrOS.
StreamVideoQuad was added for Android and it is accidentally used in CrOS at the
moment.

crrev.com/2763223003 to limit the usage only to Android.

[commit-bot: I haz the power, 2017-03-28 03:20:53]

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

[commit-bot: I haz the power, 2017-03-28 03:20:54]

Dry run: This issue passed the CQ dry run.

[dshwang, 2017-03-28 16:42:25]

https://codereview.chromium.org/2683763003/diff/40001/cc/output/overlay_candi...
File cc/output/overlay_candidate.cc (right):

https://codereview.chromium.org/2683763003/diff/40001/cc/output/overlay_candi...
cc/output/overlay_candidate.cc:289: candidate->format =
resource_provider->GetBufferFormat(quad->resource_id());
On 2017/03/28 02:01:39, Daniele Castagna wrote:
> We're not supposed to use the StreamVideoQuad on CrOS.
> StreamVideoQuad was added for Android and it is accidentally used in CrOS at
the
> moment.
> 
> crrev.com/2763223003 to limit the usage only to Android.

wow, that's why cros use StreamVideoQuad recently.

By the way, this CL is still valid, because it's not very related to
StreamVideoQuad. It rather makes video_layer_impl set buffer format to resource
for mailbox resource. Without this change, TextureQuad from video cannot know
buffer format. In addition, even though this line isn't used by cros, I think
it's not bad to keep it.