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

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

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

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

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

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 <wkocher@mozilla.com> 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 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()

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

> @@ +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.

(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; }

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

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

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

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

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 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 on attachment 8358186 [details] [diff] [review] adapt-to-jb43-codebase-refine-4.diff Looks good!

This wants to be landed, too.