Closed Bug 1152135 Opened 4 years ago Closed 4 years ago

Split EGLSurface buffer swap and HWC buffer swap

Categories

(Core :: Graphics, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

(Whiteboard: gfx-noted)

Attachments

(4 files, 17 obsolete files)

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
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.
The patch also remove quirks from GLContextEGL.
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: nobody → sotaro.ikeda.g
Blocks: 1116089
Summary: Modify HwcComposer2D::Render() as not to handle eglSwapBuffers(). → Split EGLSurface buffer swap and HWC buffer swap
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)
Fix nits.
Attachment #8589386 - Attachment is obsolete: true
Attachment #8589386 - Flags: feedback?(mwu)
Attachment #8589402 - Flags: review?(mwu)
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 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+
(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.
Remove HwcComposer2D::Init().
Attachment #8589402 - Attachment is obsolete: true
Attachment #8589760 - Attachment is obsolete: true
Update nits.
Attachment #8589825 - Attachment is obsolete: true
Attachment #8589827 - Flags: review?(jgilbert)
Attachment #8589827 - Flags: review?(jgilbert) → review+
Blocks: 1144012
Blocks: RefactorHwc
Update a comment.
Attachment #8589823 - Attachment is obsolete: true
Attachment #8590251 - Flags: review?(sushilchauhan)
Attachment #8589831 - Flags: review?(nical.bugzilla)
Attachment #8589820 - Flags: review?(mwu)
Attachment #8589820 - Flags: review?(mwu) → review+
Attachment #8589831 - Flags: review?(nical.bugzilla) → review+
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+
I changed the name because if is confusing to Composer2D::TryRender(). It might be better to change TryRender() to TryRenderWithHwc().
Apply the comment. Carry "r=mwu"
Attachment #8590251 - Attachment is obsolete: true
Attachment #8590251 - Flags: review?(sushilchauhan)
Attachment #8591062 - Flags: review+
unbitrot. Carry "r=mwu".
Attachment #8589820 - Attachment is obsolete: true
Attachment #8591177 - Flags: review+
unbitrot. Carry "r=jgilbert".
Attachment #8589827 - Attachment is obsolete: true
Attachment #8591178 - Flags: review+
unbitrot. Carry "r=nical".
Attachment #8589831 - Attachment is obsolete: true
Attachment #8591180 - Flags: review+
Update nits.
Attachment #8591177 - Attachment is obsolete: true
Attachment #8591300 - Flags: review+
Update nits.
Attachment #8591062 - Attachment is obsolete: true
Attachment #8591301 - Flags: review+
correct patch.
Attachment #8591300 - Attachment is obsolete: true
Attachment #8591302 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/29d01ad10d0b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Depends on: 153976
Status: RESOLVED → REOPENED
Depends on: 1153976
No longer depends on: 153976
Resolution: FIXED → ---
This bug caused Bug 1153976. I am going to backout soon.
I used nexus-5-l and emulator ICS, then I did not recognized flame-kk's problem :-(
Fix the regression.
Attachment #8591301 - Attachment is obsolete: true
Attachment #8591930 - Flags: review+
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.
Depends on: 1154313
un-bitrot.
Attachment #8591302 - Attachment is obsolete: true
Attachment #8593501 - Flags: review+
un-bitrot.
Attachment #8591180 - Attachment is obsolete: true
Attachment #8593504 - Flags: review+
Fix ics rendering problem.
Attachment #8591178 - Attachment is obsolete: true
Attachment #8593567 - Flags: review+
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)
I found one mistake in attachment 8593567 [details] [diff] [review].
Flags: needinfo?(sotaro.ikeda.g)
Fix text failure.
Attachment #8593567 - Attachment is obsolete: true
Attachment #8593951 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e3c19212d8d0
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Depends on: 1186031
Blocks: 1225402
No longer blocks: 1225402
You need to log in before you can comment on or make changes to this bug.