1000640 - Support fast WebGL compositing for new-layers D3D11 OMTC

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Kelsey Gilbert [:jgilbert]]

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

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: 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?

Comment 2

[Kelsey Gilbert [:jgilbert]]

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.

Comment 3

[Matt Woodrow (:mattwoodrow)]

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

Comment 4

[Nicolas Silva [:nical]]

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.

Comment 5

[Kelsey Gilbert [:jgilbert]]

Attachment: stream-tex-host

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?

Comment 6

[Matt Woodrow (:mattwoodrow)]

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.

Comment 7

[Nicolas Silva [:nical]]

Attachment: Rebased + enabled ANGLE code paths with D3D11

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.

Comment 8

[Nicolas Silva [:nical]]

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

Comment 9

[Nicolas Silva [:nical]]

try push with build fixed: https://tbpl.mozilla.org/?tree=Try&rev=8d8217e99669

Comment 10

[Nicolas Silva [:nical]]

try push with the build even more fixed: https://tbpl.mozilla.org/?tree=Try&rev=f1b5f5ba3048

Comment 11

[Kelsey Gilbert [:jgilbert]]

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

Comment 12

[Nicolas Silva [:nical]]

new try push with some fixes: https://tbpl.mozilla.org/?tree=Try&rev=26047e2e4100

Comment 13

[Gary [:streetwolf52]]

Is there a try build where I can test this out?

Comment 14

[Kelsey Gilbert [:jgilbert]]

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

Comment 15

[Gary [:streetwolf52]]

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

Comment 16

[Nicolas Silva [:nical]]

new try push https://tbpl.mozilla.org/?tree=Try&rev=fd6afcfb33da

Comment 17

[Nicolas Silva [:nical]]

Attachment: latest version of the patch

Comment 18

[Nicolas Silva [:nical]]

Jeff, I'll let you finish this while I go back to investigating OMTC talos regressions on Windows.

Comment 19

[Kelsey Gilbert [: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.

Comment 20

[Gary [:streetwolf52]]

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.

Comment 21

[Kelsey Gilbert [:jgilbert]]

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

Comment 22

[Kelsey Gilbert [:jgilbert]]

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)

Comment 23

[Kelsey Gilbert [:jgilbert]]

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

Comment 24

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 1: Handle alpha-non-premult

Comment 25

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 2: Add a client/host for SurfaceStream.

Comment 26

[Matt Woodrow (:mattwoodrow)]

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

Comment 27

[Matt Woodrow (:mattwoodrow)]

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.

Comment 28

[Kelsey Gilbert [:jgilbert]]

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.

Comment 29

[Kelsey Gilbert [:jgilbert]]

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

Comment 30

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 2: I hate whitespace.

Comment 31

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 3: Update Gralloc and ANGLE ShSurf backends

Comment 32

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 4: New StreamTextureClient/Host

Comment 33

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 5: Mark XP as failing

Comment 34

[Matt Woodrow (:mattwoodrow)]

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.

Comment 35

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 5: Mark XP as failing

s/fail-if/fails-if/
r=mattwoodrow carried

Comment 36

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 4: New StreamTextureClient/Host

Addressed review comments.
r=mattwoodrow carried forward.

Comment 37

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 5: Mark XP as failing

Add a comment that WinXP might work if OMTC is disabled, and that it only fails on Try, but WFM otherwise.
r=mattwoodrow still

Comment 38

[Kelsey Gilbert [:jgilbert]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/f644eda342c2
 https://hg.mozilla.org/integration/mozilla-inbound/rev/9d1dd5a9be4f
 https://hg.mozilla.org/integration/mozilla-inbound/rev/cb1ea8162165
 https://hg.mozilla.org/integration/mozilla-inbound/rev/3a764e34c366
 https://hg.mozilla.org/integration/mozilla-inbound/rev/fa44003a4040

Comment 39

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 6: Mark Color1.0/Alpha0.5 test as random, since it works on Win7-Ru and not on WinXP-R

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.

Comment 40

[Kelsey Gilbert [:jgilbert]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/5be1a688e645

Comment 41

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

Various gl-related failures after this landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/08cf77174532
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6ef90e16969
https://hg.mozilla.org/integration/mozilla-inbound/rev/abf745ddb9fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4f4d4353263
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb48bc62d23
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad28fb0d24e6

Comment 42

[Matt Woodrow (:mattwoodrow)]

Failures looked like this:

https://tbpl.mozilla.org/php/getParsedLog.php?id=41073154&tree=Mozilla-Inbound&full=1#error1
https://tbpl.mozilla.org/php/getParsedLog.php?id=41073543&tree=Mozilla-Inbound&full=1#error1

Comment 43

[Matt Woodrow (:mattwoodrow)]

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.

Comment 44

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bfb5297101cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/11bb536cdede
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1b96309bb9d
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4681bf6680c
https://hg.mozilla.org/integration/mozilla-inbound/rev/5458e23a7f0a
https://hg.mozilla.org/integration/mozilla-inbound/rev/dacd814d3bb0

Comment 45

[Ed Morley [:emorley]]

Backed out for compilation failures on B2G:
https://tbpl.mozilla.org/php/getParsedLog.php?id=41528515&tree=Mozilla-Inbound

Seeing as this has now been backed out twice - please can we try a push to Try before the next relanding? :-)

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5ac32bb1487a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f7c51ce94272
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2515e665defe
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6b72f1fad830
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/634de90c950f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3cf701c3b021

Comment 46

[Nicolas Silva [:nical]]

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

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 7: Remove failing assert for now

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

Comment 48

[Matt Woodrow (:mattwoodrow)]

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.

Comment 49

[Kelsey Gilbert [:jgilbert]]

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

Comment 50

[Kelsey Gilbert [:jgilbert]]

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

Comment 51

[Kelsey Gilbert [:jgilbert]]

Attachment: 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?

Comment 52

[Matt Woodrow (:mattwoodrow)]

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

Comment 53

[Sotaro Ikeda [:sotaro]]

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

Comment 54

[Sotaro Ikeda [:sotaro]]

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

Comment 55

[Sotaro Ikeda [:sotaro]]

It is not a good idea to create TextureHost. It is same to Comment 53.

Comment 56

[Sotaro Ikeda [:sotaro]]

Therefore it seems better to deliver fence to GrallocTextureSourceOGL in StreamTextureHost case.

Comment 57

[Sotaro Ikeda [:sotaro]]

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.

Comment 58

[Nicolas Silva [:nical]]

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

Comment 59

[Sotaro Ikeda [:sotaro]]

SkiaGL backed Canvas 2d also seems to use SurfaceStream.

Comment 60

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 4.5: Crash if we take the Gralloc path in TextureHost.

Comment 61

[Matt Woodrow (:mattwoodrow)]

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.

Comment 62

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 4.5: Don't use SurfStream stuff on Gonk.

Comment 63

[Matt Woodrow (:mattwoodrow)]

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.

Comment 64

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 4.5: Don't use SurfStream stuff on Gonk.

Comment 65

[Kelsey Gilbert [:jgilbert]]

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

Comment 66

[Matt Woodrow (:mattwoodrow)]

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.

Comment 67

[Kelsey Gilbert [:jgilbert]]

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

Comment 68

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 4.5: Don't use SurfStream stuff on Gonk.

r=mattwoodrow

Comment 69

[Kelsey Gilbert [:jgilbert]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1b61cfccf828
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/26d7072ac870
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8c58bf5d1779
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/68dfeae0d1be
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/058e30492755
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9f1de5457faa
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8c6e3225774d

Comment 70

[Florian Bender]

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

Comment 71

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1b61cfccf828
https://hg.mozilla.org/mozilla-central/rev/26d7072ac870
https://hg.mozilla.org/mozilla-central/rev/8c58bf5d1779
https://hg.mozilla.org/mozilla-central/rev/68dfeae0d1be
https://hg.mozilla.org/mozilla-central/rev/058e30492755
https://hg.mozilla.org/mozilla-central/rev/9f1de5457faa
https://hg.mozilla.org/mozilla-central/rev/8c6e3225774d

Comment 72

[Kelsey Gilbert [:jgilbert]]

Attachment: rolled-up patch (aurora)

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

Comment 73

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

Comment on attachment 8445552 [details] [diff] [review]
rolled-up patch (aurora)

We're disabling OMTC on Aurora/32, no need to uplift this.