Closed Bug 985772 Opened 6 years ago Closed 6 years ago

Remove SurfaceDescriptor around GonkNativeWindow

Categories

(Core :: Graphics: Layers, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
blocking-b2g 1.4+
Tracking Status
firefox31 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 17 obsolete files)

143.27 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
181.99 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
4.74 KB, patch
bjacob
: review+
Details | Diff | Splinter Review
6.58 KB, patch
Details | Diff | Splinter Review
SurfaceDescriptor should be used around IPC codes. It should not be used other places.
Assignee: nobody → sotaro.ikeda.g
No longer blocks: BadSurfaceDescriptor
The patch works only on ICS. Confirmed the patch works some level on master hamachi.
On local playback, I saw some lags. It seem Bug 984576.
Add JB and KK change. On JB and KK, a crash happens around ImageBridge IPC.
Attachment #8393871 - Attachment is obsolete: true
Fixed problems on JB and KK. On KK, camera app's preview does not work. It is a b2g's problem. On current master, camera app's preview does not work. On nexus-5, I confirmed WebRTC's camera works.
Attachment #8394483 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Fix nits. On ICS, genlock failure happens during video playback. Buffer seems to returned to OMXCodec too early.
Attachment #8395294 - Attachment is obsolete: true
Attached patch rollup patch (obsolete) — Splinter Review
Fix the genlock problem on ICS. All probloems that I recognized are fixed.
Attachment #8395407 - Attachment is obsolete: true
Attachment #8395839 - Attachment description: patch v5 - Remove SurfaceDescriptor around GonkNativeWindow → rollup patch
Attached patch patch part 2 - ImageBridgeChild (obsolete) — Splinter Review
Attachment #8395865 - Attachment is patch: true
Attachment #8395865 - Attachment mime type: text/x-patch → text/plain
Attached patch patch part 7 - Change to camera (obsolete) — Splinter Review
Attached patch patch part 8 - Change to media (obsolete) — Splinter Review
Attachment #8395864 - Flags: review?(nical.bugzilla)
Attachment #8395865 - Flags: review?(nical.bugzilla)
Attachment #8395868 - Flags: review?(nical.bugzilla)
Attachment #8395870 - Flags: review?(nical.bugzilla)
Attachment #8395871 - Flags: review?(nical.bugzilla)
Attachment #8395872 - Flags: review?(nical.bugzilla)
Attachment #8395873 - Flags: review?(nical.bugzilla)
Attachment #8395875 - Flags: review?(nical.bugzilla)
Attachment #8395870 - Flags: review?(pchang)
Attachment #8395871 - Flags: review?(pchang)
Attachment #8395872 - Flags: review?(schiu)
Depends on: 986933
Attachment #8395872 - Flags: review?(schiu) → review+
Attachment #8395864 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8395865 [details] [diff] [review]
patch part 2 - ImageBridgeChild

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

::: gfx/layers/ipc/ImageBridgeChild.cpp
@@ +234,5 @@
>    GrallocParam(const IntSize& aSize,
> +                const uint32_t& aFormat,
> +                const uint32_t& aUsage,
> +                MaybeMagicGrallocBufferHandle* aHandle,
> +                PGrallocBufferChild** aChild)

nit: indendaton is off by 1
Attachment #8395865 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8395868 [details] [diff] [review]
patch part 3 - Change to GrallocImage

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

::: gfx/layers/GrallocImages.cpp
@@ +63,5 @@
>    NS_PRECONDITION(aData.mYSize.height % 2 == 0, "Image should have even height");
>    NS_PRECONDITION(aData.mYStride % 16 == 0, "Image should have stride of multiple of 16 pixels");
>  
> +  if (mTextureClient) {
> +    return;

So, if we call SetData on a GrallocImage that already has a TextureClient, we don't do anything. It looks like we never want this to happen. Shouldn't we copy the data into the TextureClient? if not shouldn't we assert?
I think we need either of the two or at least a comment explaining what's going on when we get into this state.
Comment on attachment 8395870 [details] [diff] [review]
patch part 4 - change to GonkNativeWindowICS

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

I don't know this part of the code very well so this wasn't a very thorough review. Looks good to me.
Attachment #8395870 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8395871 [details] [diff] [review]
patch part 5 change to GonkNativeWindowJB

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

::: widget/gonk/nativewindow/GonkBufferQueueJB.h
@@ +32,5 @@
>  #include <utils/Vector.h>
>  #include <utils/threads.h>
>  
>  #include "mozilla/layers/LayersSurfaces.h"
> +#include "mozilla/layers/TextureClient.h"

nit: you could probably avoid #including TextureClient.h here with some forward declarations and moving ctors/dtors to the .cpp. GonkBufferQueueJB.h is probably not included in a lot of places so I guess it is not a big deal.
Attachment #8395871 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8395872 [details] [diff] [review]
patch part 6 - Change to GonkNativeWindowKK

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

::: widget/gonk/nativewindow/GonkBufferQueueKK.cpp
@@ +60,5 @@
>          default: return "Unknown";
>      }
>  }
>  
> +class nsProxyReleaseTask : public CancelableTask

nit: You added this exact same struct to several places (which I suppose are conditionally compiled). I think It would be better to factor this out somewhere.
Attachment #8395872 - Flags: review?(nical.bugzilla) → review+
Attachment #8395873 - Flags: review?(nical.bugzilla) → review+
Attachment #8395875 - Flags: review?(nical.bugzilla) → review+
Attachment #8395873 - Flags: review?(mhabicher)
Attachment #8395875 - Flags: review?(cajbir.bugzilla)
Comment on attachment 8395873 [details] [diff] [review]
patch part 7 - Change to camera

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

Looks good to me. Built and tested with hamachi and nexus-4 to make sure.
Attachment #8395873 - Flags: review?(mhabicher) → review+
(In reply to Nicolas Silva [:nical] from comment #16)
> Comment on attachment 8395868 [details] [diff] [review]
> patch part 3 - Change to GrallocImage
> 
> Review of attachment 8395868 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/GrallocImages.cpp
> @@ +63,5 @@
> >    NS_PRECONDITION(aData.mYSize.height % 2 == 0, "Image should have even height");
> >    NS_PRECONDITION(aData.mYStride % 16 == 0, "Image should have stride of multiple of 16 pixels");
> >  
> > +  if (mTextureClient) {
> > +    return;
> 
> So, if we call SetData on a GrallocImage that already has a TextureClient,
> we don't do anything. It looks like we never want this to happen. Shouldn't
> we copy the data into the TextureClient?

Yeah, ASSERT is better. I am going to update a patch.
(In reply to Nicolas Silva [:nical] from comment #19)
> Comment on attachment 8395872 [details] [diff] [review]
> patch part 6 - Change to GonkNativeWindowKK
> 
> Review of attachment 8395872 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/nativewindow/GonkBufferQueueKK.cpp
> @@ +60,5 @@
> >          default: return "Unknown";
> >      }
> >  }
> >  
> > +class nsProxyReleaseTask : public CancelableTask
> 
> nit: You added this exact same struct to several places (which I suppose are
> conditionally compiled). I think It would be better to factor this out
> somewhere.

I am going to update the patch.
Apply the comment. Carry "r=nical".
Attachment #8395865 - Attachment is obsolete: true
Attachment #8397225 - Flags: review+
Attachment #8397225 - Attachment description: patch part 2 - ImageBridgeChild → patch part 2 v2 - ImageBridgeChild
Apply the comment.
Attachment #8395868 - Attachment is obsolete: true
Attachment #8395868 - Flags: review?(nical.bugzilla)
Attachment #8397234 - Flags: review?(nical.bugzilla)
Attachment #8397234 - Flags: review?(nical.bugzilla) → review+
(In reply to Nicolas Silva [:nical] from comment #18)
> Comment on attachment 8395871 [details] [diff] [review]
> patch part 5 change to GonkNativeWindowJB
> 
> Review of attachment 8395871 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/nativewindow/GonkBufferQueueJB.h
> @@ +32,5 @@
> >  #include <utils/Vector.h>
> >  #include <utils/threads.h>
> >  
> >  #include "mozilla/layers/LayersSurfaces.h"
> > +#include "mozilla/layers/TextureClient.h"
> 
> nit: you could probably avoid #including TextureClient.h here with some
> forward declarations and moving ctors/dtors to the .cpp. GonkBufferQueueJB.h
> is probably not included in a lot of places so I guess it is not a big deal.

I am going to do it as follow up. I tried a little bit and recognized that the change could affect more code than I thought at first.
(In reply to Nicolas Silva [:nical] from comment #19)
> Comment on attachment 8395872 [details] [diff] [review]
> patch part 6 - Change to GonkNativeWindowKK
> 
> Review of attachment 8395872 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/nativewindow/GonkBufferQueueKK.cpp
> @@ +60,5 @@
> >          default: return "Unknown";
> >      }
> >  }
> >  
> > +class nsProxyReleaseTask : public CancelableTask
> 
> nit: You added this exact same struct to several places (which I suppose are
> conditionally compiled). I think It would be better to factor this out
> somewhere.

Same as Comment 25. I am going to do the above as a follow up bug.
(In reply to Sotaro Ikeda [:sotaro] from comment #25)
> (In reply to Nicolas Silva [:nical] from comment #18)
> > Comment on attachment 8395871 [details] [diff] [review]
> > patch part 5 change to GonkNativeWindowJB
> > 
> > Review of attachment 8395871 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: widget/gonk/nativewindow/GonkBufferQueueJB.h
> > @@ +32,5 @@
> > >  #include <utils/Vector.h>
> > >  #include <utils/threads.h>
> > >  
> > >  #include "mozilla/layers/LayersSurfaces.h"
> > > +#include "mozilla/layers/TextureClient.h"
> > 
> > nit: you could probably avoid #including TextureClient.h here with some
> > forward declarations and moving ctors/dtors to the .cpp. GonkBufferQueueJB.h
> > is probably not included in a lot of places so I guess it is not a big deal.
> 
> I am going to do it as follow up. I tried a little bit and recognized that
> the change could affect more code than I thought at first.

I wouldn't worry about this. I think your time is better spent on the remaining 1.4 blockers than on fixing this review nit.
Comment on attachment 8395870 [details] [diff] [review]
patch part 4 - change to GonkNativeWindowICS

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

::: widget/gonk/nativewindow/GonkNativeWindowICS.cpp
@@ +36,5 @@
>  using namespace mozilla;
>  using namespace mozilla::gfx;
>  using namespace mozilla::layers;
>  
> +class nsProxyReleaseTask : public CancelableTask

Do you require CancelableTask? I think Task is enough for you because there is no implementation in Cancel()

@@ +90,5 @@
> +              mSlots[i].mTextureClient->ClearRecycleCallback();
> +              // release TextureClient in ImageBridge thread
> +              nsProxyReleaseTask* task = new nsProxyReleaseTask(mSlots[i].mTextureClient);
> +              mSlots[i].mTextureClient = NULL;
> +              ImageBridgeChild::GetSingleton()->GetMessageLoop()->PostTask(FROM_HERE, task);

These codes are duplicated in several places, how about create another function "ReleaseTextureClientInBackground"
Attachment #8395870 - Flags: review?(pchang) → review+
Comment on attachment 8395871 [details] [diff] [review]
patch part 5 change to GonkNativeWindowJB

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

::: widget/gonk/nativewindow/GonkBufferQueueJB.cpp
@@ +58,5 @@
>          default: return "Unknown";
>      }
>  }
>  
> +class nsProxyReleaseTask : public CancelableTask

Use Task is enough
Attachment #8395871 - Flags: review?(pchang) → review+
cajbir, review ping.
Flags: needinfo?(cajbir.bugzilla)
Comment on attachment 8395875 [details] [diff] [review]
patch part 8 - Change to media

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

This looks pretty straight forward so I will steal the review. One question to double check.

::: content/media/omx/OmxDecoder.cpp
@@ +809,5 @@
>      }
>  
> +    if (textureClient) {
> +      // Increment MediaBuffer's ref count
> +      mVideoBuffer->add_ref();

Where is this decremented?
Attachment #8395875 - Flags: review?(cajbir.bugzilla) → review+
Flags: needinfo?(cajbir.bugzilla)
(In reply to Andreas Gal :gal from comment #31)
> Comment on attachment 8395875 [details] [diff] [review]
> patch part 8 - Change to media
> 
> Review of attachment 8395875 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks pretty straight forward so I will steal the review. One question
> to double check.
> 
> ::: content/media/omx/OmxDecoder.cpp
> @@ +809,5 @@
> >      }
> >  
> > +    if (textureClient) {
> > +      // Increment MediaBuffer's ref count
> > +      mVideoBuffer->add_ref();
> 
> Where is this decremented?

I will add more comment here. This manually increment reference count for TextureClient. It keeps MediaBuffer in use during TextureClient is in use.
(In reply to peter chang[:pchang][:peter] from comment #29)
> Comment on attachment 8395871 [details] [diff] [review]
> patch part 5 change to GonkNativeWindowJB
> 
> Review of attachment 8395871 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/nativewindow/GonkBufferQueueJB.cpp
> @@ +58,5 @@
> >          default: return "Unknown";
> >      }
> >  }
> >  
> > +class nsProxyReleaseTask : public CancelableTask
> 
> Use Task is enough

Will update in a next patch.
(In reply to peter chang[:pchang][:peter] from comment #28)
> Comment on attachment 8395870 [details] [diff] [review]
> patch part 4 - change to GonkNativeWindowICS
> 
> Review of attachment 8395870 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/nativewindow/GonkNativeWindowICS.cpp
> @@ +36,5 @@
> >  using namespace mozilla;
> >  using namespace mozilla::gfx;
> >  using namespace mozilla::layers;
> >  
> > +class nsProxyReleaseTask : public CancelableTask
> 
> Do you require CancelableTask? I think Task is enough for you because there
> is no implementation in Cancel()
> 
> @@ +90,5 @@
> > +              mSlots[i].mTextureClient->ClearRecycleCallback();
> > +              // release TextureClient in ImageBridge thread
> > +              nsProxyReleaseTask* task = new nsProxyReleaseTask(mSlots[i].mTextureClient);
> > +              mSlots[i].mTextureClient = NULL;
> > +              ImageBridgeChild::GetSingleton()->GetMessageLoop()->PostTask(FROM_HERE, task);
> 
> These codes are duplicated in several places, how about create another
> function "ReleaseTextureClientInBackground"

I am going to update it in a followup bug.
Attached patch rollup patch (obsolete) — Splinter Review
Updated rollup patch.
Attachment #8395839 - Attachment is obsolete: true
(In reply to Sotaro Ikeda [:sotaro] from comment #36)
> https://tbpl.mozilla.org/?tree=Try&rev=2245d68fb21b

GrallocImage::SetData() seems to have a problem. Need to fix it.
Attached patch rollup patch (obsolete) — Splinter Review
Adds more error handling to GrallocTextureClientOGL::AllocateGralloc().
Attachment #8398924 - Attachment is obsolete: true
I recognize HAL_PIXEL_FORMAT_YV12 is not supported on emulator gralloc hal :-(
Attached patch rollup patchSplinter Review
Remove HAL_PIXEL_FORMAT_YV12 usage on emulator.
Attachment #8399244 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/d79f664ff595
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Duplicate of this bug: 941902
Duplicate of this bug: 951870
Duplicate of this bug: 941413
Depends on: 959089
No longer depends on: 959089
Depends on: 1014360
Blocks: 1042308
Blocks: 1039937
A patch for b2g-v1.4. The patch also include Bug 986933 and Bug 1014360 fixes. Carry "review+".
Attachment #8460896 - Flags: review+
Attachment #8399336 - Flags: review+
Attachment #8395864 - Attachment is obsolete: true
Attachment #8395870 - Attachment is obsolete: true
Attachment #8395871 - Attachment is obsolete: true
Attachment #8395872 - Attachment is obsolete: true
Attachment #8395873 - Attachment is obsolete: true
Attachment #8395875 - Attachment is obsolete: true
Attachment #8397225 - Attachment is obsolete: true
Attachment #8397234 - Attachment is obsolete: true
[Blocking Requested - why for this release]:
Bug 1039937 is nominating to b2g v1.4. This bug blocks Bug 1039937.
blocking-b2g: --- → 1.4?
Blocking a blocker for 1.4: 1039937 affecting camera performance, so +'d
blocking-b2g: 1.4? → 1.4+
Attachment #8460896 - Attachment is obsolete: true
I am going to update the patch for b2g-v1.4.
Comment on attachment 8460896 [details] [diff] [review]
rollup patch for b2g-v1.4

Sorry for churn, update becomes not necessary. Enable it again.
Attachment #8460896 - Attachment is obsolete: false
Duplicate of this bug: 1039937
Nical, can you take a look at these when you get in? Sotaro is away, and you reviewed this code, so you probably know most about it.
Flags: needinfo?(sotaro.ikeda.g) → needinfo?(nical.bugzilla)
I assume this means we're asking for gralloc and getting shmem or vice versa, but :nical will know better.
(In reply to Wes Kocher (:KWierso) from comment #57)
> This seems to have broken some tests: 
/tbpl.mozilla.org/php/getParsedLog.php?id=44568570&tree=Mozilla-B2g30-
> v1.4

Backed out.
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/802e5cf2834b
Flags: needinfo?(ryanvm)
There's a lot of code here so I'm not close to finding out what makes things work on central and not b2g_30, but this looks odd: https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/file/9c0751774a7b/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp#l302

We fail to initCheck, the returned MaybeGrallocBufferHandle stays null_t and we return the actor. On the other side, things seem to (wrongly) assume that if allocator->AllocGrallocBuffer returns an actor, then, the MaybeMagicGrallocBufferHandle will be a handle.
Comment on attachment 8460896 [details] [diff] [review]
rollup patch for b2g-v1.4

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

::: gfx/layers/ipc/ImageBridgeChild.cpp
@@ -745,5 @@
> -{
> -#ifdef MOZ_HAVE_SURFACEDESCRIPTORGRALLOC
> -  MaybeMagicGrallocBufferHandle handle;
> -  PGrallocBufferChild* gc = SendPGrallocBufferConstructor(aSize, aFormat, aUsage, &handle);
> -  if (handle.Tnull_t == handle.type()) {

This looks like the code that used to check for the handle type but doesn't exist anymore
Dear Rachelle,
  this issue seriously Block dolphin , please help to push to fix it asap.
Flags: needinfo?(ryang)
Jason,

see comment 57 - comment 62.
Flags: needinfo?(ryang)
:nical, thanks for following up. 
Would be great if you could see this through while Sotaro is away, feel free to let me know if anything blocks you.
I haven't tested the patch yet but I'm putting it up for review asap to avoid delaying this uplift more than needed
Attachment #8464102 - Flags: review?(bjacob)
Flags: needinfo?(nical.bugzilla)
Attachment #8464102 - Flags: review?(bjacob) → review+
Comment on attachment 8464102 [details] [diff] [review]
Don't crash if allocating a gralloc buffer failed in ImageBridge.

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

Please don't submit this patch.
It will cause build error.

../../../gecko/gfx/layers/ipc/ImageBridgeChild.cpp: In member function 'void mozilla::layers::ImageBridgeChild::AllocGrallocBufferNow(const IntSize&, uint32_t, uint32_t, mozilla::layers::PImageBridgeChild::MaybeMagicGrallocBufferHandle*, mozilla::layers::PImageBridgeChild::PGrallocBufferChild**)':
../../../gecko/gfx/layers/ipc/ImageBridgeChild.cpp:868:7: error: 'handle' was not declared in this scope
../../../gecko/gfx/layers/ipc/ImageBridgeChild.cpp: At global scope:
../../../gecko/gfx/layers/ipc/ImageBridgeChild.cpp:880:42: error: variable or field 'ProxyDeallocGrallocBufferNow' declared void
../../../gecko/gfx/layers/ipc/ImageBridgeChild.cpp:880:42: error: 'ISurfaceAllocator' was not declared in this scope
../../../gecko/gfx/layers/ipc/ImageBridgeChild.cpp:880:42: note: suggested alternative:
In file included from ../../dist/include/mozilla/layers/CompositableForwarder.h:14:0,
                 from ../../../gecko/gfx/layers/ipc/ImageBridgeChild.h:15,
                 from ../../../gecko/gfx/layers/ipc/ImageBridgeChild.cpp:6:
../../dist/include/mozilla/layers/ISurfaceAllocator.h:80:7: note:   'mozilla::layers::ISurfaceAllocator'
../../../gecko/gfx/layers/ipc/ImageBridgeChild.cpp:880:61: error: 'aAllocator' was not declared in this scope
../../../gecko/gfx/layers/ipc/ImageBridgeChild.cpp:881:42: error: 'PGrallocBufferChild' was not declared in this scope
../../../gecko/gfx/layers/ipc/ImageBridgeChild.cpp:881:42: note: suggested alternative:
In file included from ../../dist/include/mozilla/layers/ShadowLayerUtilsGralloc.h:15:0,
                 from ../../dist/include/gfxipc/ShadowLayerUtils.h:35,
                 from ../../ipc/ipdl/_ipdlheaders/mozilla/layers/LayersSurfaces.h:22,
                 from ../../ipc/ipdl/_ipdlheaders/mozilla/layers/LayersMessages.h:35,
                 from ../../dist/include/mozilla/layers/ISurfaceAllocator.h:17,
                 from ../../dist/include/mozilla/layers/CompositableForwarder.h:14,
                 from ../../../gecko/gfx/layers/ipc/ImageBridgeChild.h:15,
                 from ../../../gecko/gfx/layers/ipc/ImageBridgeChild.cpp:6:
../../ipc/ipdl/_ipdlheaders/mozilla/layers/PGrallocBufferChild.h:56:7: note:   'mozilla::layers::PGrallocBufferChild'
../../../gecko/gfx/layers/ipc/ImageBridgeChild.cpp:881:63: error: 'aChild' was not declared in this scope
../../../gecko/gfx/layers/ipc/ImageBridgeChild.cpp:882:58: error: expected primary-expression before '*' token
../../../gecko/gfx/layers/ipc/ImageBridgeChild.cpp:882:60: error: 'aBarrier' was not declared in this scope
../../../gecko/gfx/layers/ipc/ImageBridgeChild.cpp:883:42: error: expected primary-expression before 'bool'
../../../gecko/gfx/layers/ipc/ImageBridgeChild.cpp:1081:1: error: expected '}' at end of input

::: gfx/layers/ipc/ImageBridgeChild.cpp
@@ +864,5 @@
>    *aChild = SendPGrallocBufferConstructor(aSize,
>                                            aFormat,
>                                            aUsage,
>                                            aHandle);
> +  if (handle.type() == handle.Tnull_t) {

The 'hanle' will cause build error.
Yeah, I had a few over typos and mistakes, and made other try pushes since. Here is the last one: https://tbpl.mozilla.org/?tree=Try&rev=f665e69699b9
Great work nical!
I was a bit confused by the approval process for landing this but looking at Sotaro's first landing it seems that I just need to put "a=1.4+" in the commit message since the bug is tagged 1.4+, so that's what I did here. Sorry if I made a mistake. There is no string change and the risk isn't higher than Sotaro's initial patch.

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/99077a3d6b26
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/ffab5c83b5f5
(In reply to Nicolas Silva [:nical] from comment #72)
> I was a bit confused by the approval process for landing this but looking at
> Sotaro's first landing it seems that I just need to put "a=1.4+" in the
> commit message since the bug is tagged 1.4+, so that's what I did here.
> Sorry if I made a mistake. There is no string change and the risk isn't
> higher than Sotaro's initial patch.
> 
> https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/99077a3d6b26
> https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/ffab5c83b5f5

nical, thanks a lot!
You need to log in before you can comment on or make changes to this bug.