Closed Bug 1100727 Opened 10 years ago Closed 10 years ago

[lollipop] Porting LL FrameBufferSurface to gonk

Categories

(Firefox OS Graveyard :: GonkIntegration, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S9 (21Nov)

People

(Reporter: pchang, Assigned: boris)

References

Details

Attachments

(1 file, 9 obsolete files)

Porting new FrameBufferSurface from LL to gonk.
Blocks: gonk-L
Comment on attachment 8524278 [details] [diff] [review]
Widget/Gonk patch from all-hammerhead

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

::: widget/gonk/libdisplay/FramebufferSurface.cpp
@@ +49,5 @@
>   * This implements the (main) framebuffer management. This class
>   * was adapted from the version in SurfaceFlinger
>   */
> +#if ANDROID_VERSION >= 21
> +FramebufferSurface::FramebufferSurface(int disp, uint32_t width, uint32_t height, uint32_t format, const sp<IGraphicBufferConsumer>& consumer) :

It looks like you're switching from BufferQueue to IGraphicBufferConsumer. IGraphicBufferConsumer should be available in KK. Can you convert KK to use this too?
Assignee: nobody → boris.chiou
(In reply to Michael Wu [:mwu] from comment #2)
> Comment on attachment 8524278 [details] [diff] [review]
> Widget/Gonk patch from all-hammerhead
> 
> Review of attachment 8524278 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/libdisplay/FramebufferSurface.cpp
> @@ +49,5 @@
> >   * This implements the (main) framebuffer management. This class
> >   * was adapted from the version in SurfaceFlinger
> >   */
> > +#if ANDROID_VERSION >= 21
> > +FramebufferSurface::FramebufferSurface(int disp, uint32_t width, uint32_t height, uint32_t format, const sp<IGraphicBufferConsumer>& consumer) :
> 
> It looks like you're switching from BufferQueue to IGraphicBufferConsumer.
> IGraphicBufferConsumer should be available in KK. Can you convert KK to use
> this too?

Thanks, I can check it in flame-kk now. BTW, this patch is just copied from b2g-l branch (patches/all-hammerhead/), so I will also revise the format of this patch.
Hm on second thought, I think it should be possible to adapt the current FramebufferSurface constructor to work here - we'd just have to make sure the function signature changes on KK and newer, but it'll let us avoid duplicating all the code in the constructor.
Use the new API, BufferQueue::createBufferQueue() to new the consumer
and the producer on LL.
Attachment #8524408 - Attachment is obsolete: true
Attachment #8524416 - Flags: feedback?(pchang)
Comment on attachment 8524416 [details] [diff] [review]
[WIP] Port LL FrameBufferSurface to gonk (v1.1)

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

Please consider to rename the GonkDispalyJB to align KK and LL.

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +137,5 @@
> +#else
> +    sp<BufferQueue> bq = new BufferQueue(true, mAlloc);
> +#endif
> +    mFBSurface = new FramebufferSurface(0, mWidth, mHeight, surfaceformat, bq);
> +

I think we could also declare consumer and producer for KK since BufferQuere is their derived class.

sp<BufferQueue>bq = new BufferQueue(mAlloc);
producer = bq;
consumer = bq;

Then we could assign consumer for 'new FramebufferSurface' and produce for 'new Surface'
Attachment #8524416 - Flags: feedback?(pchang)
I don't think there's much value in renaming GonkDisplayJB - it supports everything from JB to L.
(In reply to Michael Wu [:mwu] from comment #8)
> I don't think there's much value in renaming GonkDisplayJB - it supports
> everything from JB to L.

How about rename to GonkDisplay.cpp for latest android versions support?
If there is no big difference between KK and LL, we could seperate JB into GonkDisplayJB.cpp and leave KK/LL in GonkDisplay.cpp.
(In reply to Boris Chiou [:boris] from comment #6)
> Created attachment 8524416 [details] [diff] [review]
> [WIP] Port LL FrameBufferSurface to gonk (v1.1)
> 
> Use the new API, BufferQueue::createBufferQueue() to new the consumer
> and the producer on LL.

Hi Boris

Patch (v1.1) will cause build break due to the underlying code change.
Please adjust it accordingly.
Thanks

diff --git a/widget/gonk/libdisplay/FramebufferSurface.cpp b/widget/gonk/libdisplay/Frame
index 073c9de..351285d 100644
--- a/widget/gonk/libdisplay/FramebufferSurface.cpp
+++ b/widget/gonk/libdisplay/FramebufferSurface.cpp
@@ -116,7 +116,7 @@ status_t FramebufferSurface::nextBuffer(sp<GraphicBuffer>& outBuffer,
         err = releaseBufferLocked(mCurrentBufferSlot, EGL_NO_DISPLAY,
                 EGL_NO_SYNC_KHR);
 #endif
-        if (err != NO_ERROR && err != BufferQueue::STALE_BUFFER_SLOT) {
+        if (err != NO_ERROR && err != IGraphicBufferConsumer::STALE_BUFFER_SLOT) {
             ALOGE("error releasing buffer: %s (%d)", strerror(-err), err);
             return err;
         }
(In reply to wchi from comment #10)
> (In reply to Boris Chiou [:boris] from comment #6)
> > Created attachment 8524416 [details] [diff] [review]
> > [WIP] Port LL FrameBufferSurface to gonk (v1.1)
> > 
> > Use the new API, BufferQueue::createBufferQueue() to new the consumer
> > and the producer on LL.
> 
> Hi Boris
> 
> Patch (v1.1) will cause build break due to the underlying code change.
> Please adjust it accordingly.
> Thanks
> 
> diff --git a/widget/gonk/libdisplay/FramebufferSurface.cpp
> b/widget/gonk/libdisplay/Frame
> index 073c9de..351285d 100644
> --- a/widget/gonk/libdisplay/FramebufferSurface.cpp
> +++ b/widget/gonk/libdisplay/FramebufferSurface.cpp
> @@ -116,7 +116,7 @@ status_t
> FramebufferSurface::nextBuffer(sp<GraphicBuffer>& outBuffer,
>          err = releaseBufferLocked(mCurrentBufferSlot, EGL_NO_DISPLAY,
>                  EGL_NO_SYNC_KHR);
>  #endif
> -        if (err != NO_ERROR && err != BufferQueue::STALE_BUFFER_SLOT) {
> +        if (err != NO_ERROR && err !=
> IGraphicBufferConsumer::STALE_BUFFER_SLOT) {
>              ALOGE("error releasing buffer: %s (%d)", strerror(-err), err);
>              return err;
>          }

Thanks for your help. I will fix this soon.
Use the new API, BufferQueue::createBufferQueue() to new the consumer
and the producer on LL.
Attachment #8525189 - Attachment is obsolete: true
Attachment #8525201 - Flags: feedback?(pchang)
Attachment #8525201 - Flags: feedback?(sotaro.ikeda.g)
Comment on attachment 8525201 [details] [diff] [review]
Port LL FrameBufferSurface to gonk (v1.3)

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

Looks fine. Inline CreateSurface and request review.

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +124,5 @@
>          ALOGW("Couldn't show bootanimation (%s)", strerror(-error));
>  }
>  
> +sp<ANativeWindow>
> +GonkDisplayJB::CreateSurface()

Inline this. No point in making a new function.
(In reply to Michael Wu [:mwu] from comment #14)
> Comment on attachment 8525201 [details] [diff] [review]
> Port LL FrameBufferSurface to gonk (v1.3)
> 
> Review of attachment 8525201 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks fine. Inline CreateSurface and request review.
> 
> ::: widget/gonk/libdisplay/GonkDisplayJB.cpp
> @@ +124,5 @@
> >          ALOGW("Couldn't show bootanimation (%s)", strerror(-error));
> >  }
> >  
> > +sp<ANativeWindow>
> > +GonkDisplayJB::CreateSurface()
> 
> Inline this. No point in making a new function.

Thanks, Michael. I will put this code back.
Comment on attachment 8525201 [details] [diff] [review]
Port LL FrameBufferSurface to gonk (v1.3)

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

LGTM. Regarding to separate the JB related code in this file, I'll leave it up to you

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +132,5 @@
> +    sp<IGraphicBufferConsumer> consumer;
> +    BufferQueue::createBufferQueue(&producer, &consumer, mAlloc);
> +#elif ANDROID_VERSION >= 19
> +    sp<BufferQueue> consumer = new BufferQueue(mAlloc);
> +    sp<IGraphicBufferProducer> producer = consumer;

I would prefer to declare sp<BufferQueue>bq and convert it to consumer and producer.
Attachment #8525201 - Attachment is obsolete: false
Attachment #8525748 - Flags: review?(sotaro.ikeda.g)
Attachment #8525748 - Flags: review?(sotaro.ikeda.g) → review+
Attachment #8525766 - Flags: review?(mwu) → review+
Use the new API, BufferQueue::createBufferQueue() to new the consumer
and the producer on LL.

Fix JB build errors (Android version 18).

Try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e721dead57f
Attachment #8525766 - Attachment is obsolete: true
Attachment #8525815 - Attachment is obsolete: true
Attachment #8525839 - Flags: review+
(In reply to Boris Chiou [:boris] from comment #21)
> Created attachment 8525839 [details] [diff] [review]
> Port LL FrameBufferSurface to gonk. (v2.3, carry mwu's r+)
> 
> Use the new API, BufferQueue::createBufferQueue() to new the consumer
> and the producer on LL.
> 
> Fix JB build errors (Android version 18).
> 
> Try server:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e721dead57f

By the way, I tested this patch on my nexus-5-l device and it looks OK.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/2513d21bbff2
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S9 (21Nov)
You need to log in before you can comment on or make changes to this bug.