Closed Bug 1252835 Opened 9 years ago Closed 8 years ago

Make TextureHost recycling implicit

Categories

(Core :: Graphics: Layers, defect)

Product:
Core
Component:
Graphics: Layers
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: sotaro, Assigned: sotaro)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 60 obsolete files)

patch part 0 - Remove TextureClientRecycleAllocator workaround of bug 1260611
4.65 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
patch part 1 - Make TextureHost recycling implicit
171.56 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
patch part 2 - Check if CompositableRef works as expected
11.83 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
patch part 3 - Keep video recycling workaround of bug 1260611
22.31 KB, patch
nical
: review+
Details | Diff | Splinter Review
patch part 4 - Add CancelWaitForRecycle()
11.97 KB, patch
nical
: review+
Details | Diff | Splinter Review
TexureHost recycling uses explicit mechanism. It makes code annoying. It should be recycled by an implicit way.
Assignee: nobody → sotaro.ikeda.g
Depends on: 1253478
Depends on: 1253481
Depends on: 1253489
Attachment #8726507 - Attachment is obsolete: true
See Also: → 1227718
Attachment #8726582 - Attachment is obsolete: true
(In reply to Sotaro Ikeda [:sotaro] from comment #3) > Created attachment 8731549 [details] [diff] [review] > patch - Make TextureHost recycling implicit rebased.
Attachment #8731549 - Attachment is obsolete: true
The patch seems to work basically, but it seems to cause TileLayer's TextureCliets waits longer.
Address Tiled Layers's problem.
Attachment #8731556 - Attachment is obsolete: true
Attachment #8731556 - Attachment is obsolete: false
Attachment #8732047 - Attachment is obsolete: true
Rebased.
Attachment #8731556 - Attachment is obsolete: true
Blocks: 1254006
Attachment #8733684 - Attachment is obsolete: true
Attachment #8733788 - Attachment is obsolete: true
Attachment #8733794 - Attachment is obsolete: true
Remove an unnecessary member.
Attachment #8733858 - Attachment is obsolete: true
Code clean up.
Attachment #8734205 - Attachment is obsolete: true
During testing attachment 8734223 [details] [diff] [review], I saw a problem during video playback on b2g aries. Sometimes video did not resume. From investigation, the problem is not related to attachment 8734223 [details] [diff] [review]. It is a problem of media framework, VideoSink holds too many video buffers and android::MediaCodec's out port becomes out of buffers.
Depends on: 1259366
Rebased.
Attachment #8734223 - Attachment is obsolete: true
Rebase and fixed tiled layer problems on gonk.
Attachment #8734597 - Attachment is obsolete: true
Blocks: unaccel-video
Code clean up.
Attachment #8736242 - Attachment is obsolete: true
attachment 8736595 [details] [diff] [review] still has a problem of tiled layer on gonk.
(In reply to Sotaro Ikeda [:sotaro] from comment #18) > attachment 8736595 [details] [diff] [review] still has a problem of tiled > layer on gonk. It seems to be caused by TileClient::DiscardFrontBuffer(). Assumption of the function seems to be broken during progressive update. TextureClient was returned to TextureClientPool even when TextureClient is still used on host side.
Use TextureClient recycle callback in TextureClientPool.
Fixed recycling problems.
Attachment #8737030 - Attachment is obsolete: true
Update nits.
Attachment #8736595 - Attachment is obsolete: true
Attachment #8737108 - Attachment is obsolete: true
Attachment #8737115 - Attachment is obsolete: true
Attachment #8737451 - Attachment is obsolete: true
Attachment #8737105 - Attachment description: patch part 2 - Make TextureHost recycling implicit → patch part 2 - Use recycle callback in TextureClientPool
Attachment #8737454 - Attachment is obsolete: true
Attachment #8737468 - Attachment is obsolete: true
Attachment #8737105 - Attachment is obsolete: true
Attachment #8737475 - Attachment is obsolete: true
Attachment #8737491 - Attachment is obsolete: true
Attachment #8737496 - Attachment is obsolete: true
Attachment #8737502 - Attachment is obsolete: true
Attachment #8737503 - Attachment is obsolete: true
Comment on attachment 8737560 [details] [diff] [review] patch part 1 - Make TextureHost recycling implicit nical, can you feedback to the patch?
Attachment #8737560 - Flags: feedback?(nical.bugzilla)
attachment 8737560 [details] [diff] [review] changes a way of recycling between TextureClient and TextureHost. When TextureFlags::RECYCLE is set to TextureClient, TextureClient is expected to be recycled via recycle callback. Recycleing of TextureHost is automatically cared when the TextureFlags::RECYCLE is set. With the patch applied, usage of AsyncTransactionTrackersHolder becomes minimized. Only one AsyncTransactionTrackersHolder exists per one ImageBridgeChild. It is used only to handle ImageBridgeChild::FlushAllImages(). Recycleing of TextureHost is checked by using CompositableRef + "use texture generation" by ShadowLayerForwarder and ImageBridgeChild. They hold TextureClient ref until they receive "host side usage end" or their shutdown. When CompositableRef becomes 0, TextureHost is thought that its usage is ended. The CompositableRef and TextureHost's "use texture generation" is notified to TextureClient. The "use texture generation" is used to care async remote operation between client and host. It is set by ShadowLayerForwarder and ImageBridgeChild. On gonk, we also need to care about a recycling without recycle callback to deliver fence handle. We continue to support it until all layers transaction becomes async.
(In reply to Sotaro Ikeda [:sotaro] from comment #37) > The CompositableRef and TextureHost's "use texture > generation" is notified to TextureClient. Correction: The generation is notified to ShadowLayerForwarder or ImageBridgeChild and they decide if the host side usage is ended.
Comment on attachment 8737560 [details] [diff] [review] patch part 1 - Make TextureHost recycling implicit Review of attachment 8737560 [details] [diff] [review]: ----------------------------------------------------------------- Still going through the patch, I am slowly getting a better image of the recycling mechanism but I haven't connected all of the pieces in my head yet. More comments would help (not only for the reviews/feedbacks, but in general). So far I like what I see. ::: gfx/layers/client/TextureClient.cpp @@ +196,5 @@ > mTextureData = nullptr; > } > } > > +/* static */ Atomic<uint64_t> TextureClient::sSerialCounter(0); nit: A comment explaining what this serial counter is used for would be useful. ::: gfx/layers/ipc/CompositableTransactionParent.cpp @@ +230,5 @@ > NS_ASSERTION(tiledHost, "The compositable is not tiled"); > > const SurfaceDescriptorTiles& tileDesc = op.tileLayerDescriptor(); > + > + // Set generation to TextureHost and enaure TextureHost::ReleaseCompositableRef() is called. nit: s/enaure/ensure/
Comment on attachment 8737560 [details] [diff] [review] patch part 1 - Make TextureHost recycling implicit Review of attachment 8737560 [details] [diff] [review]: ----------------------------------------------------------------- It took me a while to get an idea of what the UseTextureGeneration (which is passed around everywhere in this patch) is useful for. I think that I would prefer a TransactionId (incremented every transaction) rather than a UseTextureGeneration. I think both give you the same type information you need here, which is "what was the last moment the texture was used, and is this OpNotifyCompositableRefReleased corresponding to the same moment or is it out of date". The UseTextureGeneration gives you a finer granularity of "moment" but I think that a transaction-level granularity is enough for your purpose (I might be wrong, let me know). The advantage of the Transaction id is that it's easier to understand than "UseTexture" (I think), and I have completely unrelated plans that would make use of an incremented TransactionId, so it would be more useful for other purposes as well. What do you think? ::: gfx/layers/ipc/CompositableForwarder.h @@ +177,5 @@ > { > return mTextureFactoryIdentifier; > } > > + uint64_t GetUseTextureGeneration() { return ++mUseTextureGeneration; } It's a bit counter-intuitive that a method that looks like a Getter will return a different value every time. How about GetNewUseTextureGeneration() to make it clear that it's here that we advance the generation counter?
(In reply to Nicolas Silva [:nical] from comment #39) > Comment on attachment 8737560 [details] [diff] [review] > patch part 1 - Make TextureHost recycling implicit > > Review of attachment 8737560 [details] [diff] [review]: > ----------------------------------------------------------------- > > Still going through the patch, I am slowly getting a better image of the > recycling mechanism but I haven't connected all of the pieces in my head > yet. More comments would help (not only for the reviews/feedbacks, but in > general). So far I like what I see. > > ::: gfx/layers/client/TextureClient.cpp > @@ +196,5 @@ > > mTextureData = nullptr; > > } > > } > > > > +/* static */ Atomic<uint64_t> TextureClient::sSerialCounter(0); > > nit: A comment explaining what this serial counter is used for would be > useful. I am going to add a comment in a next patch. > > ::: gfx/layers/ipc/CompositableTransactionParent.cpp > @@ +230,5 @@ > > NS_ASSERTION(tiledHost, "The compositable is not tiled"); > > > > const SurfaceDescriptorTiles& tileDesc = op.tileLayerDescriptor(); > > + > > + // Set generation to TextureHost and enaure TextureHost::ReleaseCompositableRef() is called. > > nit: s/enaure/ensure/ I'll update in a next patch.
(In reply to Nicolas Silva [:nical] from comment #40) > Comment on attachment 8737560 [details] [diff] [review] > patch part 1 - Make TextureHost recycling implicit > > Review of attachment 8737560 [details] [diff] [review]: > ----------------------------------------------------------------- > > It took me a while to get an idea of what the UseTextureGeneration (which is > passed around everywhere in this patch) is useful for. > I think that I would prefer a TransactionId (incremented every transaction) > rather than a UseTextureGeneration. I think both give you the same type > information you need here, which is "what was the last moment the texture > was used, and is this OpNotifyCompositableRefReleased corresponding to the > same moment or is it out of date". The UseTextureGeneration gives you a > finer granularity of "moment" but I think that a transaction-level > granularity is enough for your purpose (I might be wrong, let me know). The > advantage of the Transaction id is that it's easier to understand than > "UseTexture" (I think), and I have completely unrelated plans that would > make use of an incremented TransactionId, so it would be more useful for > other purposes as well. > > What do you think? Yea, it seems to work. I am going to update in a next patch. > > ::: gfx/layers/ipc/CompositableForwarder.h > @@ +177,5 @@ > > { > > return mTextureFactoryIdentifier; > > } > > > > + uint64_t GetUseTextureGeneration() { return ++mUseTextureGeneration; } > > It's a bit counter-intuitive that a method that looks like a Getter will > return a different value every time. How about GetNewUseTextureGeneration() > to make it clear that it's here that we advance the generation counter? I am going to replace UseTextureGeneration with TransactionId.
I am waiting for the next version of the patch before going for another round of feedbacks, if that's okay with you.
Attachment #8737560 - Flags: feedback?(nical.bugzilla)
Rebased.
Attachment #8737478 - Attachment is obsolete: true
Attachment #8737560 - Attachment is obsolete: true
There is other transaction id(by refresh driver). Then the id was named FwdTransactionId.
Attachment #8741199 - Flags: review?(nical.bugzilla)
Attachment #8741199 - Flags: review?(nical.bugzilla) → feedback?(nical.bugzilla)
Attachment #8741199 - Flags: feedback?(nical.bugzilla)
Update nits.
Attachment #8741199 - Attachment is obsolete: true
Attachment #8741207 - Flags: feedback?(nical.bugzilla)
:nical, about when can you feedback to the patch? Thanks.
Flags: needinfo?(nical.bugzilla)
Comment on attachment 8741207 [details] [diff] [review] patch part 1 - Make TextureHost recycling implicit Review of attachment 8741207 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, I am being pickier than usual on the terminology. This is because the whole thing is pretty complicated and even after several rounds of feedbacks I am still forgetting stuff and having trouble understanding some parts. So I think it's important to compensate for the complexity by being very explicit in the naming and documentation. I'll probably have more nits about that. As far as the logic goes I feel mostly good about this patch (just would like some clarification on the AutoCompositableAddRef business). ::: gfx/layers/client/TextureClient.h @@ +588,5 @@ > + void WaitFenceHandleOnImageBridge(Mutex& aMutex); > + void ClearWaitFenceHandleOnImageBridge(Mutex& aMutex); > + void CancelWaitFenceHandleOnImageBridge(); > + > + void UpdateFwdTransactionId(uint64_t aTransactionId) nit: SetLastFwdTransactionId would be a better name because you already use UpdateFwdTransactionId to generate the ids which is different, and also it would better document what this is about: marking on the TextureClient the id of last transaction it has been used in. Generally, I find that having the type of the parameter in the name of the function can be useful, but not as much as having the reason we us the parameter in the name (I say that because I also have the tendency to name things like "SetFoo(Foo*)" rather than think of a more meaningful names). ::: gfx/layers/composite/TextureHost.h @@ +40,5 @@ > > class BufferDescriptor; > class Compositor; > class CompositableParentManager; > +class GrallocTextureHostOGL; nit: is this needed? @@ +487,5 @@ > * Get the TextureHost corresponding to the actor passed in parameter. > */ > static TextureHost* AsTextureHost(PTextureParent* actor); > > + static uint64_t AsTextureSerial(PTextureParent* actor); nit: "As" implies that the two representations overlay the same thing, which is sort of OK for TextureHost and its IPDL actor, but doesn't make as much sense for a texture and an integer id (which doesn't represent the texture itself but the last transaction it was used in) GetTextureSerial would be better I think. ::: gfx/layers/ipc/CompositableTransactionParent.cpp @@ +69,5 @@ > !aPictureRect.IsEmpty(); > } > #endif > > +class MOZ_STACK_CLASS AutoTilesCompositableAddRef Could you give a bit more detail about why we need this? setting the FwdTransactionId could be done in one of the loops in UseTiles, and It's not entirely clear to me why we need to AddCompositableRef and ReleaseCompositableRef for all textures when receiving the transaction. Is it because a texture may end up never being AddCompositableRef'ed because of whatever failure case and that would cause the child side to wait forever? If that's so the relationship between the CompositableRef and the child side deadlocking isn't explicit and I fear that we'll eventually break this guarantee without knowing. Having a method that gets called on the texture whenever it fails that does the signaling to the child explicitly would be easier to understand, although off hands I don't have a precise idea of how to make something like this explicit in the API in a safe way. I am not sure that's the actual problem this is solving. At least a detailed comment would help a lot. ::: gfx/layers/ipc/CompositableTransactionParent.h @@ +32,5 @@ > > virtual void SendAsyncMessage(const InfallibleTArray<AsyncParentMessageData>& aMessage) = 0; > > + virtual void NotifyCompositableRefReleased(PTextureParent* aTexture, > + uint64_t aTransactionId) = 0; How about "NotifiyNotUsed" instead? Sounds like that name would better discribe the intent (which is to notify that texture is no longer used by the compositor, rather than the symptom (the ReleaseCompositableRef was called down to a ref count of 0).
Attachment #8741207 - Flags: feedback?(nical.bugzilla) → feedback+
Flags: needinfo?(nical.bugzilla)
Comment on attachment 8740835 [details] [diff] [review] patch part 2 - Use recycle callback in TextureClientPool Recycling problem should be addressed by bug 1265873
Attachment #8740835 - Attachment is obsolete: true
Depends on: 1265873
(In reply to Nicolas Silva [:nical] from comment #49) > Comment on attachment 8741207 [details] [diff] [review] > patch part 1 - Make TextureHost recycling implicit > > ::: gfx/layers/client/TextureClient.h > @@ +588,5 @@ > > + void WaitFenceHandleOnImageBridge(Mutex& aMutex); > > + void ClearWaitFenceHandleOnImageBridge(Mutex& aMutex); > > + void CancelWaitFenceHandleOnImageBridge(); > > + > > + void UpdateFwdTransactionId(uint64_t aTransactionId) > > nit: SetLastFwdTransactionId would be a better name because you already use > UpdateFwdTransactionId to generate the ids which is different, and also it > would better document what this is about: marking on the TextureClient the > id of last transaction it has been used in. I will update a patch and add a comment here. > > ::: gfx/layers/composite/TextureHost.h > @@ +40,5 @@ > > > > class BufferDescriptor; > > class Compositor; > > class CompositableParentManager; > > +class GrallocTextureHostOGL; > > nit: is this needed? It is not necessary anymore. It was necessary in early stage of implementation. > > @@ +487,5 @@ > > * Get the TextureHost corresponding to the actor passed in parameter. > > */ > > static TextureHost* AsTextureHost(PTextureParent* actor); > > > > + static uint64_t AsTextureSerial(PTextureParent* actor); > > nit: "As" implies that the two representations overlay the same thing, which > is sort of OK for TextureHost and its IPDL actor, but doesn't make as much > sense for a texture and an integer id (which doesn't represent the texture > itself but the last transaction it was used in) > > GetTextureSerial would be better I think. I will update in a next patch. > > ::: gfx/layers/ipc/CompositableTransactionParent.cpp > @@ +69,5 @@ > > !aPictureRect.IsEmpty(); > > } > > #endif > > > > +class MOZ_STACK_CLASS AutoTilesCompositableAddRef > > Could you give a bit more detail about why we need this? > > setting the FwdTransactionId could be done in one of the loops in UseTiles, > and It's not entirely clear to me why we need to AddCompositableRef and > ReleaseCompositableRef for all textures when receiving the transaction. > Is it because a texture may end up never being AddCompositableRef'ed because > of whatever failure case and that would cause the child side to wait forever? > If that's so the relationship between the CompositableRef and the child side > deadlocking isn't explicit and I fear that we'll eventually break this > guarantee without knowing. > Having a method that gets called on the texture whenever it fails that does > the signaling to the child explicitly would be easier to understand, > although off hands I don't have a precise idea of how to make something like > this explicit in the API in a safe way. I am not sure that's the actual > problem this is solving. > At least a detailed comment would help a lot. AutoTilesCompositableAddRef is used to ensure to call ReleaseCompositableRef. If CompositableHost does not call TextureHost's AddCompositableRef, ReleaseCompositableRef will not be called. ReleaseCompositableRef have to be called if TextureClient has TextureFlags::RECYCLE flag. When TextureClient has TextureFlags::RECYCLE, CompositableForwarder holds its ref until host sides end its usage. ReleaseCompositableRef is a trigger to send host side's end of its usage. If ReleaseCompositableRef is not called, CompositableForwarder is going to hold many TextureClients. The symptom becomes memory leak. Except gonk, it does not cause deadlocking on client side. SurfaceFactory and TextureClientRecycleAllocator just allocate new TextureClient if TextureCliets are not recycled. It seems better to detect the leak happening as ASSERT. I am going to add a check of it. > ::: gfx/layers/ipc/CompositableTransactionParent.h > @@ +32,5 @@ > > > > virtual void SendAsyncMessage(const InfallibleTArray<AsyncParentMessageData>& aMessage) = 0; > > > > + virtual void NotifyCompositableRefReleased(PTextureParent* aTexture, > > + uint64_t aTransactionId) = 0; > > How about "NotifiyNotUsed" instead? Sounds like that name would better > discribe the intent (which is to notify that texture is no longer used by > the compositor, rather than the symptom (the ReleaseCompositableRef was > called down to a ref count of 0). I will update in a next patch.
Apply the comments except the leak check.
Attachment #8741207 - Attachment is obsolete: true
Correct patch.
Attachment #8745893 - Attachment is obsolete: true
See Also: 1227718 →
Is there some way we could combine what tiling does (gfxSharedReadLock), and what texture recycling does (SetRecycleCallback) to avoid duplication? Could we use locks for everything, and get rid of the callback system? Texture recycling could keep an array of allocated textures, and during allocation it can scan the list looking for one with a read lock count of 0. We also need a solution to deal with the compositor reading from texture memory after the TextureHost has been dropped (see bug 1260611). TiledContentHost/locking deals with this by holding on to the lock until the next composite for textures that don't have an intermediate buffer. This still isn't quite correct though, as BasicCompositor isn't deferred, so we're holding the lock longer than necessary in that case. One possible solution would be to let the Compositor know about locks (probably by having them available from the TextureSource), and have it grab a read lock if it needs to, and then be responsible for releasing it. A BufferTextureHost would release it's read lock during upload, and create a TextureSource with no lock. A D3D11 TextureHost would create a TextureSource referencing the same data, so would also share the same lock. The TextureHost would release it's lock when it's no longer attached to a compositable. CompositorD3D11 would grab an extra read lock each time a D3D11 texture source is drawn, and then release it again at the appropriate time.
(In reply to Matt Woodrow (:mattwoodrow) from comment #54) > Is there some way we could combine what tiling does (gfxSharedReadLock), and > what texture recycling does (SetRecycleCallback) to avoid duplication? I agree that there's de-duplication to do, and that something clever could be done with copy-on write locks like we do with tiling, but it's quite different from what sotaro is doing here which is a nice improvement of the current state of things, so I'd rather let sotaro finish this and then we figure something out along what you suggest after that. The London workweek could be a good time to settle for a good and unified design (one that could leverage the tile-style locks, wouldn't rely on fiddling with the texture's reference count, would not require a destruction callback, etc.). There's plenty of things to fix, unify and improve and I don't expect it to be a simple patch to write so i would like sotaro's current work to land in the mean time.
(In reply to Matt Woodrow (:mattwoodrow) from comment #54) > Is there some way we could combine what tiling does (gfxSharedReadLock), and > what texture recycling does (SetRecycleCallback) to avoid duplication? Yea, it is better to unify the recycling system to one. But I want to put off as a follow up bug. There are 3 ways of recycling. This bug is going to reduce to 2(tile and another). Recycling callback based "b2g recycling" was originally implemented to deliver fence object from compositor side to client side. If we want to use gfxSharedReadLock every where, we need to make clear how to handle fence object.
(In reply to Nicolas Silva [:nical] from comment #55) > (In reply to Matt Woodrow (:mattwoodrow) from comment #54) > The London workweek could be a good time to settle for a good and unified > design (one that could leverage the tile-style locks, wouldn't rely on > fiddling with the texture's reference count, would not require a destruction > callback, etc.). Yup, London workweek could be a good time.
(In reply to Sotaro Ikeda [:sotaro] from comment #56) > (In reply to Matt Woodrow (:mattwoodrow) from comment #54) > > Is there some way we could combine what tiling does (gfxSharedReadLock), and > > what texture recycling does (SetRecycleCallback) to avoid duplication? > > Yea, it is better to unify the recycling system to one. But I want to put > off as a follow up bug. There are another reason why callback style is used. In media framework, Image could be used my multiple users. Even when, host side releases it, a client side user still could use it and release timing could be any time.
No longer blocks: 1260611
URL: , 1260611
URL: , 1260611 → 1260611
Depends on: 1260611
URL: 1260611
Rebased and add comments.
Attachment #8745935 - Attachment is obsolete: true
Update comments.
Attachment #8750652 - Attachment is obsolete: true
Attachment #8750663 - Flags: feedback?(nical.bugzilla)
attachment 8750714 [details] [diff] [review] checks if TextureHosts are CompositableAddRefed correctly in ReceiveCompositableUpdate() by helper classes. If TextureHosts are not compositableAddRefed by the helper classes, it is detected by assert.
Attachment #8750714 - Flags: feedback?(nical.bugzilla)
Attachment #8750714 - Attachment description: patch - Check if CompositableRef works as expected → patch part 2 - Check if CompositableRef works as expected
Attachment #8750663 - Attachment description: patch - Make TextureHost recycling implicit → patch part 1 - Make TextureHost recycling implicit
Attachment #8750663 - Flags: feedback?(nical.bugzilla) → feedback+
Attachment #8751118 - Attachment description: patch - Keep video recycling workaround of bug 1260611 → patch part 3 - Keep video recycling workaround of bug 1260611
Rebased.
Attachment #8751118 - Attachment is obsolete: true
Add an additional sanity check.
Attachment #8750714 - Attachment is obsolete: true
Attachment #8750714 - Flags: feedback?(nical.bugzilla)
Update nits.
Attachment #8750663 - Attachment is obsolete: true
Attachment #8752038 - Flags: feedback?(nical.bugzilla)
Attachment #8752038 - Flags: feedback?(nical.bugzilla)
Fix build failure.
Attachment #8752038 - Attachment is obsolete: true
Attachment #8752061 - Flags: feedback?(nical.bugzilla)
Attachment #8751596 - Attachment is obsolete: true
Comment on attachment 8752061 [details] [diff] [review] patch part 2 - Check if CompositableRef works as expected Review of attachment 8752061 [details] [diff] [review]: ----------------------------------------------------------------- In my opinion this adds too much complexity for the sake of debug-only checks.
Attachment #8752061 - Flags: feedback?(nical.bugzilla) → feedback-
Attachment #8752038 - Attachment is obsolete: false
Attachment #8752061 - Attachment is obsolete: true
Comment on attachment 8752038 [details] [diff] [review] patch part 2 - Check if CompositableRef works as expected nical, how about this patch. It is previous one. I do not think it is too complex.
Attachment #8752038 - Flags: feedback?(nical.bugzilla)
Attachment #8752038 - Attachment is obsolete: true
Attachment #8752038 - Flags: feedback?(nical.bugzilla)
Attachment #8750714 - Attachment is obsolete: false
Attachment #8750714 - Flags: feedback?(nical.bugzilla)
(In reply to Sotaro Ikeda [:sotaro] from comment #71) > nical, how about this patch. It is previous one. I do not think it is too > complex. Sorry, Attachment 8750714 [details] [diff] is a patch that I am asking to feedback again.
nica, do you think that even Attachment 8750714 [details] [diff] is not necessary?
Comment on attachment 8750714 [details] [diff] [review] patch part 2 - Check if CompositableRef works as expected I am going to update the patch to simplify the check.
Attachment #8750714 - Flags: feedback?(nical.bugzilla)
Attachment #8750714 - Attachment is obsolete: true
By Bug 1264764, TextureParent does not hold CompositableParentManager any more. Instead, it holds just ISurfaceAllocator.
Depends on: 1274145
Updated a patch based on bug 1274145.
Attachment #8752053 - Attachment is obsolete: true
Simplified compared to previous one.
Rebased.
Attachment #8752063 - Attachment is obsolete: true
Comment on attachment 8754686 [details] [diff] [review] patch part 2 - Check if CompositableRef works as expected :nical, can you feedback again? The patch is simpler than the previous one, it does not do though sanity check though.
Attachment #8754686 - Flags: feedback?(nical.bugzilla)
Fix build failure.
Attachment #8754685 - Attachment is obsolete: true
Fix build failure.
Attachment #8755253 - Attachment is obsolete: true
Comment on attachment 8754686 [details] [diff] [review] patch part 2 - Check if CompositableRef works as expected Review of attachment 8754686 [details] [diff] [review]: ----------------------------------------------------------------- I am not convinced that this is very useful. If I understand correctly you want to add assertions to check that NotifyNotUsed is sent, so the code tracks that the extra CompositableRef stuff from the other patch is done, and the actual assertion is called by NotifyNotUsed itself. So instead of checking the result we care about (NotifyNotUsed is sent), we end up checking that some extra steps we take to make it happen was actually done. This is not that invasive so if you really think it's worth it go ahead, but in this case please add a comment that explains why we check this particular thing and what it means if this assertion is hit some day in the future.
Attachment #8754686 - Flags: feedback?(nical.bugzilla)
I think I am still missing something. The AutoTilesCompositableAddRef RAII goes through every tile in the array of TileDescriptors to make sure that AddCompositableRef is called for all TextureHosts in the array. Is that right, and is there something else that it also ensures ? Now looking at the code in UseTiles, the first step also goes through all of the tiles from the same array, creates TileHost objects which also calls AddCompositableRef. So we should have the exact same guarantee, right ? The only exception to this is if UseTiles early-returned false because (!aAllocator || !aCompositor) or if the resolution is invalid before the loop, but in these cases we will kill the child process anyway. So I don't understand what the RAII added in Part 1 gives us. Considering how subtle the CompositableRef stuff is I am not super found of touching it if we don't explicitly need to (TextureHost::BindTextureSource behaves differently if the count is 2 rather than 1, if the count is 2 when it could be 1 the behavior is correct but slower, which means it's hard to detect we are going down the slow path). Sorry for still questioning this logic after so many feedback rounds. I'd really like to understand the full picture because I am also writing code that interacts with this and I'd like to make sure everything stays compatible (and also because it's already pretty complex).
Flags: needinfo?(sotaro.ikeda.g)
Attachment #8754686 - Attachment is obsolete: true
(In reply to Nicolas Silva [:nical] from comment #83) > I think I am still missing something. The AutoTilesCompositableAddRef RAII > goes through every tile in the array of TileDescriptors to make sure that > AddCompositableRef is called for all TextureHosts in the array. Is that > right, and is there something else that it also ensures ? AutoTilesCompositableAddRef exist for 2 reason. - Make sure TextureHosts are always Compositable AddRefed in ReceiveCompositableUpdate() - Set last fwd transaction id of TextureHosts. > Now looking at the code in UseTiles, the first step also goes through all of > the tiles from the same array, creates TileHost objects which also calls > AddCompositableRef. So we should have the exact same guarantee, right ? > > The only exception to this is if UseTiles early-returned false because > (!aAllocator || !aCompositor) or if the resolution is invalid before the > loop, but in these cases we will kill the child process anyway. So I don't > understand what the RAII added in Part 1 gives us. Originally I implemented RAII to set last fwd transaction id in UseTiles() then moved to ReceiveCompositableUpdate() to remove recycling handing code from CompositableHost as much as possible. I do not wanted recycling code to depend on only CompositableHost's AddCompositableRef. It seems easily broken silently. For example, ImageHost via ImageBridge case, just after start up, there could be a case that ImageHost does not have Compositor during the transaction. And Compositor renewal might hit the condition. > Considering how subtle > the CompositableRef stuff is I am not super found of touching it if we don't > explicitly need to (TextureHost::BindTextureSource behaves differently if > the count is 2 rather than 1, if the count is 2 when it could be 1 the > behavior is correct but slower, which means it's hard to detect we are going > down the slow path). It is a good point! I am going to update the patch. > > Sorry for still questioning this logic after so many feedback rounds. I'd > really like to understand the full picture because I am also writing code > that interacts with this and I'd like to make sure everything stays > compatible (and also because it's already pretty complex). No problem about it :)
Flags: needinfo?(sotaro.ikeda.g)
Addressed "compositable ref == 2" problem in Comment 83.
Attachment #8755659 - Attachment is obsolete: true
Comment on attachment 8755670 [details] [diff] [review] patch part 2 - Check if CompositableRef works as expected :nical, how about the patch. It addressed "compositable ref == 2" problem.
Attachment #8755670 - Flags: feedback?(nical.bugzilla)
(In reply to Sotaro Ikeda [:sotaro] from comment #85) > (In reply to Nicolas Silva [:nical] from comment #83) > > I think I am still missing something. The AutoTilesCompositableAddRef RAII > > goes through every tile in the array of TileDescriptors to make sure that > > AddCompositableRef is called for all TextureHosts in the array. Is that > > right, and is there something else that it also ensures ? > > AutoTilesCompositableAddRef exist for 2 reason. > - Make sure TextureHosts are always Compositable AddRefed in > ReceiveCompositableUpdate() > - Set last fwd transaction id of TextureHosts. Ah! I forgot about this one. > > > Now looking at the code in UseTiles, the first step also goes through all of > > the tiles from the same array, creates TileHost objects which also calls > > AddCompositableRef. So we should have the exact same guarantee, right ? > > > > The only exception to this is if UseTiles early-returned false because > > (!aAllocator || !aCompositor) or if the resolution is invalid before the > > loop, but in these cases we will kill the child process anyway. So I don't > > understand what the RAII added in Part 1 gives us. > > Originally I implemented RAII to set last fwd transaction id in UseTiles() > then moved to ReceiveCompositableUpdate() to remove recycling handing code > from CompositableHost as much as possible. I am still not sure why we have to do the AddCompositableRef/ReleaseCOmpositableRef thing at all, because the loop inside of UseTiles already assigns all textures to CompositableTextureRefs slots. How about adding a loop that only does that without the AddRef/Release? If you want to assert that the texture was assigned to a CompositableTextureTextureRef slot and that it does not change, you could add something just after UseTiles that would do look like: > for (size_t i = 0; i < tileDesc.tiles().Length(); i++) { > const TileDescriptor& tileDesc = tileDesc.tiles()[i]; > RefPtr<TextureHost> tex = TextureHost::AsTextureHost(texturedDesc.textureParent(); > tex->SetFwdTransactionId(id); > // Make sure that each texture was handled by the compositable > // because the recycling logic depends on it. > MOZ_ASSERT(tex->NumCompositableRefs() > 0); > } I think that this loop achieves what you need by verifying that the compositable did not forget any texture and the CompositableRefs stuff is correct, as well as setting the id without needing to modify the CompositableRefs itself, and is also less code. By the way the texture's read locks used by tiling (and soon by other layer types) also rely on the fact that no texture has been forgotten by the compositable, so it is good to assert after the fact that the compositable did not forget any texture (better than re-touching every texture after the fact to be sure, because this will not fix the read-lock case). > > I do not wanted recycling code to depend on only CompositableHost's > AddCompositableRef. It seems easily broken silently. For example, ImageHost > via ImageBridge case, just after start up, there could be a case that > ImageHost does not have Compositor during the transaction. And Compositor > renewal might hit the condition. Makes sense. What do you think above the loop I proposed above? Would it solve the problems you are solving with the RAII mechanism?
Rebased.
Attachment #8755265 - Attachment is obsolete: true
> > > Now looking at the code in UseTiles, the first step also goes through all of > > > the tiles from the same array, creates TileHost objects which also calls > > > AddCompositableRef. So we should have the exact same guarantee, right ? > > > > > > The only exception to this is if UseTiles early-returned false because > > > (!aAllocator || !aCompositor) or if the resolution is invalid before the > > > loop, but in these cases we will kill the child process anyway. So I don't > > > understand what the RAII added in Part 1 gives us. > > > > Originally I implemented RAII to set last fwd transaction id in UseTiles() > > then moved to ReceiveCompositableUpdate() to remove recycling handing code > > from CompositableHost as much as possible. > > I am still not sure why we have to do the > AddCompositableRef/ReleaseCOmpositableRef thing at all, because the loop > inside of UseTiles already assigns all textures to CompositableTextureRefs > slots. It is because, RAII code implementation is transparent about inside of UseTiles. "(!aAllocator || !aCompositor)" exists now and might be changed in future. > How about adding a loop that only does that without the AddRef/Release? > If you want to assert that the texture was assigned to a > CompositableTextureTextureRef slot and that it does not change, you could > add something just after UseTiles that would do look like: > > > for (size_t i = 0; i < tileDesc.tiles().Length(); i++) { > > const TileDescriptor& tileDesc = tileDesc.tiles()[i]; > > RefPtr<TextureHost> tex = TextureHost::AsTextureHost(texturedDesc.textureParent(); > > tex->SetFwdTransactionId(id); > > // Make sure that each texture was handled by the compositable > > // because the recycling logic depends on it. > > MOZ_ASSERT(tex->NumCompositableRefs() > 0); > > } > > I think that this loop achieves what you need by verifying that the > compositable did not forget any texture and the CompositableRefs stuff is > correct, as well as setting the id without needing to modify the > CompositableRefs itself, and is also less code. > > By the way the texture's read locks used by tiling (and soon by other layer > types) also rely on the fact that no texture has been forgotten by the > compositable, so it is good to assert after the fact that the compositable > did not forget any texture (better than re-touching every texture after the > fact to be sure, because this will not fix the read-lock case). It is OK for me to remove AddCompositableRef and ReleaseCompositableRef from AutoTilesCompositableAddRef and insert assert. > > I do not wanted recycling code to depend on only CompositableHost's > > AddCompositableRef. It seems easily broken silently. For example, ImageHost > > via ImageBridge case, just after start up, there could be a case that > > ImageHost does not have Compositor during the transaction. And Compositor > > renewal might hit the condition. > > Makes sense. What do you think above the loop I proposed above? Would it > solve the problems you are solving with the RAII mechanism? It is OK for me. I am going to update the patch.
Attachment #8755670 - Attachment is obsolete: true
Attachment #8755670 - Flags: feedback?(nical.bugzilla)
Comment on attachment 8756180 [details] [diff] [review] patch part 2 - Check if CompositableRef works as expected nical, how about this patch?
Attachment #8756180 - Flags: feedback?(nical.bugzilla)
Comment on attachment 8756180 [details] [diff] [review] patch part 2 - Check if CompositableRef works as expected Review of attachment 8756180 [details] [diff] [review]: ----------------------------------------------------------------- I really think you should remove these RAII classes, they hide the logic more than anything and only protect us against someone in the future early-returning before calling Compositable->Use***, but the same person might as well early-return before the RAII guard causing the same issue anyway. ::: gfx/layers/composite/TextureHost.h @@ +603,5 @@ > PTextureParent* mActor; > TextureFlags mFlags; > int mCompositableCount; > uint64_t mFwdTransactionId; > + bool mIsTrackingForCompositableRef; We should not mIsTrackingForCompositableRef and the extra methods that come with it, since all we care about is asserting that all textures have been assigned to CompositableTextureRef slots. ::: gfx/layers/ipc/CompositableTransactionParent.cpp @@ +179,1 @@ > texture->ReleaseCompositableRef(); As far as I understand, this is not correct (not this patch particularly but the approach in general for timed textures). When an ImageHost receives a set of timed textures, only the most "current" is assigned to a CompositableTextureRef slot. If you add and release all texture's compositableRef, it means that all textures except the current one will be marked as recyclable, which should not be the case, because they will be used later (most probably) when their timestamp correspond to the compositor's timestamp. To fix this you could make the ImageHost's TimedImage hold a CompositableTextureHostRef insted of a RefPtr<TextureHost>. I think this would make sense. If you do this, it is very important that you make mCurrentTextureHost a RefPtr<TextureHost> instead of a CompositableTextureHostRef, because otherwise you'll end up with a compositableRef counter always equal to 2 for the current texture, which will go down the slow path every time. There should be no place where you have to do the manual Add/Release CompositableRefs (be it tiling or any other layer type), because if you do it manually, it means that the compositable has missed something and it is a bug on the compositable's side and should be fixed there, and/or you are marking as recyclable a texture that will be used later. @@ +220,1 @@ > mTextureHost->ReleaseCompositableRef(); Same here, if the texture was not assigned to a CompositableTextureRef slot, it's a bug in the CompositableHost and it will break plenty of other things. It's better to document that CompositableHost are supposed to do something, than add some extra code in case they don't do what they are supposed to do. Instead of 34 lines of RAII class, we can just add a single call to SetLastFwdTransactionId per texture and optionally an assertion about the NumCompositableTextureRefs if you are worried that bugs may be introduced in the CompositableHost (I personally think that a comment explaining why it is important would be more effective than the assertion, but asserting the right thing can't hurt).
Attachment #8756180 - Flags: feedback?(nical.bugzilla) → feedback-
(In reply to Nicolas Silva [:nical] from comment #93) > > Same here, if the texture was not assigned to a CompositableTextureRef slot, > it's a bug in the CompositableHost and it will break plenty of other things. > It's better to document that CompositableHost are supposed to do something, > than add some extra code in case they don't do what they are supposed to do. > Instead of 34 lines of RAII class, we can just add a single call to > SetLastFwdTransactionId per texture and optionally an assertion about the > NumCompositableTextureRefs if you are worried that bugs may be introduced in > the CompositableHost (I personally think that a comment explaining why it is > important would be more effective than the assertion, but asserting the > right thing can't hurt). The prerequisite about CompositableHost seems different than me. It is OK to remove the check and assigning CompositableTextureRef should be a responsibility of CompositableHost. But I am not sure about your comment of mentioning about how to do "assertion about the NumCompositableTextureRefs" without RAII.
Apply the comment except removing RAII.
Attachment #8756180 - Attachment is obsolete: true
Comment on attachment 8756670 [details] [diff] [review] patch part 2 - Check if CompositableRef works as expected :nical, can you feedback to the patch? I applied the comment except removing RAII, since your idea of removing RAII was not clear for me.
Attachment #8756670 - Flags: feedback?(nical.bugzilla)
Comment on attachment 8756670 [details] [diff] [review] patch part 2 - Check if CompositableRef works as expected Review of attachment 8756670 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for the confusing explanations about the RAII thing, Thanks for removing the manual Add/ReleaseCompositableRefs, though. It's not worth arguing forever about RAII vs explicit control flow, so if you feel strongly about the RAII class, go ahead. But please rename it to AutoSetFwdTransactionId. ::: gfx/layers/ipc/CompositableTransactionParent.cpp @@ +113,2 @@ > } > } By removing the RAII, I meant simply putting this loop directly after the line "bool success = tiledHost->UseTiledLayerBuffer(this, tileDesc);" in CompositableTransactionParent.cpp, just like you did for UseComponentAlphaTextures. Same thing for OpUseTexture. We don't need this to be triggered by an RAII guard, we just need it to run right after UseTiledLayerBuffer. RAII guards are great when you need something to happen at the end of a scope and the function can return from several places in that scope. But that comes at the cost of hiding what is happening since the destructor is called implicitly and not explicitly. In our case the control flow is simple, the function doesn't return half-way through and you only need the loop to run after the Use***() call (not specifically at the end of the scope). Seeing the sequence of instructions explcitly written where they are run makes it simpler to understand what the code does at a glance.
Attachment #8756670 - Flags: feedback?(nical.bugzilla) → feedback+
I understand the RAII part. I am going to remove it.
Attachment #8756670 - Attachment is obsolete: true
Rebased.
Attachment #8754691 - Attachment is obsolete: true
Attachment #8751593 - Flags: review?(nical.bugzilla)
Attachment #8756175 - Flags: review?(nical.bugzilla)
Attachment #8757742 - Flags: review?(nical.bugzilla)
Attachment #8757755 - Flags: review?(nical.bugzilla)
(In reply to Sotaro Ikeda [:sotaro] from comment #101) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=2f65ada16c0d&selectedJob=21646873 There is a crash of SharedSurface_IOSurface(). If we wait Bug 1167235, it might be addressed.
Attachment #8751593 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8756175 [details] [diff] [review] patch part 1 - Make TextureHost recycling implicit Review of attachment 8756175 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, could you fold part 2 into this patch ? ::: gfx/layers/client/TextureClient.h @@ +588,5 @@ > + void CancelWaitFenceHandleOnImageBridge(); > + > + /** > + * Set last transaction id of CompositableForwarder. > + * nit: trailing space ::: gfx/layers/ipc/ShadowLayers.cpp @@ +385,5 @@ > mSyncObject = SyncObject::CreateSyncObject(aIdentifier.mSyncHandle); > } > > +ShadowLayerForwarder::ShadowLayerForwarder(ClientLayerManager* aClientLayerManager) > + : mClientLayerManager(aClientLayerManager) nit: trailing space
Attachment #8756175 - Flags: review?(nical.bugzilla) → review+
Attachment #8757742 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8757755 [details] [diff] [review] patch part 3 - Keep video recycling workaround of bug 1260611 Review of attachment 8757755 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/composite/LayerManagerComposite.h @@ +399,5 @@ > > nsTArray<ImageCompositeNotification> mImageCompositeNotifications; > > + nsTArray<CompositableTextureHostRef> mCurrentHeldTextureHosts; > + nsTArray<CompositableTextureHostRef> mPreviousHeldTextureHosts; I am not found of this. for one, it doesn't interact well with the ReadLock patch queue (although it's not then end of the world if this is only used with TextureFlags::RECYCLE on ImageHost). More generally we should really use CompositableTextureRef only for what it was intended: counting the number of compositables that are using a given texture at any time. The original bug advertises this as a short-term fix. Do we have plans to fix the video flickering issue in a different way?
Depends on: 1272600
(In reply to Nicolas Silva [:nical] from comment #104) > > The original bug advertises this as a short-term fix. Do we have plans to > fix the video flickering issue in a different way? The patch is independent from bug 1272600. Based on bug 1272600, it is possible to implement. I am going to update the patch.
Attachment #8757755 - Flags: review?(nical.bugzilla)
Depends on: 1167235
No longer depends on: 1167235
See Also: → 1167235
Attachment #8760110 - Flags: review+
Rebased.
Attachment #8756175 - Attachment is obsolete: true
Attachment #8760115 - Flags: review+
Rebased.
Attachment #8757742 - Attachment is obsolete: true
Attachment #8760118 - Flags: review+
Rebased and apply the comment.
Attachment #8757755 - Attachment is obsolete: true
Update nits.
Attachment #8760136 - Attachment is obsolete: true
Fix build failure.
Attachment #8760137 - Attachment is obsolete: true
Attachment #8760152 - Attachment is obsolete: true
Attachment #8760532 - Attachment is obsolete: true
Attachment #8760676 - Flags: review?(nical.bugzilla)
Comment on attachment 8760676 [details] [diff] [review] patch part 3 - Keep video recycling workaround of bug 1260611 Review of attachment 8760676 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Wasn't there something about D3D11 textures as well? Or did the patch that blocks compositing Frame N until frame N-1 is finished take care of the D3D11 issue completely (I don't remember)? ::: gfx/layers/composite/LayerManagerComposite.cpp @@ +394,5 @@ > > if (mRoot && !(aFlags & END_NO_IMMEDIATE_REDRAW)) { > MOZ_ASSERT(!aTimeStamp.IsNull()); > UpdateAndRender(); > + mCompositor->FlushPendingNotifyNotUsed(); Wouldn't it be simpler to call this in Compositor::EndFrame?
Attachment #8760676 - Flags: review?(nical.bugzilla) → review+
(In reply to Nicolas Silva [:nical] from comment #115) > > > > if (mRoot && !(aFlags & END_NO_IMMEDIATE_REDRAW)) { > > MOZ_ASSERT(!aTimeStamp.IsNull()); > > UpdateAndRender(); > > + mCompositor->FlushPendingNotifyNotUsed(); > > Wouldn't it be simpler to call this in Compositor::EndFrame? attachment 8760676 [details] [diff] [review] should work mostly similar to bug 1260611. Compositor::EndFrame is called too early for FlushPendingNotifyNotUsed(). Then it was not used. In CompositorD3D11::EndFrame(), Compositor::EndFrame() is called in the beginning. "Previous frame complete waiting code" is in the bottom of CompositorD3D11::EndFrame(). https://dxr.mozilla.org/mozilla-central/source/gfx/layers/d3d11/CompositorD3D11.cpp#1297
(In reply to Nicolas Silva [:nical] from comment #115) > Comment on attachment 8760676 [details] [diff] [review] > patch part 3 - Keep video recycling workaround of bug 1260611 > > Review of attachment 8760676 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks good. Wasn't there something about D3D11 textures as well? Or did the > patch that blocks compositing Frame N until frame N-1 is finished take care > of the D3D11 issue completely (I don't remember)? > CompositorD3D11::EndFrame() has a code to wait previous frame. Therefore if FlushPendingNotifyNotUsed() is called after the EndFrame(), it means wait n-1 frame complete.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=79a68b6559f6&selectedJob=22111542 There is a test failure on mac that is caused by SharedSurface_IOSurface that seems alive longer than GL context. The problem might be addressed by waiting bug 1167235.
Revived CancelWaitForCompositorRecycle() as CancelWaitForRecycle() to shorten TextureClients lifetime. It is for fixing Comment 118.
Attachment #8762714 - Flags: review?(nical.bugzilla)
Attachment #8762714 - Flags: review?(nical.bugzilla) → review+
Pushed by sikeda@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5748a69df90b Make TextureHost recycling implicit r=nical
https://hg.mozilla.org/mozilla-central/rev/5748a69df90b
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
No longer blocks: 1281169
Depends on: 1281169
Sotaro, could this have caused crashes like this one: https://crash-stats.mozilla.com/report/index/99a61377-b65f-49e3-9718-7d7e82160820. Linux only, in 50 and 51. Tracked in bug 1298158.
Flags: needinfo?(sotaro.ikeda.g)
(In reply to Milan Sreckovic [:milan] from comment #124) > Sotaro, could this have caused crashes like this one: > https://crash-stats.mozilla.com/report/index/99a61377-b65f-49e3-9718- > 7d7e82160820. Linux only, in 50 and 51. Tracked in bug 1298158. I take a look. It might be related to Bug 1155000.
Flags: needinfo?(sotaro.ikeda.g)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: