Split EGLSurface buffer swap and HWC buffer swap

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

Trunk
mozilla40
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

(Whiteboard: gfx-noted)

Attachments

(4 attachments, 17 obsolete attachments)

9.38 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
5.53 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
3.43 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
2.69 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
HwcComposer2D::Render() handle primary display's eglSwapBuffers() and HWC's buffer swap. It cause a problem when we want to support multiple display. It seems better to split HWC's buffer swap and EGLSurface's buffer swap.

It seems also better to remove gonk specific quirks from GLContextEGL.
(Assignee)

Comment 1

4 years ago
Created attachment 8589386 [details] [diff] [review]
patch - Split EGLSurface buffer swap and HWC buffer swap

The patch also remove quirks from GLContextEGL.
(Assignee)

Comment 2

4 years ago
HWC does not need EGLDisplay nor EGLSurface except HWC v1.0. And Firefox OS does not support HWC v1.0.
The following is related code.
  https://dxr.mozilla.org/mozilla-central/source/widget/gonk/libdisplay/GonkDisplayJB.cpp#73

Android's hwc_display_contents_1 have fields for EGLDisplay and EGLSurface. They are used only on HWC_DEVICE_VERSION_1_0.
  http://androidxref.com/5.1.0_r1/xref/hardware/libhardware/include/hardware/hwcomposer.h#359

Therefore, we can split EGLSurface buffer swap and HWC buffer swap. But HwcComposer2D still needs EGLDisplay nor EGLSurface only to support BLIT Composition. When BLIT Composition is done, HwcComposer2D::Render() is not called. Therefore it does not affect to the modification of HwcComposer2D::Render().

The following is BLIT Composition related code. 
  https://dxr.mozilla.org/mozilla-central/source/widget/gonk/HwcComposer2D.cpp#738
(Assignee)

Updated

4 years ago
Assignee: nobody → sotaro.ikeda.g
(Assignee)

Updated

4 years ago
Blocks: 1116089
(Assignee)

Updated

4 years ago
Summary: Modify HwcComposer2D::Render() as not to handle eglSwapBuffers(). → Split EGLSurface buffer swap and HWC buffer swap
(Assignee)

Comment 3

4 years ago
Comment on attachment 8589386 [details] [diff] [review]
patch - Split EGLSurface buffer swap and HWC buffer swap

mwu, can you provide a feedback?
Attachment #8589386 - Flags: feedback?(mwu)
(Assignee)

Comment 4

4 years ago
Created attachment 8589402 [details] [diff] [review]
patch - Split EGLSurface buffer swap and HWC buffer swap

Fix nits.
Attachment #8589386 - Attachment is obsolete: true
Attachment #8589386 - Flags: feedback?(mwu)
(Assignee)

Updated

4 years ago
Attachment #8589402 - Flags: review?(mwu)
(Assignee)

Comment 5

4 years ago
Comment on attachment 8589402 [details] [diff] [review]
patch - Split EGLSurface buffer swap and HWC buffer swap

Sorry, I am just asking feedback. Thanks.
Attachment #8589402 - Flags: review?(mwu) → feedback?(mwu)

Comment 6

4 years ago
Comment on attachment 8589402 [details] [diff] [review]
patch - Split EGLSurface buffer swap and HWC buffer swap

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

Nice! This is definitely a step in the right direction.

::: widget/gonk/nsWindow.cpp
@@ +604,5 @@
> +        // Called before primary display's EGLSurface creation.
> +        // Initialize HwcComposer2D
> +        HwcComposer2D* hwc = HwcComposer2D::GetInstance();
> +        MOZ_ASSERT(!hwc->Initialized());
> +        hwc->Init();

GetNativeData(NS_NATIVE_WINDOW) might only be called once right now, but I'm not sure if that's a good assumption to make..

@@ +615,5 @@
> +void
> +nsWindow::SetNativeData(uint32_t aDataType, uintptr_t aVal)
> +{
> +    switch (aDataType) {
> +    case NS_NATIVE_OPENGL_CONTEXT:

This is great. I didn't know this existed.
Attachment #8589402 - Flags: feedback?(mwu) → feedback+
(Assignee)

Comment 7

4 years ago
(In reply to Michael Wu [:mwu] from comment #6)
> Comment on attachment 8589402 [details] [diff] [review]
> patch - Split EGLSurface buffer swap and HWC buffer swap
> 
> Review of attachment 8589402 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice! This is definitely a step in the right direction.
> 
> ::: widget/gonk/nsWindow.cpp
> @@ +604,5 @@
> > +        // Called before primary display's EGLSurface creation.
> > +        // Initialize HwcComposer2D
> > +        HwcComposer2D* hwc = HwcComposer2D::GetInstance();
> > +        MOZ_ASSERT(!hwc->Initialized());
> > +        hwc->Init();
> 
> GetNativeData(NS_NATIVE_WINDOW) might only be called once right now, but I'm
> not sure if that's a good assumption to make..

Yhea, the assumption is correct right now. I also not sure about the future. It might be better to change it. We could remove Init() at all.

In android, there is a case that multiple EGLSurface is created for one BufferQueue. For example, an application renders to Surface(BufferQueue) then mediaserver side create a new Surface for the BufferQueue and render to it.
(Assignee)

Comment 8

4 years ago
Created attachment 8589760 [details] [diff] [review]
patch - Split EGLSurface buffer swap and HWC buffer swap

Remove HwcComposer2D::Init().
Attachment #8589402 - Attachment is obsolete: true
(Assignee)

Comment 9

4 years ago
Created attachment 8589820 [details] [diff] [review]
patch part 1 - Change to nsWindow and GonkDisplay
Attachment #8589760 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
Created attachment 8589823 [details] [diff] [review]
patch part 2 - Change to Composer2D
(Assignee)

Comment 11

4 years ago
Created attachment 8589825 [details] [diff] [review]
patch part 3 - Change to gfx Layers
(Assignee)

Comment 12

4 years ago
Created attachment 8589827 [details] [diff] [review]
patch part 4 - Change to GLContextEGL
(Assignee)

Comment 13

4 years ago
Created attachment 8589831 [details] [diff] [review]
patch part 3 - Change to gfx Layers

Update nits.
Attachment #8589825 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8589827 - Flags: review?(jgilbert)
Attachment #8589827 - Flags: review?(jgilbert) → review+
Blocks: 1144012
Blocks: 1138811
(Assignee)

Comment 14

4 years ago
Created attachment 8590251 [details] [diff] [review]
patch part 2 - Change to Composer2D

Update a comment.
Attachment #8589823 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8590251 - Flags: review?(sushilchauhan)
(Assignee)

Updated

4 years ago
Attachment #8589831 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

4 years ago
Attachment #8589820 - Flags: review?(mwu)

Updated

4 years ago
Attachment #8589820 - Flags: review?(mwu) → review+
Attachment #8589831 - Flags: review?(nical.bugzilla) → review+
(Assignee)

Updated

4 years ago
Attachment #8590251 - Flags: review?(mwu)
Comment on attachment 8590251 [details] [diff] [review]
patch part 2 - Change to Composer2D

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

The changes all look fine to me, though renaming Render to SwapBuffers seems a bit confusing. SwapBuffers sounds like something related to EGL, but now we're not calling eglSwapBuffers at all in this path. The previous name seems to be more accurate for what it actually does.
Attachment #8590251 - Flags: review?(mwu) → review+
(Assignee)

Comment 16

4 years ago
I changed the name because if is confusing to Composer2D::TryRender(). It might be better to change TryRender() to TryRenderWithHwc().
(Assignee)

Comment 17

4 years ago
Created attachment 8591062 [details] [diff] [review]
patch part 2 - Change to Composer2D

Apply the comment. Carry "r=mwu"
Attachment #8590251 - Attachment is obsolete: true
Attachment #8590251 - Flags: review?(sushilchauhan)
Attachment #8591062 - Flags: review+
Whiteboard: gfx-noted
(Assignee)

Comment 18

4 years ago
Created attachment 8591177 [details] [diff] [review]
patch part 1 - Change to nsWindow and GonkDisplay

unbitrot. Carry "r=mwu".
Attachment #8589820 - Attachment is obsolete: true
Attachment #8591177 - Flags: review+
(Assignee)

Comment 19

4 years ago
Created attachment 8591178 [details] [diff] [review]
patch part 4 - Change to GLContextEGL

unbitrot. Carry "r=jgilbert".
Attachment #8589827 - Attachment is obsolete: true
Attachment #8591178 - Flags: review+
(Assignee)

Comment 20

4 years ago
Created attachment 8591180 [details] [diff] [review]
patch part 3 - Change to gfx Layers

unbitrot. Carry "r=nical".
Attachment #8589831 - Attachment is obsolete: true
Attachment #8591180 - Flags: review+
(Assignee)

Comment 21

4 years ago
Created attachment 8591300 [details] [diff] [review]
patch part 2 - Change to Composer2D

Update nits.
Attachment #8591177 - Attachment is obsolete: true
Attachment #8591300 - Flags: review+
(Assignee)

Comment 22

4 years ago
Created attachment 8591301 [details] [diff] [review]
patch part 2 - Change to Composer2D

Update nits.
Attachment #8591062 - Attachment is obsolete: true
Attachment #8591301 - Flags: review+
(Assignee)

Comment 23

4 years ago
Created attachment 8591302 [details] [diff] [review]
patch part 1 - Change to nsWindow and GonkDisplay

correct patch.
Attachment #8591300 - Attachment is obsolete: true
Attachment #8591302 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/29d01ad10d0b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(Assignee)

Updated

4 years ago
Depends on: 153976
(Assignee)

Updated

4 years ago
Status: RESOLVED → REOPENED
Depends on: 1153976
No longer depends on: 153976
Resolution: FIXED → ---
(Assignee)

Comment 27

4 years ago
This bug caused Bug 1153976. I am going to backout soon.
(Assignee)

Comment 29

4 years ago
I used nexus-5-l and emulator ICS, then I did not recognized flame-kk's problem :-(
(Assignee)

Comment 30

4 years ago
Created attachment 8591930 [details] [diff] [review]
patch part 2 - Change to Composer2D

Fix the regression.
Attachment #8591301 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8591930 - Flags: review+
(Assignee)

Comment 31

4 years ago
attachment 8591930 [details] [diff] [review] fixed the regression. But during debugging, I found another problem. When "Hardware Composer" usage is disabled by Setting app. It also disable hwc's buffer swap. Its problem also need to be addressed before re-checkin.
(In reply to Sotaro Ikeda [:sotaro] from comment #28)
> backed out.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/2b3889fb3a0c

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/2b3889fb3a0c

Triggering new B2G nightlies on the merge.
(Assignee)

Updated

4 years ago
Depends on: 1154313
(Assignee)

Comment 33

4 years ago
Created attachment 8593501 [details] [diff] [review]
patch part 1 - Change to nsWindow and GonkDisplay

un-bitrot.
Attachment #8591302 - Attachment is obsolete: true
Attachment #8593501 - Flags: review+
(Assignee)

Comment 34

4 years ago
Created attachment 8593504 [details] [diff] [review]
patch part 3 - Change to gfx Layers

un-bitrot.
Attachment #8591180 - Attachment is obsolete: true
Attachment #8593504 - Flags: review+
(Assignee)

Comment 35

4 years ago
Created attachment 8593567 [details] [diff] [review]
patch part 4 - Change to GLContextEGL

Fix ics rendering problem.
Attachment #8591178 - Attachment is obsolete: true
Attachment #8593567 - Flags: review+
(Assignee)

Comment 37

4 years ago
Checked the patches on flame-kk and hamachi.
sorry had to back this out for perma test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8974459&repo=mozilla-inbound
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 41

4 years ago
I found one mistake in attachment 8593567 [details] [diff] [review].
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 42

4 years ago
Created attachment 8593951 [details] [diff] [review]
patch part 4 - Change to GLContextEGL

Fix text failure.
Attachment #8593567 - Attachment is obsolete: true
Attachment #8593951 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e3c19212d8d0
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

3 years ago
Depends on: 1186031
(Assignee)

Updated

3 years ago
Blocks: 1225402
(Assignee)

Updated

3 years ago
No longer blocks: 1225402
You need to log in before you can comment on or make changes to this bug.