Closed
Bug 1200595
Opened 9 years ago
Closed 9 years ago
Separate the ownership and the backend logic of TextureClient
Categories
(Core :: Graphics: Layers, defect)
Core
Graphics: Layers
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: nical, Assigned: nical)
References
(Blocks 2 open bugs)
Details
Attachments
(12 files, 7 obsolete files)
19.68 KB,
patch
|
mattwoodrow
:
review+
sotaro
:
review+
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
65.87 KB,
patch
|
sotaro
:
review+
|
Details | Diff | Splinter Review |
89.82 KB,
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
65.79 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
28.49 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
6.76 KB,
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
8.06 KB,
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
7.25 KB,
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
19.22 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
10.63 KB,
patch
|
jgilbert
:
review+
|
Details | Diff | Splinter Review |
44.71 KB,
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
53.93 KB,
patch
|
mattwoodrow
:
review+
|
Details | Diff | Splinter Review |
The idea is to have a TextureData class that doesn't have any ownership semantic. It is uniquely owned by the TextureClient object which forwards backend-specific calls to the TextureData. I want this to be able to: * properly use the TextureClient's refcounting without horrible hacks when attempting to recycle textures * simplify the deallocation code * add the possibility to asynchronously deallocate textures on the client side. * remove a lot of code duplication between TextureClient implementations As a bonus, while working on this I noticed a bunch of duplicate functionalities inside TextureClient itself (like the Fence and SyncObject stuff), which I want to clean up in the process.
Assignee | ||
Updated•9 years ago
|
Summary: Separate the ownersip and the backend logic of TextureClient → Separate the ownership and the backend logic of TextureClient
Assignee | ||
Comment 1•9 years ago
|
||
I added a temporary ClientTexture inheriting from TextureClient but they'll get merged back when all texture clients are based on TextureData.
Attachment #8655416 -
Flags: review?(matt.woodrow)
Assignee | ||
Updated•9 years ago
|
Attachment #8655416 -
Flags: review?(sotaro.ikeda.g)
Updated•9 years ago
|
Attachment #8655416 -
Flags: review?(bas)
Comment 4•9 years ago
|
||
Comment on attachment 8655416 [details] [diff] [review] add the TextureData interface and make the X11 implementation use it Review of attachment 8655416 [details] [diff] [review]: ----------------------------------------------------------------- Skimmed over the actual changes a bit since they all seem fairly mechanical. I really like the concept :) ::: gfx/layers/client/TextureClient.h @@ +576,5 @@ > + virtual already_AddRefed<gfx::DrawTarget> BorrowDrawTarget() = 0; > + > + virtual void Deallocate(ISurfaceAllocator* aAllocator) = 0; > + > + virtual bool Serialize(SurfaceDescriptor& aDescrptor) = 0; aDescriptor (typo). @@ +586,5 @@ > + > + virtual bool UpdateFromSurface(gfx::SourceSurface* aSurface) = 0; > +}; > + > +class ClientTexture : public TextureClient { This naming is fun :) If it's going to survive for more than just this patch queue, then please add a comment explaining that it's a TextureClient impl that is backed by a TextureData object, and will go away when all TextureClients are that.
Attachment #8655416 -
Flags: review?(matt.woodrow) → review+
Comment 5•9 years ago
|
||
Comment on attachment 8655417 [details] [diff] [review] Make BufferTextureClient use TextureData Review of attachment 8655417 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/BufferTexture.cpp @@ +88,5 @@ > + gfx::BackendType aMoz2DBackend, TextureFlags aFlags, > + TextureAllocationFlags aAllocFlags, > + ISurfaceAllocator* aAllocator) > +{ > + if (!aAllocator || aAllocator->IsSameProcess()) { Do we want to support a nullptr allocator? The old code didn't. @@ +152,5 @@ > +{ > + switch (mFormat) { > + case gfx::SurfaceFormat::YUV: > + case gfx::SurfaceFormat::NV12: > + case gfx::SurfaceFormat::UNKNOWN: Might be better to list the formats that we do support in case someone adds some other weird video one and forget to update this. ::: gfx/layers/BufferTexture.h @@ +30,5 @@ > + size_t mBufferSize; > + gfx::SurfaceFormat mFormat; > +}; > + > +class RawYCbCrTextureBuffer : public RefCounted<RawYCbCrTextureBuffer> This appears to be unused? ::: gfx/layers/ipc/SharedRGBImage.cpp @@ +96,2 @@ > } > + return 0; nullptr
Attachment #8655417 -
Flags: review?(matt.woodrow) → review+
Updated•9 years ago
|
Attachment #8655416 -
Flags: review?(sotaro.ikeda.g) → review+
Comment 6•9 years ago
|
||
Comment on attachment 8655419 [details] [diff] [review] Gralloc part Review of attachment 8655419 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/nativewindow/GonkBufferQueueJB.cpp @@ +426,5 @@ > + ISurfaceAllocator* allocator = ImageBridgeChild::GetSingleton(); > + GrallocTextureData* texData = GrallocTextureData::Create(IntSize(w,h), > + SurfaceFormatForPixelFormat(format), > + gfx::BackendType::NONE, usage, > + allocator); Does it support to allocate chip dependent YUV format buffers allocation? 'format' seems not directly used for gralloc allocation.
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Sotaro Ikeda [:sotaro] from comment #6) > Does it support to allocate chip dependent YUV format buffers allocation? > 'format' seems not directly used for gralloc allocation. Hm, actually I might be loosing some format information in the process here (if we have pixel formats that are not handled in SurfaceFormatForPizelFormat which seems to be the case). I'll update the patch to take android's pixel format instead of SurfaceFormat as input.
Assignee | ||
Updated•9 years ago
|
Attachment #8655419 -
Flags: review?(sotaro.ikeda.g)
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8655419 -
Attachment is obsolete: true
Attachment #8657869 -
Flags: review?(sotaro.ikeda.g)
Assignee | ||
Comment 9•9 years ago
|
||
Assignee | ||
Comment 10•9 years ago
|
||
Assignee | ||
Comment 11•9 years ago
|
||
This patch moves all of the deallocation logic into a single function and adds the possibility to deallocation TextureClients on the client side without waiting for the compositor. The function handles dispatching the deallocation to the IPDL thread, blocking or not blocking, and makes sure the TextureData is deallocated after the host is gone (so we don't need the KeepAlive thing). There are a few little things to figure out, like the RecycleCallback. I would like to use an interface like TextureRecycler or whatnot, but in practice it would not be very different from what we do with the function pointer today (just a bit more flexible, and no more ClearRecycleCallback for the callback: you create a texture with a recycler and the latter decides whether to recycle the TextureData or not).
Updated•9 years ago
|
Attachment #8657869 -
Flags: review?(sotaro.ikeda.g) → review+
Assignee | ||
Comment 12•9 years ago
|
||
A bug that took me a while to figure out was that BufferTextureClient exposes pointers to its raw buffer (with some metadata at the beginning of the buffer) whereas most users of BufferTextureClient really only want to get the address of the pixels themselves and I had a place where I'd give a pointer to the beginning of the buffer instead of the beginning of the pixels. This patch is mosty identical to the one that got r+ with the notable exception that TextureClient can now expose address to pixels using the method bool BorrowMappedData(MappedTextureData& descriptor) and its YCbCr equivalent, that provide access to the pixels independently of how they are stored in the buffer instead of exposing the buffer and letting the called figure out how do read from it. As a bonus, this will allow the gralloc texture to expose its pixels with the same interface which it couldn't do previously. I think that there are some software video decoding code paths that could remove an extra copy by using gralloc instead of BufferTextureClient thanks to this (need to check). Anyway, re-asking for a review because of the code changes related to BorrowMappedData.
Attachment #8655417 -
Attachment is obsolete: true
Attachment #8678883 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 14•9 years ago
|
||
Assignee | ||
Comment 15•9 years ago
|
||
Attachment #8657870 -
Attachment is obsolete: true
Attachment #8657871 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8678885 -
Flags: review?(bas)
Assignee | ||
Updated•9 years ago
|
Attachment #8678887 -
Flags: review?(matt.woodrow)
Comment 18•9 years ago
|
||
Comment on attachment 8678883 [details] [diff] [review] TextureData: BufferTextureClient + improve the way we expose pointers to the texture's buffer Review of attachment 8678883 [details] [diff] [review]: ----------------------------------------------------------------- I'm trusting that most of the new BufferTextureData code is just copied from BufferTextureClient, I didn't check it line by line. ::: gfx/layers/client/TextureClient.cpp @@ +345,5 @@ > +{ > + MOZ_ASSERT(mValid); > + > + // TODO - SharedRGBImage just accesses the buffer without properly locking > + // the texture. It's bad. File a followup to fix this?
Attachment #8678883 -
Flags: review?(matt.woodrow) → review+
Updated•9 years ago
|
Attachment #8678887 -
Flags: review?(matt.woodrow) → review+
Comment 19•9 years ago
|
||
Comment on attachment 8678891 [details] [diff] [review] TextureData: android surface Review of attachment 8678891 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/client/TextureClient.cpp @@ +939,5 @@ > uint8_t* dst = aDst.data + i * aDst.stride; > for (int32_t j = 0; j < size.width; ++j) { > *dst = *src; > src += 1 + skip; > + dst += 1 + aDst.skip; I assume you'll fix this in the original patch, not here. ::: gfx/layers/opengl/TextureClientOGL.cpp @@ +79,5 @@ > + if (!aSurfTex || !XRE_IsParentProcess()) { > + return nullptr; > + } > + > + // XXX - This is quite sad and slow. Can you elaborate on this? (in the previous patch too). I doubt this is useful as it stands (except to you).
Attachment #8678891 -
Flags: review?(matt.woodrow) → review+
Comment 20•9 years ago
|
||
Comment on attachment 8678893 [details] [diff] [review] TextureData: MacIOSurface Review of attachment 8678893 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/opengl/MacIOSurfaceTextureClientOGL.cpp @@ +3,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > #include "MacIOSurfaceTextureClientOGL.h" > +#include "mozilla/gfx/MacIOSurface.h" Whitespace.
Attachment #8678893 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 22•9 years ago
|
||
SharedSurfaceTextureClient was trickier to convert because a lot of the SharedSurface code poke at the SharedTextureClient's internals, so I kept a special TextureClient class for it which is a bit awkward, but the patch remains reasonably small and we still have a dedicated TextureData object which is what I need to finally fix the destruction sequence and recycling logic. The interaction between TextureClient and SharedSurface is still full of surprises and it'll eventually need to be improved in a way that doesn't require synchronous deallocation and where the data is really owned by the TextureClient. so I still consider it in a temporary state and there's plenty more to do in this area.
Attachment #8683668 -
Flags: review?(jgilbert)
Comment 23•9 years ago
|
||
Comment on attachment 8683668 [details] [diff] [review] TextureData: SharedSurface Review of attachment 8683668 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/client/TextureClientSharedSurface.h @@ +55,5 @@ > + virtual bool Serialize(SurfaceDescriptor& aOutDescriptor) override; > + > + virtual void Deallocate(ISurfaceAllocator*) override; > + > + gl::SharedSurface* Surf() const { return mSurf.get(); } Why is this added? mSurf is const, so it is its own getter. No need to add a useless function.
Attachment #8683668 -
Flags: review?(jgilbert) → review+
Assignee | ||
Comment 24•9 years ago
|
||
There we go, the awkward ClientTexture class is gone.
Attachment #8684974 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 25•9 years ago
|
||
This is one of the big wins from this patch queue. TextureClient's deallocation code used to be spread in different places including some duplicated parts in ShadowLayers.cpp and ImageBridgeChiold.cpp. Now all of the logic goes into one function which is much clearer. In addition, Textures can be deallocated on the client side asynchronously which was not possible before that and that will let us remove some really nasty and slow synchronous deallocation cases. And also we don't need the KeepUntilFullDeallocation stuff and other refcounting tricks anymore, because all of the TextureData objects are kept until TextureChild::ActorDestroy is called.
Attachment #8657875 -
Attachment is obsolete: true
Attachment #8684979 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 26•9 years ago
|
||
The previous version had a silly issue with synchronous deallocation.
Attachment #8684979 -
Attachment is obsolete: true
Attachment #8684979 -
Flags: review?(matt.woodrow)
Assignee | ||
Comment 27•9 years ago
|
||
Bas, in case that eases the reviews, the first patch (the one that introduces the ClientTexture class) adds some amount of ugliness which is removed in the last patch of the series. I had to do that to keep things working until all texture types were converted, but it looks good once all patches are in place. The only thing to worry about is whether the XYZTextureData type converted from TextureClientXYZ has the same behavior. Separating TextureClient and TextureData into several objects has several benefits, some of which are coming from the fact that the TextureData needs to stay alive for the entire deallocation handshake between the TextureChild and TextureParent, while the TextureHost object is destroyed earlier at the beginning of the handshake when its reference count reaches zero. It also reduces the amount of duplicated code since TextureClient takes care of checking inputs/states and the TextureData just implements the backend bits, but fixing the complexity and awfulness of how we currently destroy textures is the strongest motivator here.
Comment 28•9 years ago
|
||
Comment on attachment 8684974 [details] [diff] [review] Merge back TextureClient and ClientTexture Review of attachment 8684974 [details] [diff] [review]: ----------------------------------------------------------------- \o/
Attachment #8684974 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 29•9 years ago
|
||
Review ping. The patchy queue is looking good on try and rebasing it is a long a risky process because of the amount of changes involved.
Comment 30•9 years ago
|
||
Comment on attachment 8655416 [details] [diff] [review] add the TextureData interface and make the X11 implementation use it Review of attachment 8655416 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/client/TextureClient.cpp @@ +209,5 @@ > + RefPtr<DataSourceSurface> dataSurf = snapshot->GetDataSurface(); > + mReadbackSink->ProcessReadback(dataSurf); > + } > + } > + mBorrowedDrawTarget = nullptr; Currently we keep the DT around in a lot of cases even when the texture is unlocked I think.. are we sure we want to null this? Creating the DT can have its cost. Although I guess we can have mData decide to hold on to it or now. ::: gfx/layers/client/TextureClient.h @@ +588,5 @@ > +}; > + > +class ClientTexture : public TextureClient { > +public: > + ClientTexture(TextureData* aData, TextureFlags aFlags, ISurfaceAllocator* aAllocator) Since you're deleting the object pointed to by aData here in the destructor be sure to comment on the transfer of ownership and perhaps even use some C++ magic to add safety. @@ +616,5 @@ > + > + virtual bool ToSurfaceDescriptor(SurfaceDescriptor& aOutDescriptor) override; > + > + already_AddRefed<TextureClient> > + CreateSimilar(TextureFlags aFlags = TextureFlags::DEFAULT, nit: indent
Attachment #8655416 -
Flags: review?(bas) → review+
Comment 31•9 years ago
|
||
Comment on attachment 8678885 [details] [diff] [review] TextureData: D3D9 implementation Review of attachment 8678885 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/d3d9/TextureD3D9.cpp @@ +538,4 @@ > return nullptr; > } > > + RefPtr<DrawTarget> dt; It worries me that we'll now longer be caching this through lock/unlock pairs.
Attachment #8678885 -
Flags: review?(bas) → review+
Comment 32•9 years ago
|
||
Comment on attachment 8678884 [details] [diff] [review] TextureData: D3D11 implementation Review of attachment 8678884 [details] [diff] [review]: ----------------------------------------------------------------- I'm assuming this is mostly code moving around, I haven't reviewed the bodies of a bunch of actual functions in detail, if this was a mistake let me know. ::: gfx/layers/d3d11/TextureD3D11.cpp @@ +680,5 @@ > } > > +already_AddRefed<DrawTarget> > +D3D11TextureData::BorrowDrawTarget() > +{ It's good to see we're continuing to cache this one since we definitely have to do so here.
Attachment #8678884 -
Flags: review?(bas) → review+
Updated•9 years ago
|
Attachment #8683229 -
Flags: review?(bas) → review+
Assignee | ||
Comment 33•9 years ago
|
||
I tried to keep caching the DT in the places we used to but I'll double check and make sure we keep around the D3D9 one if we did before the patch. I have a patch in the making to use UniquePtr with TextureData (should have done that in the first place)
Assignee | ||
Comment 34•9 years ago
|
||
Woops, I forgot to set the review flag for this one. A bit too much got folded into this patch, sorry, but basically it consolidates the destruction logic in one place that is easier to understand and cover all corner cases (there are more of them than I expected). It also cleans up a whole bunch of stuff that was made useless by this patch and the previous ones, and contains the various fixes I needed to get things green on try.
Attachment #8685041 -
Attachment is obsolete: true
Attachment #8689130 -
Flags: review?(matt.woodrow)
Comment 35•9 years ago
|
||
Comment on attachment 8689130 [details] [diff] [review] Consolidate the texture deallocation code + fixes Review of attachment 8689130 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/2D.h @@ +1132,5 @@ > { > public: > + struct Config { > + int32_t mMaxTextureSize; > + int32_t mMaxAllocSize; What is this added for? I don't see it being used. ::: gfx/layers/client/TextureClient.cpp @@ +225,5 @@ > +/// > +/// This funciton takes care of dispatching work to the right thread using > +/// a synchronous proxy if needed, and handles client/host deallocation. > +void > +TextureClientDeallocation(TextureDeallocParams params) DoTextureClientDeallocation, or DeallocateTextureClient.
Attachment #8689130 -
Flags: review?(matt.woodrow) → review+
Assignee | ||
Comment 36•9 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #35) > What is this added for? I don't see it being used. Ah, darnit. That's me working on two patch sets at the same time. This has nothing to do here. > DoTextureClientDeallocation, or DeallocateTextureClient. ok
Comment 37•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe2164aa1468 https://hg.mozilla.org/integration/mozilla-inbound/rev/6dc38a1e6073 https://hg.mozilla.org/integration/mozilla-inbound/rev/0092c9d7a86b https://hg.mozilla.org/integration/mozilla-inbound/rev/a8cdfbf443d8 https://hg.mozilla.org/integration/mozilla-inbound/rev/6819f6d82287 https://hg.mozilla.org/integration/mozilla-inbound/rev/b555b130d439 https://hg.mozilla.org/integration/mozilla-inbound/rev/09e71ba15ea8 https://hg.mozilla.org/integration/mozilla-inbound/rev/a369a2983ceb https://hg.mozilla.org/integration/mozilla-inbound/rev/9c27c8e2c021 https://hg.mozilla.org/integration/mozilla-inbound/rev/e55376bd2cf8 https://hg.mozilla.org/integration/mozilla-inbound/rev/4b88e8a3d8af https://hg.mozilla.org/integration/mozilla-inbound/rev/c13228f84d85
Comment 39•9 years ago
|
||
(In reply to Pulsebot from comment #37) sorry had to back this out since something seems have caused https://treeherder.mozilla.org/logviewer.html#?job_id=17609171&repo=mozilla-inbound here :(
Flags: needinfo?(nical.bugzilla)
Comment 40•9 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/027edbd76ba2
Comment 41•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb84403701b7 https://hg.mozilla.org/integration/mozilla-inbound/rev/8cc12f12f3d1 https://hg.mozilla.org/integration/mozilla-inbound/rev/0dc93424546c https://hg.mozilla.org/integration/mozilla-inbound/rev/ed34bc528a1b https://hg.mozilla.org/integration/mozilla-inbound/rev/ab326c34f1cf https://hg.mozilla.org/integration/mozilla-inbound/rev/68aba6b39588 https://hg.mozilla.org/integration/mozilla-inbound/rev/add3fe9afc0c https://hg.mozilla.org/integration/mozilla-inbound/rev/3ad5a4c457fe https://hg.mozilla.org/integration/mozilla-inbound/rev/fc1fbb97c8eb https://hg.mozilla.org/integration/mozilla-inbound/rev/7663208f1582 https://hg.mozilla.org/integration/mozilla-inbound/rev/65da564f952c https://hg.mozilla.org/integration/mozilla-inbound/rev/cf8cf1a039dd
I had to back this all out to fix some b2g crashes https://treeherder.mozilla.org/logviewer.html#?job_id=17749266&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/9783264529d3
It's possible this also caused an intermittent failure in web-platform-tests(1) like https://treeherder.mozilla.org/logviewer.html#?job_id=17751633&repo=mozilla-inbound but I'm less sure about that one.
Comment 45•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3a6b8f4f2cd https://hg.mozilla.org/integration/mozilla-inbound/rev/c2674884d1f7 https://hg.mozilla.org/integration/mozilla-inbound/rev/8cb40615eeb0 https://hg.mozilla.org/integration/mozilla-inbound/rev/981a6bc9b1ae https://hg.mozilla.org/integration/mozilla-inbound/rev/4b2a5d9c0ccd https://hg.mozilla.org/integration/mozilla-inbound/rev/d296b6686792 https://hg.mozilla.org/integration/mozilla-inbound/rev/0083654ce7df https://hg.mozilla.org/integration/mozilla-inbound/rev/cccfd06730e9 https://hg.mozilla.org/integration/mozilla-inbound/rev/ca5f63ec86ec https://hg.mozilla.org/integration/mozilla-inbound/rev/94e3e5bfbeb3 https://hg.mozilla.org/integration/mozilla-inbound/rev/971189684dd5 https://hg.mozilla.org/integration/mozilla-inbound/rev/49e11078c536
Comment 46•9 years ago
|
||
Hello, this has caused build bustage on B2G emulator builds (as of now). Can you please look urgently?
Comment 49•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d3a6b8f4f2cd https://hg.mozilla.org/mozilla-central/rev/c2674884d1f7 https://hg.mozilla.org/mozilla-central/rev/8cb40615eeb0 https://hg.mozilla.org/mozilla-central/rev/981a6bc9b1ae https://hg.mozilla.org/mozilla-central/rev/4b2a5d9c0ccd https://hg.mozilla.org/mozilla-central/rev/d296b6686792 https://hg.mozilla.org/mozilla-central/rev/0083654ce7df https://hg.mozilla.org/mozilla-central/rev/cccfd06730e9 https://hg.mozilla.org/mozilla-central/rev/ca5f63ec86ec https://hg.mozilla.org/mozilla-central/rev/94e3e5bfbeb3 https://hg.mozilla.org/mozilla-central/rev/971189684dd5 https://hg.mozilla.org/mozilla-central/rev/49e11078c536 https://hg.mozilla.org/mozilla-central/rev/54693bad0ca1 https://hg.mozilla.org/mozilla-central/rev/4847034e573e
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(nical.bugzilla)
Comment 50•9 years ago
|
||
Hi Nical, Recently I'd working on 2d canvas in Worker thread, which the bug id is Bug 801176. It aims at operating 2d canvas in Worker thread. It seems that your recently patch landing in this bug changes behaviors with my working patch. https://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/TextureClient.cpp#481 You put a check mechanism to make sure this function runs in main thread. May I know your concern if it runs in Worker? Really thanks for your comment. :)
Flags: needinfo?(nical.silva)
Assignee | ||
Comment 51•9 years ago
|
||
(In reply to Vincent Liu[:vliu] from comment #50) > Hi Nical, > > Recently I'd working on 2d canvas in Worker thread, which the bug id is Bug > 801176. It aims at operating 2d canvas in Worker thread. It seems that your > recently patch landing in this bug changes behaviors with my working patch. > > https://dxr.mozilla.org/mozilla-central/source/gfx/layers/client/ > TextureClient.cpp#481 > > You put a check mechanism to make sure this function runs in main thread. > May I know your concern if it runs in Worker? Really thanks for your > comment. :) Hi, The video pipeline needed to update a TextureClient off the main thread and we had to add the TextureClient::UpdateFromSurface in order to do that. I think that this is because of a restriction with the D2D backend. I guess that we would be fine doing this with the cario-image backend but probably not with skia, and probably not with cairo-xlib either, I don't know for core graphics (Bas, probably knows more about the threading restrictions involved here with the different backends). If you only need to copy the content of the canvas into the TextureClient, then you can use UpdateFromSurface. If the canvas draws directly into the TextureClient, then we need to figure something out (and probably restrict to cairo-image, and even with this, we'll run into issues rendering text (can't do that in parallel with our version of cairo, right now).
Flags: needinfo?(nical.silva) → needinfo?(bas)
Comment 52•9 years ago
|
||
nical, jerry found that attachment 8657869 [details] [diff] [review] remove WaitForBufferOwnership(). It is necessary to wait the case when fence is not delivered to client side. It could cause a regression.
Flags: needinfo?(nical.bugzilla)
Updated•9 years ago
|
Flags: needinfo?(nical.bugzilla)
Comment 53•9 years ago
|
||
(In reply to Nicolas Silva [:nical] from comment #51) > If you only need to copy the content of the canvas into the TextureClient, > then you can use UpdateFromSurface. If the canvas draws directly into the > TextureClient, then we need to figure something out (and probably restrict > to cairo-image, and even with this, we'll run into issues rendering text > (can't do that in parallel with our version of cairo, right now). Please correct me if I got anything wrong. As your suggestion, I need to get SourceSurface as argument before calling UpdateFromSurface. To get it, I need to have DrawTarget. The thing is I can't get DrawTarget when calling BorrowDrawTarget. If so, could you please have more suggestions for the problem I met? Thanks.
Flags: needinfo?(nical.silva)
Assignee | ||
Comment 54•9 years ago
|
||
(In reply to Vincent Liu[:vliu] from comment #53) > To get it, I need to have DrawTarget. You don't need a DrawTarget to get a SourceSurface, although that's one way to do it. If the data you want to copy into the TextureClient is on the cpu, you are probably better off wrapping a temporary SourceSurface around with gfx::Factory::CreateWrappingDataSourceSurface. What kind of backend are you dealing with off the main thread? is it exclusively cpu backends (cairo-image and skia) or can it also be a gpu backend like D2D?
Flags: needinfo?(nical.silva)
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(bas)
You need to log in
before you can comment on or make changes to this bug.
Description
•