Sushil Reporter
	Description • 10 years ago

Bug 1051636 has caused regression in Video playback and Camera/Camcorder preview and recording on b2g v2.1.

1. Black screen is visible sometimes. If the Video content is indeed Opaque, why are we not setting the correct content flag: "Layer::CONTENT_OPAQUE" ?

2. This patch will cause power regression as well since we were dropping layers hidden under full screen Opaque Video layer, in HWC Composition path. Plus HAL power optimization for Opaque layers in layer tree, will not kick-in with this patch.

As per Comment 0 of Bug 1051636, it was identified as regression on Windows. So why not catch and fix the original regression ?

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 1 • 10 years ago

(In reply to Sushil from comment #0)
> Bug 1051636 has caused regression in Video playback and Camera/Camcorder
> preview and recording on b2g v2.1.
> 
> 1. Black screen is visible sometimes. If the Video content is indeed Opaque,
> why are we not setting the correct content flag: "Layer::CONTENT_OPAQUE" ?

The layer was being marked CONTENT_OPAQUE in its visible region, even though its visible region extended beyond the area of the actual image and the layer therefore wasn't really opaque in that region.

There's another problem which is that sometimes a <video> element doesn't render anything because we have no video frame loaded. In that case the layer is not opaque.

Another issue is that sooner or later we'll support video formats with transparency. Chrome already does. For such formats we can't just mark the layer opaque; we need to check the opaqueness of each video frame.

I'm not sure why not marking the layer opaque can cause a black screen to be visible sometimes. Can you tell me more about what, when and why that happens?

> 2. This patch will cause power regression as well since we were dropping
> layers hidden under full screen Opaque Video layer, in HWC Composition path.
> Plus HAL power optimization for Opaque layers in layer tree, will not
> kick-in with this patch.

We can still do these optimizations when the ImageLayer contains an opaque Image. The HWC code needs to be adjusted to check the opacity of the Image (and whether there is an Image) instead of checking for CONTENT_OPAQUE on the layer. Can you point me at the code that needs to be fixed?

> As per Comment 0 of Bug 1051636, it was identified as regression on Windows.
> So why not catch and fix the original regression ?

Windows just happens to be the platform that showed the bug. The three issues I mentioned above are cross-platform bugs that have been in our code for quite a long time, that happened to be revealed by changes to layer construction that we made.

	Michael Vines [:m1] [:evilmachines]
	Comment 2 • 10 years ago

[Blocking Requested - why for this release]:

Maybe we can back out bug 1051636 of v2.1 until we determine the root cause of the issue?

	Sushil Reporter
	Comment 3 • 10 years ago

This patch affects [1] which decides blending mode of layer at [2]. Hence, we are not able to punch a hole in FrameBuffer at [3], i.e. clearing FB with transparent pixels at the destination rectangle on screen where the Video layer would be composed by HWC in partial HWC composition path. Higher z-order RGBA layers are composed by GPU on FrameBuffer. This is the cause for black screen on Video layer in Camera & Video use cases. SurfaceFlinger in Android also follows same approach.

If you can confirm, that checking only layer's opacity is sufficient to confirm the Opacity of content and layer, then I can modify [1]. If yes, then is it applicable only for Image layer OR all other layer types as well (Thebes, canvas, etc)?

[1]: http://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.cpp#429

[2]: http://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.cpp#462

[3]: http://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.cpp#691

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 4 • 10 years ago

Thanks for the info!

(In reply to Sushil from comment #3)
> If you can confirm, that checking only layer's opacity is sufficient to
> confirm the Opacity of content and layer, then I can modify [1].

That's not the right thing to do. A layer can have opacity==0xff but it might still be a buffer with some transparent pixels.

We can treat an ImageLayer as opaque if its current image is opaque. That's what we need here. I think we can inspect the LayoutRenderState to get that information. So something like
  opacity == 0xFF &&
    ((aLayer->GetContentFlags() & Layer::CONTENT_OPAQUE) ||
     IsOpaqueFormat(state.mSurface->getPixelFormat());
where IsOpaqueFormat is a function with a big switch() statement that returns true for pixel formats we know to be opaque, i.e. all formats without an alpha channel (which should include all the camera and video formats we normally use).

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 5 • 10 years ago

(In reply to Michael Vines [:m1] [:evilmachines] from comment #2)
> [Blocking Requested - why for this release]:
> 
> Maybe we can back out bug 1051636 of v2.1 until we determine the root cause
> of the issue?

I don't mind if you back out 1051636 in a B2G-specific way.

But it seems like we can fix this bug quite easily.

	Michael Vines [:m1] [:evilmachines]
	Comment 6 • 10 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #5)
> (In reply to Michael Vines [:m1] [:evilmachines] from comment #2)
> > [Blocking Requested - why for this release]:
> > 
> > Maybe we can back out bug 1051636 of v2.1 until we determine the root cause
> > of the issue?
> 
> I don't mind if you back out 1051636 in a B2G-specific way.
> 
> But it seems like we can fix this bug quite easily.

If we can fix it today (Friday PST) then great.  Otherwise we need it backed out as this is a major regression in B2G that risks affecting milestone dates.  I don't know how you would back it out in a B2G-specific way but if you can then that works!

	Sushil Reporter
	Comment 7 • 10 years ago

Who can upload change for |IsOpaqueFormat(state.mSurface->getPixelFormat()| mentioned in Comment 4. Given that some of YUV formats can be OEM specific (ex: Camera), I am not sure how this new framework API will cover all possible YUV formats. Isn't there a better way in Gecko to set correct content flag dynamically, i.e. "aLayer->GetContentFlags()" returns value based on the pixel format of buffer ?

Michael,
I also agree, it is a major risk for b2g v2.1 time line.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 8 • 10 years ago

Backed out on Aurora with #ifdef B2G. I assumed a=bajaj since she asked me to do it...

https://hg.mozilla.org/releases/mozilla-aurora/rev/d9a94c7c7246

(In reply to Sushil from comment #7)
> Who can upload change for |IsOpaqueFormat(state.mSurface->getPixelFormat()|
> mentioned in Comment 4. Given that some of YUV formats can be OEM specific
> (ex: Camera), I am not sure how this new framework API will cover all
> possible YUV formats.

This is just an optimization so it doesn't matter if we don't cover all possible YUV formats. We just need to cover the formats that we actually see in practice.

> Isn't there a better way in Gecko to set correct
> content flag dynamically, i.e. "aLayer->GetContentFlags()" returns value
> based on the pixel format of buffer ?

We can't set layer content flags based on the current image since the current image can be set asynchronously from any thread and and layer content flags can only be set the Gecko main thread that's managing the layer. So we might set the flag one way on the main thread and then someone might change the current image from another thread where there's no way to change the content flags.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 9 • 10 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> This is just an optimization so it doesn't matter if we don't cover all
> possible YUV formats. We just need to cover the formats that we actually see
> in practice.

Actually I guess that's not quite true because of the way HWC works.

I don't see where compositionType is set to HWC_OVERLAY. Wherever we do that, we should probably check the GraphicBuffer format to ensure it's a format we know is opaque. If we don't know it's opaque, we should not use HWC_OVERLAY.

	Sushil Reporter
	Comment 10 • 10 years ago

HWC HAL rely on framework to set correct "Opaque content" flag. So, instead of checking for graphic buffer format in HAL, it is better to check it earlier in Gecko framework or HwcComposer2D (like you mentioned in Comment 4) since it also decides the blending mode of the layer. SF in Android is checking it on Line # 1628 at: https://android.googlesource.com/platform/frameworks/native/+/master/services/surfaceflinger/SurfaceFlinger.cpp

	Michael Vines [:m1] [:evilmachines]
	Comment 11 • 10 years ago

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8)
> Backed out on Aurora with #ifdef B2G. I assumed a=bajaj since she asked me
> to do it...

Ta!

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 12 • 10 years ago

Can you tell me what sets compositionType to HWC_OVERLAY? I can't find that in our code.

	Sushil Reporter
	Comment 13 • 10 years ago

HWC HAL sets composition type to HWC_OVERLAY in the prepare call at http://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.cpp#806. Implementation is in display HAL code on CAF.

	Timothy Nikkel (:tnikkel)
	Comment 14 • 10 years ago

I think this is what roc is suggesting. I do not know this code at all.

Sushil, does this make sense to you? I'm assuming that if we can't do overlay then HWC_FRAMEBUFFER is the correct fallback to use. Also if there are any pixel formats that we want to consider opaque that aren't defined in the android PixelFormat.h we will need to add them here. Can we modify the array of layers here like this? Do we need to make any other changes when changing the composition type of a layer at this point?

	Sushil Reporter
	Comment 15 • 10 years ago

Comment on attachment 8498384 [details] [diff] [review]
disable overlay if format has alpha

Review of attachment 8498384 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/HwcComposer2D.cpp
@@ +797,5 @@
> +                case PIXEL_FORMAT_OPAQUE:
> +                    break;
> +                default:
> +                    // Not a format we know to be opaque, can't use overlay.
> +                    mList->hwLayers[j].compositionType = HWC_FRAMEBUFFER;

It is too late to change composition type here. Before hwc prepare call, HwcComposer2D needs to set "HWC_SKIP_LAYER" flag on the translucent HWC layer to tell HAL that this layer needs to be composed by GPU.

	Sushil Reporter
	Comment 16 • 10 years ago

Plz check Comment 4 by roc, you can set HWC_SKIP_LAYER flag after line: http://mxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.cpp#429 & the switch-case can be moved in IsOpaqueFormat() api.

	Timothy Nikkel (:tnikkel)
	Comment 17 • 10 years ago

Okay, thanks for the feedback. I was a little confused if I needed to set HWC_SKIP_LAYER on the layer when it's not opaque, because that doesn't seem to make sense because we don't currently do it if the layer is not opaque. So I left that out. Let me know what you think.

	Sushil Reporter
	Comment 18 • 10 years ago

Comment on attachment 8498604 [details] [diff] [review]
use format to determine opaque-ness

Review of attachment 8498604 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/gonk/HwcComposer2D.cpp
@@ +421,5 @@
>      int current = mList ? mList->numHwLayers : 0;
>  
> +    bool isOpaque = (opacity == 0xFF) &&
> +      ((aLayer->GetContentFlags() & Layer::CONTENT_OPAQUE) ||
> +       IsOpaqueFormat(state.mSurface->getPixelFormat()));

This is OK. So what is the plan here? Will getPixelFormat() API return "PIXEL_FORMAT_OPAQUE" for Video layer ?

	Timothy Nikkel (:tnikkel)
	Comment 19 • 10 years ago

I think the idea is that we add all formats that we know to be opaque to the switch statement (based on comment 4).

	Timothy Nikkel (:tnikkel)
	Comment 20 • 10 years ago

Comment on attachment 8498604 [details] [diff] [review]
use format to determine opaque-ness

So is this good enough then?

	Sushil Reporter
	Comment 21 • 10 years ago

(In reply to Timothy Nikkel (:tn) from comment #20)
> Comment on attachment 8498604 [details] [diff] [review]
> use format to determine opaque-ness
> 
> So is this good enough then?

No. Please add all YUV (Video layer) formats in switch, if you are planning to restore the patch of Bug 1051636.

	Timothy Nikkel (:tnikkel)
	Comment 22 • 10 years ago

(In reply to Sushil from comment #21)
> No. Please add all YUV (Video layer) formats in switch, if you are planning
> to restore the patch of Bug 1051636.

Where can I find the list of these formats? I was using the list from PixelFormat.h (https://android.googlesource.com/platform/frameworks/native/+/6c14f0ad82be418c742e56fe586657ea0f394b05/include/ui/PixelFormat.h) it doesn't include any YUV formats.

	Sushil Reporter
	Comment 23 • 10 years ago

I am not aware of list of all possible YUV formats. You can refer to https://github.com/android/platform_system_core/blob/master/include/system/graphics.h#L44 for defined HAL pixel formats. Plus there can be additional OEM specific YUV formats, which normally have higher values. So based on a certain range, you could check whether it is YUV (hence Opaque).

Isn't a YUV layer named as "ImageLayerComposite" ? If this can be way to detect it. I don't know if an RGBA image layer can also use same name.

	Timothy Nikkel (:tnikkel)
	Comment 24 • 10 years ago

(In reply to Sushil from comment #23)
> Isn't a YUV layer named as "ImageLayerComposite" ? If this can be way to
> detect it. I don't know if an RGBA image layer can also use same name.

I think ImageLayerComposite can hold any format, it just usually contains YUV from video.

	Timothy Nikkel (:tnikkel)
	Comment 25 • 10 years ago

A couple of the formats don't seem to be defined in whatever headers are already imported, and I didn't know which header to import to get them. They are commented out in this patch.

	Michael Wu [:mwu]
	Comment 26 • 10 years ago

I would actually reverse the logic - we know pretty well which formats have alpha, and everything else and things that are vendor defined are usually opaque.

	Sushil Reporter
	Comment 27 • 10 years ago

Yes, I was thinking the same. On the other hand, I think this issue should ideally be fixed in Video / Camera HAL or clients in Gecko, who are actually providing the rendered buffer (including OEM specific formats). If they can set CONTENT_OPAQUE flag in "mContentFlags", then "aLayer->GetContentFlags()" will return correct value.

	Sushil Reporter
	Comment 28 • 10 years ago

Comment on attachment 8499885 [details] [diff] [review]
use format to determine opaque-ness

This patch does not work for pixel format of Video layer in Video playback. You can check on Flame device.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 29 • 10 years ago

(In reply to Sushil from comment #27)
> If they can
> set CONTENT_OPAQUE flag in "mContentFlags", then "aLayer->GetContentFlags()"
> will return correct value.

I've already explained a couple of times why we can't do that.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 30 • 10 years ago

I think here we should add an opaque flag to Image objects. It could just be a virtual IsOpaque method. Then we can propagate that flag through to the compositor and check it in HwcComposer2D.

	bhavana bajaj [:bajaj]
	Comment 31 • 10 years ago

:roc/:tn, are we going to have an updated patch here based on last few comments ? Not sure what/who has the next actions here but this is a blocker for CAF 2.1 release so will need a patch soon.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 32 • 10 years ago

We already hacked in a backout for B2G on Aurora, so AFAIK this is not a problem for FxOS 2.1. The work here is to get a proper fix.

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 36 • 10 years ago

I'm not sure who should review this...

	Sotaro Ikeda [:sotaro]
	Comment 39 • 10 years ago

Comment on attachment 8513995 [details] [diff] [review]
Part 5: Mark surfaces obtained from a bue as opaque

Review of attachment 8513995 [details] [diff] [review]:
-----------------------------------------------------------------

Set to review+ if same flage is set also for ICS. In ICS, AllocateGralloc() is called in GonkNativeWindow::dequeueBuffer().
http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/GonkNativeWindowICS.cpp#328

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 40 • 10 years ago

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bcf6b7826048

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 41 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb5e6a0ac5a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e70901416d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0c83883d8af
https://hg.mozilla.org/integration/mozilla-inbound/rev/36a17c5ca3db
https://hg.mozilla.org/integration/mozilla-inbound/rev/572ad1e78752
https://hg.mozilla.org/integration/mozilla-inbound/rev/52d9cbc1d78e

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 42 • 10 years ago

Bustage fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/35cc4f43b28b

	Ryan VanderMeulen [:RyanVM]
	Comment 43 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/fb5e6a0ac5a9
https://hg.mozilla.org/mozilla-central/rev/7e70901416d9
https://hg.mozilla.org/mozilla-central/rev/a0c83883d8af
https://hg.mozilla.org/mozilla-central/rev/36a17c5ca3db
https://hg.mozilla.org/mozilla-central/rev/572ad1e78752
https://hg.mozilla.org/mozilla-central/rev/52d9cbc1d78e
https://hg.mozilla.org/mozilla-central/rev/35cc4f43b28b

	Robert O'Callahan (:roc) (email my personal email if necessary) Assignee
	Comment 44 • 10 years ago

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ce10665497aa
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/298f1a913d18
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/7d47fc29bbd1
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/1c0e619d3ca5
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/20e0c21679f7
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ccc2c718b661

	[:SalvadorR]
	Comment 45 • 10 years ago

I am unable to verify this issue. It appears to be a back-end related fix.

	Eric Chang [:ericcc] [:echang]
	Comment 46 • 10 years ago

RAT of Camera & Video for regression checking
https://moztrap.mozilla.org/manage/suites/?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-name=media&filter-name=video&filter-name=%5Brat%5D
https://moztrap.mozilla.org/manage/suites/?pagenumber=1&pagesize=20&sortfield=created_on&sortdirection=desc&filter-name=media&filter-name=%5Brat%5D&filter-name=camera