946245 - [Display][gonk-kk] Porting GonkDisplay, nativewindow and libui

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Kai-Zhen Li [:seinlin][:kli]]

GonkDisplay, nativewindow and libui are compiled failed in gonk-kk.

Comment 1

[Solomon Chiu [:schiu]]

Attachment: 0001-Modify-FramebufferSurafce.cpp-compiling-passed.patch

Comment 2

[Solomon Chiu [:schiu]]

Stack of current B2G crash:

#0  0xb5e9a7de in mozalloc_abort (
    msg=msg@entry=0xbef4776c "[Parent 1032] ###!!! ABORT: constructor for actor failed: file PLayerTransactionChild.cpp, line 133") at ../../../gecko/memory/mozalloc/mozalloc_abort.cpp:30
#1  0xb4fc58ee in Abort (
    aMsg=0xbef4776c "[Parent 1032] ###!!! ABORT: constructor for actor failed: file PLayerTransactionChild.cpp, line 133") at ../../../gecko/xpcom/base/nsDebugImpl.cpp:425
#2  NS_DebugBreak (aSeverity=<optimized out>, aStr=0xb5f5dd50 "constructor for actor failed", aExpr=0x0, 
    aFile=0xb5f788a2 "PLayerTransactionChild.cpp", aLine=133) at ../../../gecko/xpcom/base/nsDebugImpl.cpp:382
#3  0xb51546fc in mozilla::layers::PLayerTransactionChild::SendPGrallocBufferConstructor (this=this@entry=0xaf450ee0, 
    actor=<error reading variable: Cannot access memory at address 0xffffffcd>, size=..., format=@0xbef47c14: 2, 
    usage=@0xbef47c10: 307, handle=handle@entry=0xbef47c34) at PLayerTransactionChild.cpp:133
#4  0xb515475e in mozilla::layers::PLayerTransactionChild::SendPGrallocBufferConstructor (this=0xaf450ee0, size=..., 
    format=@0xbef47c14: 2, usage=@0xbef47c10: 307, handle=handle@entry=0xbef47c34) at PLayerTransactionChild.cpp:80
#5  0xb520fcca in mozilla::layers::ShadowLayerForwarder::AllocGrallocBuffer (this=<optimized out>, aSize=..., 
    aFormat=2, aUsage=307, aHandle=0xbef47c34) at ../../../gecko/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp:356
#6  0xb520fee0 in PlatformAllocSurfaceDescriptor (aBuffer=0xae8029ec, aCaps=<optimized out>, 
    aContent=GFX_CONTENT_COLOR, aSize=..., this=0xaf450eb0)
    at ../../../gecko/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp:418
#7  mozilla::layers::ISurfaceAllocator::PlatformAllocSurfaceDescriptor (this=0xaf450eb0, aSize=..., 
    aContent=GFX_CONTENT_COLOR, aCaps=<optimized out>, aBuffer=0xae8029ec)
    at ../../../gecko/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp:360

Comment 3

[Solomon Chiu [:schiu]]

Attachment: 0002-Fix-GraphicBuffer-usage-in-ShadowLayerUtilsGralloc.patch

Comment 4

[Solomon Chiu [:schiu]]

WIP:
Just fixed the usage of GraphicBuffer in ShadowLayerUtilsGralloc. Now I can see home screen presented.

Comment 5

[Peter Chang[:pchang]]

I just checked the patches. Could you generate the patch direct from current code base? Not based on the kitkat one.

Comment 6

[Peter Chang[:pchang]]

Comment on attachment 8346471 [details] [diff] [review]
0001-Modify-FramebufferSurafce.cpp-compiling-passed.patch

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

::: widget/gonk/libdisplay/FramebufferSurface.cpp
@@ +55,2 @@
>          const sp<IGraphicBufferConsumer>& consumer) :
> +    ConsumerBase(consumer, true),

you can just add the following android version checking instead of duplicating another copy for FramebufferSurface constructor.

#if ANDROID_VERSION >= 19
ConsumerBase(consumer, true),
#endif

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +123,5 @@
>      sp<SurfaceTextureClient> stc = new SurfaceTextureClient(static_cast<sp<ISurfaceTexture> >(mFBSurface->getBufferQueue()));
>  #else
>      // Todo kitkat
> +
> +#if ANDROID_VERSION == 19

This should be >= 19

Comment 7

[Solomon Chiu [:schiu]]

Attachment: adapt-to-jb43-codebase.diff

Comment 8

[Solomon Chiu [:schiu]]

Hi Peter,

Just upload the diff file based on current Nexus-4 JB43 codebase. The top commit is listed as following:

commit 56c7fcbb4ee61875b000317b892fdefddd14c588
Merge: 4c21bcf 51be634
Author: Wes Kocher <[e-mail]>
Date:   Mon Dec 16 21:33:31 2013 -0800

    Merge inbound to m-c

Meanwhile, for your above recommandation, I got your point, however the content of FramebufferSurface's constructor changes a lot, not only one line change at 'ConsumerBase(consumer, true)'. Anyway I will seek alternative way to simplify my change.

Comment 9

[Peter Chang[:pchang]]

Comment on attachment 8348641 [details] [diff] [review]
adapt-to-jb43-codebase.diff

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

::: gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
@@ +42,4 @@
>  ParamTraits<MagicGrallocBufferHandle>::Write(Message* aMsg,
>                                               const paramType& aParam)
>  {
> +#if ANDROID_VERSION == 19

Should be >=19

@@ +55,5 @@
>    char data[nbytes];
>    int fds[nfds];
> +
> +#if ANDROID_VERSION == 19
> +  // Make a copy of "data" and "fds" for flatten() to avoid casting problem

Need more comment to explain why we do this way.

::: widget/gonk/libdisplay/FramebufferSurface.cpp
@@ +51,5 @@
>   */
>  
> +#if ANDROID_VERSION == 19
> +FramebufferSurface::FramebufferSurface(int disp, uint32_t width, uint32_t height, uint32_t format,
> +        const sp<IGraphicBufferConsumer>& consumer) :

Is it possible to merge different interface by the following way?

http://dxr.mozilla.org/mozilla-central/source/widget/gonk/nativewindow/GonkBufferQueue.h#38

::: widget/gonk/libdisplay/FramebufferSurface.h
@@ +45,5 @@
>      status_t setUpdateRectangle(const Rect& updateRect);
>      status_t compositionComplete();
>  
>      virtual void dump(String8& result);
> +    // Todo kitkat

remove kitkat

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +125,5 @@
>      sp<SurfaceTextureClient> stc = new SurfaceTextureClient(static_cast<sp<ISurfaceTexture> >(mFBSurface->getBufferQueue()));
>  #else
> +
> +#if ANDROID_VERSION == 19
> +    sp<Surface> stc = new Surface(static_cast<sp<IGraphicBufferProducer> >(bq));

may be call mFBSurface->getBufferQueue()

Comment 10

[Sotaro Ikeda [:sotaro]]

(In reply to Solomon Chiu [:schiu] from comment #1)
> Created attachment 8346471 [details] [diff] [review]
> 0001-Modify-FramebufferSurafce.cpp-compiling-passed.patch

Comment and a question to attachment 8346471 [details] [diff] [review]. Can't we use Flattenable<GraphicBuffer> as replacement of Flattenablein JB. By using it, more code could be shared with older android.

>+  // In Kitkat, flatten() will change the value of nbytes and nfds.
>+  // need to change them back.
>+  nbytes = buffer->getFlattenedSize();
>+  nfds = buffer->getFdCount();

Can you explain the above part? Can you explain about it by using actual android source code? KitKat's GraphicBuffer seems not update the count and nbytes and nfds. I found the Parcel seems to add padding, but it is not flatten().

Comment 11

[Solomon Chiu [:schiu]]

Attachment: adapt-to-jb43-codebase-refine-1.diff

Comment 12

[Solomon Chiu [:schiu]]

> @@ +55,5 @@
> >    char data[nbytes];
> >    int fds[nfds];
> > +
> > +#if ANDROID_VERSION == 19
> > +  // Make a copy of "data" and "fds" for flatten() to avoid casting problem
> 
> Need more comment to explain why we do this way.
> 

Peter,

Please check my latest change according to your recommendation. And about above comment, it dues to flatten's prototype has been change to "flatten(void*& buffer, size_t& size, int*& fds, size_t& count)", casting "data" and "fds" directly is not allowed from compiler's error message.

Comment 13

[Solomon Chiu [:schiu]]

(In reply to Sotaro Ikeda [:sotaro] (PTO Dec. 21 ~ Jan. 5) from comment #10)
> (In reply to Solomon Chiu [:schiu] from comment #1)
> > Created attachment 8346471 [details] [diff] [review]
> > 0001-Modify-FramebufferSurafce.cpp-compiling-passed.patch
> 
> Comment and a question to attachment 8346471 [details] [diff] [review].
> Can't we use Flattenable<GraphicBuffer> as replacement of Flattenablein JB.
> By using it, more code could be shared with older android.

Sotaro,

Please also check my latest change according to your recommendation. Hope I get your point.

> 
> >+  // In Kitkat, flatten() will change the value of nbytes and nfds.
> >+  // need to change them back.
> >+  nbytes = buffer->getFlattenedSize();
> >+  nfds = buffer->getFdCount();
> 
> Can you explain the above part? Can you explain about it by using actual
> android source code? KitKat's GraphicBuffer seems not update the count and
> nbytes and nfds. I found the Parcel seems to add padding, but it is not
> flatten().

Google made following change to the flatten funciton in KitKat. If I don't recover the corresponding variable, the system will crashed in PLayerTransactionChild::SendPGrallocBufferConstructor().


status_t GraphicBuffer::flatten(void*& buffer, size_t& size, int*& fds, size_t& count) const {
    :
    buffer = reinterpret_cast<void*>(static_cast<int*>(buffer) + sizeNeeded);
    size -= sizeNeeded;
    fds += handle->numFds;
    count -= handle->numFds;

    return NO_ERROR;
}

Comment 14

[Sotaro Ikeda [:sotaro]]

(In reply to Solomon Chiu [:schiu] from comment #13)
> (In reply to Sotaro Ikeda [:sotaro] (PTO Dec. 21 ~ Jan. 5) from comment #10)
> > (In reply to Solomon Chiu [:schiu] from comment #1)
> > > Created attachment 8346471 [details] [diff] [review]
> > > 0001-Modify-FramebufferSurafce.cpp-compiling-passed.patch
> > 
> > Comment and a question to attachment 8346471 [details] [diff] [review].
> > Can't we use Flattenable<GraphicBuffer> as replacement of Flattenablein JB.
> > By using it, more code could be shared with older android.
> 
> Sotaro,
> 
> Please also check my latest change according to your recommendation. Hope I
> get your point.

The patch seems OK. I reconfirmed the difference between JB and KK. This difference is unavoidable.

> Google made following change to the flatten funciton in KitKat. If I don't
> recover the corresponding variable, the system will crashed in
> PLayerTransactionChild::SendPGrallocBufferConstructor().

I rechecked the JB and KK code. I understand the meaning the code. Thanks for the explanation.

Comment 15

[Peter Chang[:pchang]]

Comment on attachment 8350565 [details] [diff] [review]
adapt-to-jb43-codebase-refine-1.diff

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

Please check below comment and obsolete the old files which didn't need anymore.

::: gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
@@ +42,5 @@
>  ParamTraits<MagicGrallocBufferHandle>::Write(Message* aMsg,
>                                               const paramType& aParam)
>  {
> +#if ANDROID_VERSION >= 19
> +  sp<GraphicBuffer> flattenable = aParam.mGraphicBuffer;

what error if we change to Flattenable *flattenable = aParam.mGraphicBuffer.get();?

And you don't have to separate nbytes/nfds

@@ +54,5 @@
>  
>    char data[nbytes];
>    int fds[nfds];
> +
> +#if ANDROID_VERSION == 19

change to >=19

@@ +114,4 @@
>    }
>  
>    sp<GraphicBuffer> buffer(new GraphicBuffer());
> +#if ANDROID_VERSION == 19

change to >=19

::: widget/gonk/libdisplay/FramebufferSurface.cpp
@@ +50,4 @@
>   * was adapted from the version in SurfaceFlinger
>   */
>  
> +#if ANDROID_VERSION == 19

change to >=19... and I saw the same things in other files.

@@ +51,5 @@
>   */
>  
> +#if ANDROID_VERSION == 19
> +FramebufferSurface::FramebufferSurface(int disp, uint32_t width, uint32_t height, uint32_t format, 
> +sp<BufferQueue>& bq) :

I think we still can call "ConsumerBase(new BufferQueue(true, alloc))," and then we could keep the original FramebufferSurface::FramebufferSurface

::: widget/gonk/libdisplay/FramebufferSurface.h
@@ +35,5 @@
>  class FramebufferSurface : public ConsumerBase {
>  public:
> +#if ANDROID_VERSION == 19
> +    FramebufferSurface(int disp, uint32_t width, uint32_t height, uint32_t format, sp<BufferQueue>& bq);
> +#else

Please try if we could just paste alloc to FramebufferSurace

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

Please try if we could just paste alloc to FramebufferSurace

Comment 16

[Sotaro Ikeda [:sotaro]]

(In reply to peter chang[:pchang] from comment #15)
> Comment on attachment 8350565 [details] [diff] [review]
> adapt-to-jb43-codebase-refine-1.diff
> 
> Review of attachment 8350565 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please check below comment and obsolete the old files which didn't need
> anymore.
> 
> ::: gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
> @@ +42,5 @@
> >  ParamTraits<MagicGrallocBufferHandle>::Write(Message* aMsg,
> >                                               const paramType& aParam)
> >  {
> > +#if ANDROID_VERSION >= 19
> > +  sp<GraphicBuffer> flattenable = aParam.mGraphicBuffer;
> 
> what error if we change to Flattenable *flattenable =
> aParam.mGraphicBuffer.get();?
> 

If we want to do this in 
Flattenable<GraphicBuffer> *flattenable = aParam.mGraphicBuffer.get();

But there is no necessity to cast to Flattenable in this function. It might be better to use just sp<GraphicBuffer> also for older android code.

Comment 17

[Sotaro Ikeda [:sotaro]]

(In reply to Solomon Chiu [:schiu] from comment #13)
> 
> Google made following change to the flatten funciton in KitKat. If I don't
> recover the corresponding variable, the system will crashed in
> PLayerTransactionChild::SendPGrallocBufferConstructor().
> 
> 
> status_t GraphicBuffer::flatten(void*& buffer, size_t& size, int*& fds,
> size_t& count) const {
>     :
>     buffer = reinterpret_cast<void*>(static_cast<int*>(buffer) + sizeNeeded);
>     size -= sizeNeeded;
>     fds += handle->numFds;
>     count -= handle->numFds;
> 
>     return NO_ERROR;
> }

So , actual value of getFlattenedSize() and getFdCount() are not changed. But size, fds and count is updated for data consumption. It is used for multiple Parcelable object consumption. For example, it is used in IGraphicBufferConsumer::BufferItem::flatten()
http://androidxref.com/4.4_r1/xref/frameworks/native/libs/gui/IGraphicBufferConsumer.cpp#93

Current comment is not clear enough for this point. It might be better to explain about it.

Comment 18

[Solomon Chiu [:schiu]]

Attachment: adapt-to-jb43-codebase-refine-2.diff

Comment 19

[Solomon Chiu [:schiu]]

Attachment: adapt-to-jb43-codebase-refine-2.diff

Comment 20

[Solomon Chiu [:schiu]]

Comment on attachment 8357021 [details] [diff] [review]
adapt-to-jb43-codebase-refine-2.diff

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

::: widget/gonk/libdisplay/FramebufferSurface.cpp
@@ +50,5 @@
>   * was adapted from the version in SurfaceFlinger
>   */
>  
> +#if ANDROID_VERSION >= 19
> +FramebufferSurface::FramebufferSurface(int disp, uint32_t width, uint32_t height, uint32_t format, sp<BufferQueue>& bq) :

According to KitKat's change, the BufferQueue is created in outside of FramebufferSurface, and be kept in Surface for connection, so we can not use anonymous object for BufferQueue.

@@ +65,5 @@
> +                                       GRALLOC_USAGE_HW_COMPOSER);
> +    mConsumer->setDefaultBufferFormat(format);
> +    mConsumer->setDefaultBufferSize(width,  height);
> +    mConsumer->setDefaultMaxBufferCount(NUM_FRAMEBUFFER_SURFACE_BUFFERS);
> +}

Due to the refactoring of ConsumerBase in Android, the data member mBufferQueue of ConsumerBase has been changed from 
"sp<BufferQueue> mBufferQueue" to "sp<IGraphicBufferConsumer> mConsumer", such that I need to use relative trivial way to handle the change.

Comment 21

[Michael Wu [:mwu]]

Comment on attachment 8357021 [details] [diff] [review]
adapt-to-jb43-codebase-refine-2.diff

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

::: widget/gonk/libdisplay/FramebufferSurface.cpp
@@ +65,5 @@
> +                                       GRALLOC_USAGE_HW_COMPOSER);
> +    mConsumer->setDefaultBufferFormat(format);
> +    mConsumer->setDefaultBufferSize(width,  height);
> +    mConsumer->setDefaultMaxBufferCount(NUM_FRAMEBUFFER_SURFACE_BUFFERS);
> +}

I think we can unify these constructors a lot more than now. The ConsumerBase constructor might be a little tricky, but the rest should be the same. You can also make a local copy of consumerbase based on what base version we're on so we don't need to special case the calls to mConsumer/mBufferQueue.

Comment 22

[Solomon Chiu [:schiu]]

Attachment: adapt-to-jb43-codebase-refine-3.diff

Comment 23

[Michael Wu [:mwu]]

Comment on attachment 8357683 [details] [diff] [review]
adapt-to-jb43-codebase-refine-3.diff

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

::: widget/gonk/libdisplay/GonkDisplayJB.cpp
@@ +125,5 @@
>      sp<SurfaceTextureClient> stc = new SurfaceTextureClient(static_cast<sp<ISurfaceTexture> >(mFBSurface->getBufferQueue()));
>  #else
> +
> +#if ANDROID_VERSION >= 19
> +    sp<Surface> stc = new Surface(static_cast<sp<IGraphicBufferProducer> >(bq));

We should always be able to use this line - mFBSurface->getBufferQueue() should always be unnecessary.

Comment 24

[Solomon Chiu [:schiu]]

Attachment: adapt-to-jb43-codebase-refine-4.diff

Comment 25

[Solomon Chiu [:schiu]]

Uploaded adapt-to-jb43-codebase-refine-4.diff according to Micheal's suggestion.

Comment 26

[Michael Wu [:mwu]]

Comment on attachment 8358186 [details] [diff] [review]
adapt-to-jb43-codebase-refine-4.diff

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

r=me for the widget/gonk/libdisplay/* changes. Thanks!

Comment 27

[Peter Chang[:pchang]]

Comment on attachment 8358186 [details] [diff] [review]
adapt-to-jb43-codebase-refine-4.diff

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

::: gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp
@@ +48,2 @@
>    Flattenable *flattenable = aParam.mGraphicBuffer.get();
> +#endif

getFlattenedSize()/getFdCount() from graphicbuffer.h were changed to public func in JB 4.4.
Therefore, it is fine to separate flattenable into two cases.

Comment 28

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8358186 [details] [diff] [review]
adapt-to-jb43-codebase-refine-4.diff

Looks good!

Comment 29

[Andreas Gal :gal]

This wants to be landed, too.

Comment 30

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/b2g-inbound/rev/4c2fe1a97191

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c2fe1a97191