Closed Bug 1000640 Opened 6 years ago Closed 5 years ago

Support fast WebGL compositing for new-layers D3D11 OMTC

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: jgilbert, Assigned: jgilbert)

References

(Depends on 1 open bug)

Details

(Whiteboard: [games:p?])

Attachments

(8 files, 13 obsolete files)

17.81 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
8.04 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
15.75 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
63.54 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
2.84 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
2.75 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
6.01 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
106.04 KB, patch
jgilbert
: review+
Details | Diff | Splinter Review
I named this somewhat conservatively, since I'm not entirely sure about terminology yet.

WebGL still needs SurfaceStream to function properly, so we need to keep that around somehow. (for now)

It seems to me that what we need is a D3D equivalent of StreamTexture{Client,Host}OGL.

In my first run at this, I'm looking at s/StreamTexture{Client,Host}OGL/StreamTexture{Client,Host}/, since a TextureClient/Host for SurfaceStream doesn't really need to be restricted to OGL. Where I deviate from the existing code is in ::Lock(). Previously, there was a StreamTextureSourceOGL. It seems to me that what we can do here is do the ShSurf->TextureSource conversion here in Lock().
Attached patch WIP (obsolete) — Splinter Review
I think you can see where I got stuck here. Is this a reasonable path to take, though? Is there a reason to keep around a StreamTextureSource of some type?
Attachment #8411482 - Flags: feedback?(nical.bugzilla)
Feel free to jump in with feedback anyone. I think this bug will mostly be me learning about the new layers classes, so I'm sure I'll bump into some walls while feeling my way around.
(In reply to Jeff Gilbert [:jgilbert] from comment #1)
> Created attachment 8411482 [details] [diff] [review]
> WIP
> 
> I think you can see where I got stuck here. Is this a reasonable path to
> take, though? Is there a reason to keep around a StreamTextureSource of some
> type?

I agree that we should get rid of StreamTextureSource and just have TextureSources for the underlying object types.

Basic can always be handled by Compositor::CreateDataTextureSource.

ANGLE shared surfaces on d3d11 can be handled using DataTextureSourceD3D11.

OSX might need some work to make sure we have appropriate classes for everything, but we have MacIOSurfaceTextureSourceOGL already.

This will result in some ugly #ifdef PLATFORM code in StreamTextureHost, or alternatively some CreateTextureSourceForSharedSurface{OGL|D3D11} functions that can hide most of it. Seems like the best use of the TextureSource abstraction though.
In the ideal world, we would not need a stream specific TextureClient/Host pair, and SurfaceStream would manipulate the adequate TextureClient/Host type (or MozSurface when it becomes a real thing). I don't mind having an intermediate step where we still have a stream-specific Texture type if that helps getting proper WebGL on Windows OMTC sooner. I am interested in what the missing pieces are for TextureClient to be used in place of StreamTextureClient, though [1].

I agree with Matt about reusing the existing TextureSource implems whenever possible.

I need to spend some more time looking at your code but so far I don't see anything that rings an alarm bell.

[1] - an answer to my own question might be the work that needs to be done to merge SharedSurface with TextureClient, let me know if I am missing anything else.
Attached patch stream-tex-host (obsolete) — Splinter Review
So the body of it should look something like this. I'm not entirely sure where to hook this up, though. How do I make this host get used?
Attachment #8411482 - Attachment is obsolete: true
Attachment #8411482 - Flags: feedback?(nical.bugzilla)
Attachment #8416727 - Flags: feedback?(matt.woodrow)
Comment on attachment 8416727 [details] [diff] [review]
stream-tex-host

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

Matches what I was expecting/hoping for :)

::: gfx/layers/composite/TextureHost.cpp
@@ +726,5 @@
> +
> +        nsRefPtr<ID3D11Texture2D> tex;
> +        HRESULT hr = d3d->OpenSharedResource(shareHandle,
> +                                             __uuidof(ID3D11Texture2D),
> +                                             getter_AddRefs(tex));

Bas pointed out on irc that OpenSharedResource can be somewhat expensive. It might be nice if we could cache the result of this somehow instead of having repeat it each time we present the same buffer.

Do we have any performance tests for WebGL? Would be nice to know if it actually matters before writing more code.

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +81,1 @@
>        break;

Move this whole switch case up into the caller, here http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/TextureHost.cpp#149

That way it'll get created for all compositor backends, not just OGL.
Attachment #8416727 - Flags: feedback?(matt.woodrow) → feedback+
I just rebased Jeff's patch and made it use the ANGLE code paths with D3D11 OMTC (the previous version was using SurfaceFactory_Basic which is most certainly not going to be good enough).

It works well locally. I was discovering things along the way so I am not sure what's left to do for it to be shippable.
try push https://tbpl.mozilla.org/?tree=Try&rev=e723f6898e7b

Sorry for stealing this bug, we'd like to enable OMTC on windows asap so it'd be nice to not have a 800% webgl performance regression :)
try push with the build even more fixed: https://tbpl.mozilla.org/?tree=Try&rev=f1b5f5ba3048
Windows green:
https://tbpl.mozilla.org/?tree=Try&rev=e98d8af72dc0

Linux green, Mac reftest errors (red and blue channels are flipped), android green:
https://tbpl.mozilla.org/?tree=Try&rev=4ffb5702c13d

B2G still busted:
https://tbpl.mozilla.org/?tree=Try&rev=8f5739c671b9
Is there a try build where I can test this out?
Whiteboard: [games:p?]
blocking-b2g: --- → 1.4?
blocking-b2g: 1.4? → ---
(In reply to Gary [:streetwolf] from comment #13)
> Is there a try build where I can test this out?

If you're on windows, try my 'windows green' push.
(In reply to Jeff Gilbert [:jgilbert] from comment #14)
> (In reply to Gary [:streetwolf] from comment #13)
> > Is there a try build where I can test this out?
> 
> If you're on windows, try my 'windows green' push.

Gave it a whirl and my fps went from about 17 to 60 where it should be with OMTC enabled.  Is this patch ready to go to inbound soon?
Attached patch latest version of the patch (obsolete) — Splinter Review
Attachment #8423127 - Attachment is obsolete: true
Jeff, I'll let you finish this while I go back to investigating OMTC talos regressions on Windows.
Flags: needinfo?(jgilbert)
(In reply to Nicolas Silva [:nical] from comment #18)
> Jeff, I'll let you finish this while I go back to investigating OMTC talos
> regressions on Windows.

Sounds good. I have been neglecting to update this bug as this patch keeps bouncing off Try.
Flags: needinfo?(jgilbert)
Don't you think that this warrants a priority, 1 or 2?  Personally I've run into sites that are way slow because of this bug.
(In reply to Gary [:streetwolf] from comment #20)
> Don't you think that this warrants a priority, 1 or 2?  Personally I've run
> into sites that are way slow because of this bug.

Setting a higher priority can't fix the bugs I'm finding faster. It's already at the top of my queue, but this is more work than you would think.
As long as this doesn't explode, we should just mark failing tests as failing for now, and address them in a follow-up. (should just be the color-alpha test on win XP, which I cannot reproduce locally)
Attachment #8416727 - Attachment is obsolete: true
Attachment #8427073 - Attachment is obsolete: true
Attachment #8432772 - Flags: review?(matt.woodrow)
Attachment #8432773 - Flags: review?(matt.woodrow)
Comment on attachment 8432772 [details] [diff] [review]
patch 1: Handle alpha-non-premult

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

::: gfx/thebes/gfxUtils.cpp
@@ +287,5 @@
> +    RefPtr<DataSourceSurface> destSurf;
> +    DataSourceSurface::MappedSurface srcMap;
> +    DataSourceSurface::MappedSurface destMap;
> +    if (!MapSrcAndCreateMappedDest(srcSurf, &destSurf, &srcMap, &destMap)) {
> +        MOZ_ASSERT(false, "MapSrcAndCreateMappedDest failed.");

I don't think we should crash here, Map can fail for legitimate reasons (direct3d device reset?).
Attachment #8432772 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8432773 [details] [diff] [review]
patch 2: Add a client/host for SurfaceStream.

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

I'd really prefer if you could split this up into a queue of patches (that compiles and works at each stage) before landing. We tend to do a lot of regression ranges and history reading in Layers, and having small targeted changeset makes this infinitely easier. For example the SharedSurfaceANGLE+CanvasLayerD3D10 changes could be split out, as could the BasicCompositor ones.

Don't bother sinking too much time into it though, a best effort first pass is plenty :)

::: gfx/layers/CompositorTypes.h
@@ +87,5 @@
>    // We've previously tried a texture and it didn't work for some reason. If there
>    // is a fallback available, try that.
>    ALLOC_FALLBACK     = 1 << 17,
> +  // Data in this texture has not been alpha-premultiplied.
> +  NON_PREMULT        = 1 << 18,

Call this NON_PREMULTIPLIED. All the other values appear to be going for un-abbreviated names.

::: gfx/layers/Effects.h
@@ +237,5 @@
>                 aSourceOnWhite->GetFormat() == gfx::SurfaceFormat::B8G8R8X8);
>      return new EffectComponentAlpha(aSource, aSourceOnWhite, aFilter);
>    }
>  
> +  return CreateTexturedEffect(aSource->GetFormat(), aSource, aFilter, true);

'isAlphaPremultiplied', not 'true', right?

::: gfx/layers/Layers.h
@@ +1254,5 @@
>     * Returns the blendmode of this layer.
>     */
>    gfx::CompositionOp GetEffectiveMixBlendMode();
>    gfxContext::GraphicsOperator DeprecatedGetEffectiveMixBlendMode();
> +

Please split unrelated whitespace into a separate patch for landing.

::: gfx/layers/basic/BasicCompositor.cpp
@@ +326,5 @@
> +                                       texturedEffect->mFilter,
> +                                       aOpacity, sourceMask, &maskTransform);
> +      } else {
> +          RefPtr<DataSourceSurface> srcData = source->GetSurface(dest)->GetDataSurface();
> +          RefPtr<DataSourceSurface> premultData = gfxUtils::CreatePremultipliedDataSurface(srcData);

Maybe add a comment about how we would potentially repeat this work if we composite the same unpremultiplied texture multiple times. Would benefit from a cache, but I doubt it's worth the work right now.

::: gfx/layers/client/CanvasClient.cpp
@@ +200,5 @@
>        mBuffer = nullptr;
>      }
>  
>      if (mBuffer) {
> +	  GetForwarder()->UpdatedTexture(this, mBuffer, nullptr);

Incorrect indent

::: gfx/layers/client/ClientCanvasLayer.cpp
@@ +58,5 @@
>  
>    mCanvasClient = nullptr;
>  
>    if (mGLContext) {
> +    mIsAlphaPremultiplied = aData.mIsGLAlphaPremult;

CopyableCanvasLayer (which this inherits from) already has mIsGLAlphaPremult as a protected member, and is initialized in CopyableCanvasLayer::Initialize.

::: gfx/layers/client/TextureClient.h
@@ +519,5 @@
> +  {
> +    return gfx::SurfaceFormat::UNKNOWN;
> +  }
> +
> +  virtual bool AllocateForSurface(gfx::IntSize aSize, TextureAllocationFlags aFlags) MOZ_OVERRIDE

Either don't override this (the default is identical) or MOZ_CRASH to make sure we never hit it.

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +141,5 @@
> +    case SurfaceDescriptor::TSurfaceStreamDescriptor: {
> +      const SurfaceStreamDescriptor& desc = aDesc.get_SurfaceStreamDescriptor();
> +      result = new StreamTextureHost(aFlags, desc);
> +      break;
> +    }

I think this is dead code, we don't call into CreateTextureHostD3D11 for TSurfaceStreamDescriptor. We handle it (correctly) in the TextureHost.cpp version.
Attachment #8432773 - Flags: review?(matt.woodrow) → review+
I borrowed a WinXP machine from Marc, and could not reproduce the try failure in non-reftest form there. I will try running it as a reftest, in case that makes a difference, but in the mean time, I'm going to mark it failing on XP.
Attachment #8432773 - Attachment is obsolete: true
Attachment #8433783 - Flags: review?(matt.woodrow)
Attachment #8433785 - Flags: review?(matt.woodrow)
Attached patch patch 5: Mark XP as failing (obsolete) — Splinter Review
Attachment #8433786 - Flags: review?(matt.woodrow)
Attachment #8433783 - Flags: review?(matt.woodrow) → review+
Attachment #8433784 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8433785 [details] [diff] [review]
patch 4: New StreamTextureClient/Host

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

::: gfx/layers/client/ClientCanvasLayer.cpp
@@ +58,5 @@
>  
>    mCanvasClient = nullptr;
>  
>    if (mGLContext) {
> +    mIsAlphaPremultiplied = aData.mIsGLAlphaPremult;

This is duplicating a variable and code from CopyableCanvasLayer.

@@ +173,5 @@
>        // and doesn't require layers to do any deallocation.
>        flags |= TextureFlags::DEALLOCATE_CLIENT;
>      }
> +
> +    if (!mIsAlphaPremultiplied) {

Just use mIsGLAlphaPremul here.
Attachment #8433785 - Flags: review?(matt.woodrow) → review+
Attachment #8433786 - Flags: review?(matt.woodrow) → review+
Attached patch patch 5: Mark XP as failing (obsolete) — Splinter Review
s/fail-if/fails-if/
r=mattwoodrow carried
Attachment #8433786 - Attachment is obsolete: true
Attachment #8434392 - Flags: review+
Addressed review comments.
r=mattwoodrow carried forward.
Attachment #8433785 - Attachment is obsolete: true
Attachment #8434425 - Flags: review+
Add a comment that WinXP might work if OMTC is disabled, and that it only fails on Try, but WFM otherwise.
r=mattwoodrow still
Attachment #8434392 - Attachment is obsolete: true
Attachment #8434427 - Flags: review+
Depends on: 1020663
I hate this. It looks like my last-minute changes regressed this, since Ru was working before, and now C1.0/A0.5 works but C0.5/A0.5 appears to fail on Ru.
Attachment #8434554 - Flags: review?(matt.woodrow)
Attachment #8434554 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8433784 [details] [diff] [review]
patch 3: Update Gralloc and ANGLE ShSurf backends

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

::: gfx/gl/SharedSurfaceGralloc.h
@@ -55,5 @@
> -                           prodGL,
> -                           size,
> -                           hasAlpha)
> -        , mEGL(egl)
> -        , mSync(0)

I assume you meant to keep this initialization when you moved the ctor to the cpp file.
(In reply to Ed Morley [:edmorley UTC+0] from comment #45)
> Backed out for compilation failures on B2G:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=41528515&tree=Mozilla-
> Inbound

Looks like it conflicted with the following changeset: http://hg.mozilla.org/mozilla-central/rev/229dc47b5059

Apparently you'll need to either move back to using GrallocTextureClient/Host with SurfaceStream on B2G, or implement the fence handling code that Sotaro added recently.

I suspect that the first solution, while being ugly and a bit backward, will be less risky to uplift.
Comment on attachment 8438684 [details] [diff] [review]
patch 7: Remove failing assert for now

This isn't necessary, it is fixed by the change I suggested earlier.

What nical was talking about needs to be fixed though.
Attachment #8438684 - Flags: review?(matt.woodrow)
(In reply to Matt Woodrow (:mattwoodrow) from comment #48)
> Comment on attachment 8438684 [details] [diff] [review]
> patch 7: Remove failing assert for now
> 
> This isn't necessary, it is fixed by the change I suggested earlier.
> 
> What nical was talking about needs to be fixed though.

Oh, cool. I didn't understand what you meant.
(In reply to Nicolas Silva [:nical] from comment #46)
> (In reply to Ed Morley [:edmorley UTC+0] from comment #45)
> > Backed out for compilation failures on B2G:
> > https://tbpl.mozilla.org/php/getParsedLog.php?id=41528515&tree=Mozilla-
> > Inbound
> 
> Looks like it conflicted with the following changeset:
> http://hg.mozilla.org/mozilla-central/rev/229dc47b5059
> 
> Apparently you'll need to either move back to using
> GrallocTextureClient/Host with SurfaceStream on B2G, or implement the fence
> handling code that Sotaro added recently.
> 
> I suspect that the first solution, while being ugly and a bit backward, will
> be less risky to uplift.

Ugh, just my luck.
Attached patch patch 7: a-terrible-hack (obsolete) — Splinter Review
This compiles, but I think it's clear I shouldn't do this.

Sotaro: How should this function create a new Gralloc Host?
Attachment #8438684 - Attachment is obsolete: true
Attachment #8438781 - Flags: feedback?(sotaro.ikeda.g)
(In reply to Jeff Gilbert [:jgilbert] from comment #51)
> Created attachment 8438781 [details] [diff] [review]
> patch 7: a-terrible-hack
> 
> This compiles, but I think it's clear I shouldn't do this.
> 
> Sotaro: How should this function create a new Gralloc Host?

This code can only get hit if we're running WebGL in the root process, on b2g, right?

I think we could just not support that efficiently, and use readback.
(In reply to Jeff Gilbert [:jgilbert] from comment #51)
> Created attachment 8438781 [details] [diff] [review]
> patch 7: a-terrible-hack
> 
> This compiles, but I think it's clear I shouldn't do this.
> 
> Sotaro: How should this function create a new Gralloc Host?

It seems better not to create new Gralloc Host. TexuteHost's implementation is very IPC specific.

GrallocTextureSourceOGL refer to GrallocTextureHostOGL to call the following.

> mTextureHost->WaitAcquireFenceSyncComplete();

By delivering the fence from GrallocTextureHostOGL to GrallocTextureSourceOGL, the dependency could be removed. On StreamTextureHost case, new acquire fence should not be created during composition. Therefore, it can be set to GrallocTextureSourceOGL around GrallocTextureSourceOGL creation time.

On other use cases, Acquire fence can be delivered to GrallocTextureSourceOGL by overriding TextureHostOGL::SetAcquireFence() by GrallocTextureHostOGL.
(In reply to Jeff Gilbert [:jgilbert] from comment #51)
> Created attachment 8438781 [details] [diff] [review]
> patch 7: a-terrible-hack
> 
> This compiles, but I think it's clear I shouldn't do this.
> 
> Sotaro: How should this function create a new Gralloc Host?

Sorry, Comment 53 was a wrong comment :-( The patch should work except acquire fence delivery of StreamTextureHost use case.
It is not a good idea to create TextureHost. It is same to Comment 53.
Therefore it seems better to deliver fence to GrallocTextureSourceOGL in StreamTextureHost case.
Sorry, my comment was wrong again :-( I forgot to think and mention about Fence delivery to HwcComposer. It is not implemented yet in gecko. But it is soon checked-in by Bug 1024144.

HwComposer gets fence via TextureHostOGL. Therefore GrallocTextureHostOGL is derived from TextureHostOGL on gonk. StreamTextureHost also need to deliver Fence to HwComposer. StreamTextureHost is also need to be derived from TextureHostOGL on gonk.

so, the following seems necessary to work with both HwComposer and OpneGL compositions.
- Addd TextureHostOGL as StreamTextureHost's ancestor on gonk. 
- Change GrallocTextureSourceOGL as to reference to TextureHostOGL(instead of GrallocTextureHostOGL).
- Set StreamTextureHost pointer to GrallocTextureSourceOGL as TextureHostOGL pointer.
- StreamTextureHost stores acquire Fence on gonk.
   It is done by TextureHostOGL's implementation.
Attachment #8438781 - Flags: feedback?(sotaro.ikeda.g)
(In reply to Sotaro Ikeda [:sotaro] from comment #57)
> Sorry, my comment was wrong again :-( I forgot to think and mention about
> Fence delivery to HwcComposer. It is not implemented yet in gecko. But it is
> soon checked-in by Bug 1024144.
> 
> HwComposer gets fence via TextureHostOGL. Therefore GrallocTextureHostOGL is
> derived from TextureHostOGL on gonk. StreamTextureHost also need to deliver
> Fence to HwComposer. StreamTextureHost is also need to be derived from
> TextureHostOGL on gonk.
> 
> so, the following seems necessary to work with both HwComposer and OpneGL
> compositions.
> - Addd TextureHostOGL as StreamTextureHost's ancestor on gonk. 
> - Change GrallocTextureSourceOGL as to reference to TextureHostOGL(instead
> of GrallocTextureHostOGL).
> - Set StreamTextureHost pointer to GrallocTextureSourceOGL as TextureHostOGL
> pointer.
> - StreamTextureHost stores acquire Fence on gonk.
>    It is done by TextureHostOGL's implementation.

I might be wrong, but IIRC, when SurfaceStream is used accross ipc, the client side creates a GrallocTextureClient rather than a StreamTextureClient, which means that on the host side we get a GrallocTextureHost with all of the Fence plumbing donce. Why not just always do that, even when the client and the host are on the same process, since the latter case is probably never happening (system app doing WebGL?).
If the cross-process path works and the cross-thread doesn't, I don't see much value in making the cross-thread code different.
SkiaGL backed Canvas 2d also seems to use SurfaceStream.
Attachment #8438781 - Attachment is obsolete: true
Attachment #8440118 - Flags: review?(matt.woodrow)
Comment on attachment 8440118 [details] [diff] [review]
patch 4.5: Crash if we take the Gralloc path in TextureHost.

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

Let's just do what nical suggested, and always take the cross-process path for gralloc, even if it's cross thread. That's better than having this block of extra code that will never be tested.
Attachment #8440118 - Flags: review?(matt.woodrow) → review-
Attachment #8440118 - Attachment is obsolete: true
Attachment #8441709 - Flags: review?(matt.woodrow)
Comment on attachment 8441709 [details] [diff] [review]
patch 4.5: Don't use SurfStream stuff on Gonk.

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

::: gfx/layers/client/CanvasClient.cpp
@@ +36,5 @@
>                                   CompositableForwarder* aForwarder,
>                                   TextureFlags aFlags)
>  {
> +#ifdef MOZ_WIDGET_GONK
> +  return new CanvasClient2D(aForwarder, aFlags);

Using this is going to trigger readback for every frame, we don't want to do that.

This is the branch you want to made unconditionally true for gonk, http://mxr.mozilla.org/mozilla-central/source/gfx/layers/client/CanvasClient.cpp#161

::: gfx/layers/composite/TextureHost.cpp
@@ +948,5 @@
>          break;
>        }
>  #ifdef MOZ_WIDGET_GONK
>        case gfx::SharedSurfaceType::Gralloc: {
> +        MOZ_CRASH("WebGL in the Host process? Gralloc without E10S? Not yet supported.");

Can we just get rid of this entire switch case? No point keeping code around that has never been tested and won't ever be used.
Attachment #8441709 - Attachment is obsolete: true
Attachment #8441709 - Flags: review?(matt.woodrow)
Attachment #8441804 - Flags: review?(matt.woodrow)
Comment on attachment 8441804 [details] [diff] [review]
patch 4.5: Don't use SurfStream stuff on Gonk.

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

::: gfx/layers/client/CanvasClient.cpp
@@ +157,5 @@
>  
>    bool isCrossProcess = !(XRE_GetProcessType() == GeckoProcessType_Default);
> +
> +#ifdef MOZ_WIDGET_GONK
> +  MOZ_ALWAYS_TRUE(isCrossProcess);

This isn't guaranteed, we can't assert it.
Attachment #8441804 - Flags: review?(matt.woodrow) → review+
(In reply to Matt Woodrow (:mattwoodrow) from comment #66)
> Comment on attachment 8441804 [details] [diff] [review]
> patch 4.5: Don't use SurfStream stuff on Gonk.
> 
> Review of attachment 8441804 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/client/CanvasClient.cpp
> @@ +157,5 @@
> >  
> >    bool isCrossProcess = !(XRE_GetProcessType() == GeckoProcessType_Default);
> > +
> > +#ifdef MOZ_WIDGET_GONK
> > +  MOZ_ALWAYS_TRUE(isCrossProcess);
> 
> This isn't guaranteed, we can't assert it.

Huh, well it passes Try. I can remove it though.
r=mattwoodrow
Attachment #8441804 - Attachment is obsolete: true
Attachment #8442351 - Flags: review+
Blocks: 1027147
(In reply to Nicolas Silva [:nical] from comment #46)
> Apparently you'll need to either move back to using
> GrallocTextureClient/Host with SurfaceStream on B2G, or implement the fence
> handling code that Sotaro added recently.
> 
> I suspect that the first solution, while being ugly and a bit backward, will
> be less risky to uplift.

Is there a bug yet to fix this properly, i.e. with fence handling?
See Also: → 950079
[Approval Request Comment]
Bug caused by (feature/regressing bug #): Enable OMTC on Windows.
User impact if declined: Slow WebGL on Windows if we don't disable OMTC.
Testing completed (on m-c, etc.): Local, Try, M-C.
Risk to taking this patch (and alternatives if risky): Medium-low, but it's almost equally risky *not* to take this patch, since we don't have huge confidence in the default readback path OMTC uses without this patch.
String or IDL/UUID changes made by this patch: None.
Attachment #8445552 - Flags: review+
Attachment #8445552 - Flags: approval-mozilla-aurora?
Comment on attachment 8445552 [details] [diff] [review]
rolled-up patch (aurora)

We're disabling OMTC on Aurora/32, no need to uplift this.
Attachment #8445552 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 986479
Duplicate of this bug: 960219
Duplicate of this bug: 1027147
Duplicate of this bug: 1013039
You need to log in before you can comment on or make changes to this bug.