Closed
Bug 1152135
Opened 10 years ago
Closed 10 years ago
Split EGLSurface buffer swap and HWC buffer swap
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: sotaro, Assigned: sotaro)
References
Details
(Whiteboard: gfx-noted)
Attachments
(4 files, 17 obsolete files)
(deleted),
patch
|
sotaro
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
sotaro
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
sotaro
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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.
Assignee | ||
Comment 1•10 years ago
|
||
The patch also remove quirks from GLContextEGL.
Assignee | ||
Comment 2•10 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•10 years ago
|
Assignee: nobody → sotaro.ikeda.g
Assignee | ||
Updated•10 years ago
|
Summary: Modify HwcComposer2D::Render() as not to handle eglSwapBuffers(). → Split EGLSurface buffer swap and HWC buffer swap
Assignee | ||
Comment 3•10 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•10 years ago
|
||
Fix nits.
Attachment #8589386 -
Attachment is obsolete: true
Attachment #8589386 -
Flags: feedback?(mwu)
Assignee | ||
Updated•10 years ago
|
Attachment #8589402 -
Flags: review?(mwu)
Assignee | ||
Comment 5•10 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•10 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•10 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•10 years ago
|
||
Remove HwcComposer2D::Init().
Attachment #8589402 -
Attachment is obsolete: true
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8589760 -
Attachment is obsolete: true
Assignee | ||
Comment 10•10 years ago
|
||
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Comment 12•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8589827 -
Flags: review?(jgilbert)
Updated•10 years ago
|
Attachment #8589827 -
Flags: review?(jgilbert) → review+
Updated•10 years ago
|
Blocks: RefactorHwc
Assignee | ||
Comment 14•10 years ago
|
||
Update a comment.
Attachment #8589823 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8590251 -
Flags: review?(sushilchauhan)
Assignee | ||
Updated•10 years ago
|
Attachment #8589831 -
Flags: review?(nical.bugzilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8589820 -
Flags: review?(mwu)
Updated•10 years ago
|
Attachment #8589820 -
Flags: review?(mwu) → review+
Updated•10 years ago
|
Attachment #8589831 -
Flags: review?(nical.bugzilla) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8590251 -
Flags: review?(mwu)
Comment 15•10 years ago
|
||
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•10 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•10 years ago
|
||
Apply the comment. Carry "r=mwu"
Attachment #8590251 -
Attachment is obsolete: true
Attachment #8590251 -
Flags: review?(sushilchauhan)
Attachment #8591062 -
Flags: review+
Updated•10 years ago
|
Whiteboard: gfx-noted
Assignee | ||
Comment 18•10 years ago
|
||
unbitrot. Carry "r=mwu".
Attachment #8589820 -
Attachment is obsolete: true
Attachment #8591177 -
Flags: review+
Assignee | ||
Comment 19•10 years ago
|
||
unbitrot. Carry "r=jgilbert".
Attachment #8589827 -
Attachment is obsolete: true
Attachment #8591178 -
Flags: review+
Assignee | ||
Comment 20•10 years ago
|
||
unbitrot. Carry "r=nical".
Attachment #8589831 -
Attachment is obsolete: true
Attachment #8591180 -
Flags: review+
Assignee | ||
Comment 21•10 years ago
|
||
Update nits.
Attachment #8591177 -
Attachment is obsolete: true
Attachment #8591300 -
Flags: review+
Assignee | ||
Comment 22•10 years ago
|
||
Update nits.
Attachment #8591062 -
Attachment is obsolete: true
Attachment #8591301 -
Flags: review+
Assignee | ||
Comment 23•10 years ago
|
||
correct patch.
Attachment #8591300 -
Attachment is obsolete: true
Attachment #8591302 -
Flags: review+
Assignee | ||
Comment 24•10 years ago
|
||
Assignee | ||
Comment 25•10 years ago
|
||
Comment 26•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 27•10 years ago
|
||
This bug caused Bug 1153976. I am going to backout soon.
Assignee | ||
Comment 28•10 years ago
|
||
Assignee | ||
Comment 29•10 years ago
|
||
I used nexus-5-l and emulator ICS, then I did not recognized flame-kk's problem :-(
Assignee | ||
Comment 30•10 years ago
|
||
Fix the regression.
Attachment #8591301 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8591930 -
Flags: review+
Assignee | ||
Comment 31•10 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.
Comment 32•10 years ago
|
||
(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 | ||
Comment 33•10 years ago
|
||
un-bitrot.
Attachment #8591302 -
Attachment is obsolete: true
Attachment #8593501 -
Flags: review+
Assignee | ||
Comment 34•10 years ago
|
||
un-bitrot.
Attachment #8591180 -
Attachment is obsolete: true
Attachment #8593504 -
Flags: review+
Assignee | ||
Comment 35•10 years ago
|
||
Fix ics rendering problem.
Attachment #8591178 -
Attachment is obsolete: true
Attachment #8593567 -
Flags: review+
Assignee | ||
Comment 36•10 years ago
|
||
Assignee | ||
Comment 37•10 years ago
|
||
Checked the patches on flame-kk and hamachi.
Assignee | ||
Comment 38•10 years ago
|
||
Comment 39•10 years ago
|
||
Comment 40•10 years ago
|
||
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•10 years ago
|
||
I found one mistake in attachment 8593567 [details] [diff] [review].
Flags: needinfo?(sotaro.ikeda.g)
Assignee | ||
Comment 42•10 years ago
|
||
Fix text failure.
Attachment #8593567 -
Attachment is obsolete: true
Attachment #8593951 -
Flags: review+
Assignee | ||
Comment 43•10 years ago
|
||
Comment 44•10 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•