985772 - Remove SurfaceDescriptor around GonkNativeWindow

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Sotaro Ikeda [:sotaro]]

SurfaceDescriptor should be used around IPC codes. It should not be used other places.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch - Remove SurfaceDescriptor around GonkNativeWindow

The patch works only on ICS. Confirmed the patch works some level on master hamachi.

Comment 2

[Sotaro Ikeda [:sotaro]]

On local playback, I saw some lags. It seem Bug 984576.

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch v2 - Remove SurfaceDescriptor around GonkNativeWindow

Add JB and KK change. On JB and KK, a crash happens around ImageBridge IPC.

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: patch v3 - Remove SurfaceDescriptor around GonkNativeWindow

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.

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: patch v4 - Remove SurfaceDescriptor around GonkNativeWindow

Fix nits. On ICS, genlock failure happens during video playback. Buffer seems to returned to OMXCodec too early.

Comment 6

[Sotaro Ikeda [:sotaro]]

Attachment: rollup patch

Fix the genlock problem on ICS. All probloems that I recognized are fixed.

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 1 - Change to GrallocTextureClientOGL

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 - ImageBridgeChild

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 - Change to GrallocImage

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 4 - change to GonkNativeWindowICS

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 5 change to GonkNativeWindowJB

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 6 - Change to GonkNativeWindowKK

Comment 13

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 7 - Change to camera

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 8 - Change to media

Comment 15

[Nicolas Silva [:nical]]

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

Comment 16

[Nicolas Silva [:nical]]

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 17

[Nicolas Silva [:nical]]

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.

Comment 18

[Nicolas Silva [:nical]]

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.

Comment 19

[Nicolas Silva [:nical]]

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.

Comment 20

[Mike Habicher [:mikeh] (high bugzilla latency)]

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.

Comment 21

[Sotaro Ikeda [:sotaro]]

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

Comment 22

[Sotaro Ikeda [:sotaro]]

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

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 2 v2 - ImageBridgeChild

Apply the comment. Carry "r=nical".

Comment 24

[Sotaro Ikeda [:sotaro]]

Attachment: patch part 3 v2 - Change to GrallocImage

Apply the comment.

Comment 25

[Sotaro Ikeda [:sotaro]]

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

Comment 26

[Sotaro Ikeda [:sotaro]]

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

Comment 27

[Nicolas Silva [:nical]]

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

[Peter Chang[:pchang]]

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"

Comment 29

[Peter Chang[:pchang]]

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

Comment 30

[Sotaro Ikeda [:sotaro]]

cajbir, review ping.

Comment 31

[Andreas Gal :gal]

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?

Comment 32

[Sotaro Ikeda [:sotaro]]

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

Comment 33

[Sotaro Ikeda [:sotaro]]

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

Comment 34

[Sotaro Ikeda [:sotaro]]

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

Comment 35

[Sotaro Ikeda [:sotaro]]

Attachment: rollup patch

Updated rollup patch.

Comment 36

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=2245d68fb21b

Comment 37

[Sotaro Ikeda [:sotaro]]

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

Comment 38

[Sotaro Ikeda [:sotaro]]

Attachment: rollup patch

Adds more error handling to GrallocTextureClientOGL::AllocateGralloc().

Comment 39

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=e2836efe3a9e

Comment 40

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=9e18ffbfbe9f

Comment 41

[Sotaro Ikeda [:sotaro]]

I recognize HAL_PIXEL_FORMAT_YV12 is not supported on emulator gralloc hal :-(

Comment 42

[Sotaro Ikeda [:sotaro]]

Attachment: rollup patch

Remove HAL_PIXEL_FORMAT_YV12 usage on emulator.

Comment 43

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=1e7b622db40b

Comment 44

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=f18001aaeeff

Comment 45

[Sotaro Ikeda [:sotaro]]

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

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d79f664ff595

Comment 50

[Sotaro Ikeda [:sotaro]]

Attachment: rollup patch for b2g-v1.4

A patch for b2g-v1.4. The patch also include Bug 986933 and Bug 1014360 fixes. Carry "review+".

Comment 51

[Sotaro Ikeda [:sotaro]]

[Blocking Requested - why for this release]:
Bug 1039937 is nominating to b2g v1.4. This bug blocks Bug 1039937.

Comment 52

[Hema Koka [:hema]]

Blocking a blocker for 1.4: 1039937 affecting camera performance, so +'d

Comment 53

[Sotaro Ikeda [:sotaro]]

I am going to update the patch for b2g-v1.4.

Comment 54

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8460896 [details] [diff] [review]
rollup patch for b2g-v1.4

Sorry for churn, update becomes not necessary. Enable it again.

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/9c0751774a7b

Comment 57

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #55)
> https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/9c0751774a7b

This seems to have broken some tests: 

https://tbpl.mozilla.org/php/getParsedLog.php?id=44568615&tree=Mozilla-B2g30-v1.4
https://tbpl.mozilla.org/php/getParsedLog.php?id=44568570&tree=Mozilla-B2g30-v1.4

Comment 58

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 59

[Milan Sreckovic [:milan] (needinfo for best results)]

I assume this means we're asking for gralloc and getting shmem or vice versa, but :nical will know better.

Comment 60

[Ryan VanderMeulen [:RyanVM]]

(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

Comment 61

[Nicolas Silva [:nical]]

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 62

[Nicolas Silva [:nical]]

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

Comment 63

[Jason.Liu]

Dear Rachelle，
  this issue seriously Block dolphin , please help to push to fix it asap.

Comment 64

[Wayne Chang [:wchang]]

Jason,

see comment 57 - comment 62.

Comment 65

[Wayne Chang [:wchang]]

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

Comment 66

[Nicolas Silva [:nical]]

Attachment: Don't crash if allocating a gralloc buffer failed in ImageBridge.

I haven't tested the patch yet but I'm putting it up for review asap to avoid delaying this uplift more than needed

Comment 67

[Nicolas Silva [:nical]]

try push https://tbpl.mozilla.org/?tree=Try&rev=5039ef135f18

Comment 68

[James Zhang (Spreadtrum)]

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.

Comment 69

[Nicolas Silva [:nical]]

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

Comment 70

[Nicolas Silva [:nical]]

Attachment: Don't crash if allocating a gralloc buffer failed in ImageBridge. (Carrying r=bjacob, upliftable with attachement 8460896)

The two patches must be uplifted together.

Comment 71

[Andreas Pehrson [:pehrsons]]

Great work nical!

Comment 72

[Nicolas Silva [: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

Comment 73

[Sotaro Ikeda [:sotaro]]

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