Closed Bug 1152370 Opened 6 years ago Closed 6 years ago

Use DisplaySurface as abstract display surface instead of FramebufferSurface

Categories

(Core :: Graphics, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(3 files, 8 obsolete files)

FramebufferSurface is used for gonk's display surface. But it is not the only display surface on gonk. VirtualDisplaySurface also exits. Therefore, as part of multiple display support. It seems better to use DisplaySurface.
Attached patch wip patch (obsolete) — Splinter Review
Attachment #8589706 - Attachment is patch: true
Attachment #8589706 - Attachment mime type: text/x-patch → text/plain
The name of DisplaySurface comes from android. But I modified class relationship a little bit.

In android, DisplaySurface and ConsumerBase are independent each other. FramebufferSurface and VirtualDisplaySurface are derived from them. DisplaySurface is used only by SurfaceFlinger.
  https://github.com/sotaroikeda/android-diagrams/blob/master/graphics/DisplayDevice_4.4.pdf

In attachment 8589706 [details] [diff] [review], DisplaySurface is derived from ConsumerBase from the following reasons.
 - HwcComposer2D does part of android's SurfaceFlinger's role
 - gonk adds some functions to FramebufferSurface to interact with HwcComposer2D.
   To add same functions to VirtualDisplaySurface, DisplaySurface is good place to define the functions.
 - It makes classes handling simpler on gonk.
Assignee: nobody → sotaro.ikeda.g
Attached patch wip patch (obsolete) — Splinter Review
Fix build problems on gonk KK.
Attachment #8589706 - Attachment is obsolete: true
Attached patch wip patch (obsolete) — Splinter Review
Fix ICS build failure.
Attachment #8589741 - Attachment is obsolete: true
Attachment #8589751 - Flags: review?(mwu)
Comment on attachment 8589751 [details] [diff] [review]
wip patch

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

(I'll assume you're asking for feedback rather than review)

Most of this makes sense to me. Can you add VirtualDisplaySurface.(cpp|h) as a separate patch though? It'll make it easier to see what changes you did to the original android code.
Attachment #8589751 - Flags: review?(mwu)
Thanks for checking! I am going to update the patch.
VirtualDisplaySurface sources are from android v5.1 AOSP.
Attachment #8589751 - Attachment is obsolete: true
Update nits.
Attachment #8590367 - Attachment is obsolete: true
correct patch.
Attachment #8590373 - Attachment is obsolete: true
Attachment #8590375 - Flags: feedback?(mwu)
Attachment #8590366 - Attachment description: patch part 1 - Add VirtualDisplaySurface → patch part 1 - Add android aosp VirtualDisplaySurface
Attachment #8590375 - Flags: feedback?(mwu)
Attachment #8590366 - Flags: review?(mwu)
Attachment #8590375 - Flags: review?(mwu)
Blocks: 1116089
un-bitrot.
Attachment #8590375 - Attachment is obsolete: true
Attachment #8590375 - Flags: review?(mwu)
Attachment #8594845 - Flags: review?(mwu)
Comment on attachment 8590366 [details] [diff] [review]
patch part 1 - Add android aosp VirtualDisplaySurface

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

Rubberstamp.
Attachment #8590366 - Flags: review?(mwu) → review+
FWIW, I would also like to have DisplaySurface.h split out. No need to do it now though - just for the next time you refresh the patches.
I took a closer look at the patch, and most of it makes sense *but*... VirtualDisplaySurface isn't used in this patch. It's just dead code. Do you have a patch on top of this which uses VirtualDisplaySurface? It would provide better context for what's going on here. I'm not entirely sure what VirtualDisplaySurface does differently than FramebufferSurface.
Screen recording(Bug 1144103) and Wifi Display(Bug 925615) is going to use VirtualDisplaySurface.

VirtualDisplaySurface class related diagrams in android are like the following.
  https://github.com/sotaroikeda/android-diagrams/blob/master/graphics/DisplayDevice_4.4.pdf
  https://github.com/sotaroikeda/android-diagrams/raw/master/graphics/Presentation_5.1.pdf

It provide a capability that rendering result could be consumed by another system. The another system provide IGraphicBufferProducer to VirtualDisplaySurface. The VirtualDisplaySurface does the composition to buffers of the IGraphicBufferProducer. The rendered buffers are returned to the IGraphicBufferProducer. Then the another system gets the buffers via the IGraphicBufferProducer.
attachment 8594820 [details] [diff] [review] in Bug 1144103 is just a wip patch. And it includes VirtualDisplaySurface by itself.
FramebufferSurface case, rendered buffers are always consumed by hardware composer.
From comment 16, VirtualDisplaySurface could cause a security risk. Therefore, if we want to actually use it within a system, we need to add security checks.
Comment on attachment 8594845 [details] [diff] [review]
patch part 2 - Use DisplaySurface as abstract display surface

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

Ok, I think I understand enough of this to r+ it. The only thing is I couldn't find where the IGraphicBufferProducer comes from in the other WIP patches, but that's fine for now.
Attachment #8594845 - Flags: review?(mwu) → review+
Thanks. IGraphicBufferProducer normally comes from gonk code. SurfaceComposerClient::setDisplaySurface() actually set it. In screenrecord case, prepareVirtualDisplay() call it like the following.
  http://androidxref.com/5.1.0_r1/xref/frameworks/av/cmds/screenrecord/screenrecord.cpp#288
(In reply to Sotaro Ikeda [:sotaro] from comment #21)
> Thanks. IGraphicBufferProducer normally comes from gonk code.
> SurfaceComposerClient::setDisplaySurface() actually set it.

It is only about screenrecord case. Wifi display could be different route.
Split DisplaySurface to different file. Carry "r=mwu".
Attachment #8594845 - Attachment is obsolete: true
Attachment #8596183 - Flags: review+
Carry "r=muw".
Attachment #8596185 - Flags: review+
Fix build problem on android. Carry "r=mwu".
Attachment #8596185 - Attachment is obsolete: true
Attachment #8596290 - Flags: review+
Blocks: 1225402
No longer blocks: 1225402
You need to log in before you can comment on or make changes to this bug.