Fix test failures when using new-textures on OSX

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

Tracking

unspecified
mozilla25
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments, 1 obsolete attachment)

9.07 KB, patch
nical
: review+
Details | Diff | Splinter Review
773 bytes, patch
nical
: review+
Details | Diff | Splinter Review
1.04 KB, patch
nical
: review+
Details | Diff | Splinter Review
3.06 KB, patch
nical
: review+
Details | Diff | Splinter Review
4.91 KB, patch
nical
: review+
Details | Diff | Splinter Review
1.98 KB, patch
nical
: review+
Details | Diff | Splinter Review
19.56 KB, patch
nical
: review+
Details | Diff | Splinter Review
5.78 KB, patch
nical
: review+
Details | Diff | Splinter Review
4.21 KB, patch
nical
: review+
Details | Diff | Splinter Review
2.01 KB, patch
nical
: review+
Details | Diff | Splinter Review
No description provided.
Attachment #783885 - Flags: review?(nical.bugzilla)
Attachment #783887 - Flags: review?(nical.bugzilla)
This is a bit weird. My comment explains it partially.

Previously we had a special YCbCr texture host that didn't implement the tiled iterator, so we took the path that supported picture rects.

Now we just have a linked list of 3 TextureImageTextureHostOGL objects, and these do.

We iterate over the tiles (usually just one) of the first one, and call DrawQuad. This will read from all three textures, since they're in the effect.

Though if we have actual tiles with multiple textures, then we won't be advancing the iterator on the others, so it will be broken (bug 899583 I'm assuming).

This fixes it, but you may have better ideas.
Attachment #783891 - Flags: review?(nical.bugzilla)
Attachment #783892 - Flags: review?(nical.bugzilla)
https://tbpl.mozilla.org/?tree=Try&rev=e2379f6c4675


The remaining issue (which i don't think shows up on try), is that we still have a potential race when using single buffered image clients.

My preference here would be to tell new ImageClients if they are going to be used in a double buffered way.

If they're *not* (and are going to be single buffered), then they either need to be immutable, or support locking correctly.

The implementations could then decide which of these two options is right for them, and add the immutable flag to themselves if needed.

Thoughts?
Flags: needinfo?(nical.bugzilla)
Attachment #783887 - Flags: review?(nical.bugzilla) → review+
Attachment #783884 - Flags: review?(nical.bugzilla) → review+
Attachment #783885 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 783891 [details] [diff] [review]
Implement picture rect support

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

::: gfx/layers/composite/ImageHost.cpp
@@ +72,5 @@
> +                        mPictureRect.width,
> +                        mPictureRect.height);
> +  //XXX: We might have multiple texture sources here (e.g. 3 YCbCr textures), and we're
> +  // only iterating over the tiles of the first one. Are we assuming that the tiling
> +  // will be identical? Can we ensure that somehow?

You can remove this comment: with bug 899583, when we have YCbCr textures we disable the TileIterator thing (i'll land it soon).
Attachment #783891 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 783892 [details] [diff] [review]
Recreate the TextureClient if the size changes

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

::: gfx/layers/client/TextureClient.h
@@ +123,5 @@
>    virtual bool IsAllocated() const = 0;
>  
>    virtual bool ToSurfaceDescriptor(SurfaceDescriptor& aDescriptor) = 0;
>  
> +  virtual const gfx::IntSize& GetSize() const = 0;

Please return a gfx::IntSize by value rather than by const reference. That's what we do in the rest of the texture code, IntSize is 64bit in total so it's not a problem to return it by value, and it's simpler for everyone when all the GetSize functions have the same signature.
(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Created attachment 783891 [details] [diff] [review]
> Implement picture rect support
> 
> This is a bit weird. My comment explains it partially.
> 
> Previously we had a special YCbCr texture host that didn't implement the
> tiled iterator, so we took the path that supported picture rects.
> 
> Now we just have a linked list of 3 TextureImageTextureHostOGL objects, and
> these do.
> 
> We iterate over the tiles (usually just one) of the first one, and call
> DrawQuad. This will read from all three textures, since they're in the
> effect.
> 
> Though if we have actual tiles with multiple textures, then we won't be
> advancing the iterator on the others, so it will be broken (bug 899583 I'm
> assuming).
> 
> This fixes it, but you may have better ideas.

Now TextureSource is the tile iterator, not TextureHost, so if you want to iterate over multiple textures, you must do it on the (multiple) TextureSources.

With YCbCr the Y plane is bigger than the Cb and Cr so we'd have to do some tricky stuff to get it to work properly with just disabling BigImage (the new name for in-TextureSource Tiling). Bug 899583 fixes the YCbCr issue the same way we fixed it for deprecated textures (by making sure we don't use BiImage).
Flags: needinfo?(nical.bugzilla)
(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> https://tbpl.mozilla.org/?tree=Try&rev=e2379f6c4675
> 
> 
> The remaining issue (which i don't think shows up on try), is that we still
> have a potential race when using single buffered image clients.
> 
> My preference here would be to tell new ImageClients if they are going to be
> used in a double buffered way.
> 
> If they're *not* (and are going to be single buffered), then they either
> need to be immutable, or support locking correctly.
> 
> The implementations could then decide which of these two options is right
> for them, and add the immutable flag to themselves if needed.
> 
> Thoughts?

That would work for me. With a flag in CreateImageClient that provides info about what kind of usage the ImageClient is for (immutable textures only, preallocated textures only, double buffering, single buffering, async...) and remove this info from the CompositableType (which would then be just IMAGE, CONTENT, etc.).
The nice thing is that the Host doesn't need to know about buffering or immutability.
Right now choosing the image client type is the responsibility of the caller of CreateImageClient, I am not against moving this logic into CreateImageClient.
I don't which of bug 893824, bug 900133 or bug 898129 caused it, but inbound has been closed for long enough.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e1009e4b1e1
Assignee: nobody → mh+mozilla
Assignee: mh+mozilla → matt.woodrow
Attachment #783892 - Attachment is obsolete: true
Attachment #783892 - Flags: review?(nical.bugzilla)
Attachment #785078 - Flags: review?(nical.bugzilla)
Attachment #785078 - Flags: review?(nical.bugzilla) → review+
We can probably do better again by having proper cross-process locking, but this fixes the performance regression of new textures and avoids races.
Attachment #785090 - Flags: review?(nical.bugzilla)
Comment on attachment 785090 [details] [diff] [review]
Add TEXTURE_IMMEDIATE_UPLOAD to avoid racing when single buffered.

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

r=me with this comment fixed:

::: gfx/layers/CompositorTypes.h
@@ +86,5 @@
> +  // If we're not double buffered, or uploading
> +  // within a transaction, then we need to support
> +  // locking correctly.
> +  return !(aFlags & (TEXTURE_IMMEDIATE_UPLOAD |
> +                     TEXTURE_DOUBLE_BUFFERED));

... || (!aFlags & TEXTURE_IMMUTABLE)

I see below that you use this to decide whether we should mark the texture immutable, but checking for TEXTURE_IMMUTABLE will not break it and the result of this funtion will correspond more to the function name.
Attachment #785090 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 785087 [details] [diff] [review]
Call OnTransaction *after* we possibly recreate our texture host as well

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

r=me with the unneeded mCanvasClient->OnTransaction(); removed

::: gfx/layers/client/ClientCanvasLayer.cpp
@@ +103,5 @@
>        mCanvasClient->Connect();
>        ClientManager()->Attach(mCanvasClient, this);
>      }
> +  } else {
> +    mCanvasClient->OnTransaction();

I think we don't need this
Attachment #785087 - Flags: review?(nical.bugzilla) → review+
Still not quite done here.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [leave open]
Otherwise we think they're immutable before we've drawn them at all.
Attachment #790016 - Flags: review?(nical.bugzilla)
I managed to break things so we weren't specifying TEXTURE_DEALLOCATE_HOST and it was leaking.
Attachment #790018 - Flags: review?(nical.bugzilla)
Attachment #790016 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 790019 [details] [diff] [review]
Enable new textures on OSX

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

yay \o/
Attachment #790019 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 790018 [details] [diff] [review]
Make sure we always deallocate textures

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

::: gfx/layers/ipc/ShadowLayers.cpp
@@ +383,5 @@
>    if (!aTexture->ToSurfaceDescriptor(descriptor)) {
>      NS_WARNING("Failed to serialize a TextureClient");
>      return;
>    }
> +  MOZ_ASSERT(aTexture->GetFlags() != 0);

For the B2G camera that wants to manage recycling the gralloc buffers we will need to not have TEXTURE_DEALLOCATE_CLIENT nor TEXTURE_DEALLOCATE_HOST (that's the reason this info is not just stored in one bit), so this assertion may not be correct when we get there.

I am fine with the assertion staying until we completely implement new gralloc textures but it is likely to be removed/changed at some point.
Attachment #790018 - Flags: review?(nical.bugzilla) → review+
Depends on: 907950
Depends on: 908006
Depends on: 907792
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 915944
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Depends on: 919022
You need to log in before you can comment on or make changes to this bug.