1152370 - Use DisplaySurface as abstract display surface instead of FramebufferSurface

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Sotaro Ikeda [:sotaro]]

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.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Comment 2

[Sotaro Ikeda [:sotaro]]

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.

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Fix build problems on gonk KK.

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Fix ICS build failure.

Comment 5

[Sotaro Ikeda [:sotaro]]

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=51d041be7141

Comment 6

[Michael Wu [: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.

Comment 7

[Sotaro Ikeda [:sotaro]]

Thanks for checking! I am going to update the patch.

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Add android aosp VirtualDisplaySurface

VirtualDisplaySurface sources are from android v5.1 AOSP.

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Use DisplaySurface as abstract display surface

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Use DisplaySurface as abstract display surface

Update nits.

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Use DisplaySurface as abstract display surface

correct patch.

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Use DisplaySurface as abstract display surface

un-bitrot.

Comment 13

[Michael Wu [:mwu]]

Comment on attachment 8590366 [details] [diff] [review]
patch part 1 - Add android aosp VirtualDisplaySurface

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

Rubberstamp.

Comment 14

[Michael Wu [:mwu]]

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.

Comment 15

[Michael Wu [:mwu]]

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.

Comment 16

[Sotaro Ikeda [:sotaro]]

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.

Comment 17

[Sotaro Ikeda [:sotaro]]

attachment 8594820 [details] [diff] [review] in Bug 1144103 is just a wip patch. And it includes VirtualDisplaySurface by itself.

Comment 18

[Sotaro Ikeda [:sotaro]]

FramebufferSurface case, rendered buffers are always consumed by hardware composer.

Comment 19

[Sotaro Ikeda [:sotaro]]

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 20

[Michael Wu [:mwu]]

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.

Comment 21

[Sotaro Ikeda [:sotaro]]

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

Comment 22

[Sotaro Ikeda [:sotaro]]

(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.

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - Add DisplaySurface

Split DisplaySurface to different file. Carry "r=mwu".

Comment 24

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Use DisplaySurface

Carry "r=muw".

Comment 25

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Use DisplaySurface

Fix build problem on android. Carry "r=mwu".

Comment 26

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=03be7bfa3fde

Comment 27

[Sotaro Ikeda [:sotaro]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/6869f1714918
 https://hg.mozilla.org/integration/mozilla-inbound/rev/a67f53938736
 https://hg.mozilla.org/integration/mozilla-inbound/rev/1d33044efb31

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6869f1714918
https://hg.mozilla.org/mozilla-central/rev/a67f53938736
https://hg.mozilla.org/mozilla-central/rev/1d33044efb31