Closed
Bug 1133007
Opened 9 years ago
Closed 9 years ago
(LayerScope) Prevent sending duplicate texture image
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: u459114, Assigned: u459114)
References
Details
Attachments
(2 files, 20 obsolete files)
LayerScope read image of every texture of every layer. 1. ReadPixel of each texture spends tons of time. 2. Sent the same image via websocket is really slow, and the viewer need to process all duplicate images, even slower... Prevent sending duplicate image should be a big perf win for layerscope
Attachment #8564302 -
Attachment is obsolete: true
Comment on attachment 8564303 [details] [diff] [review] Prevent sending duplicate texture images Review of attachment 8564303 [details] [diff] [review]: ----------------------------------------------------------------- Hi Dan, Please help reviewing this patch. By this change, we don't repeat sent the same image via webseocket to the viewer. Two benefits at device side 1. Reduce total amount of ReadPixel 2. Reduce socket traffic For viewer side, we don't need to hash image again. Instead, we can leverage texture.contentid as hash key. LayerScope will not reduce device FPS so much after this change.
Attachment #8564303 -
Flags: review?(dglastonbury)
Attachment #8564316 -
Flags: review?(mtseng)
Attachment #8564316 -
Flags: review?(boris.chiou)
Attachment #8564303 -
Flags: review?(dglastonbury) → review+
Attachment #8564303 -
Flags: review+ → review?(chung)
Updated•9 years ago
|
Attachment #8564316 -
Flags: review?(mtseng) → review+
Comment 5•9 years ago
|
||
Comment on attachment 8564303 [details] [diff] [review] Prevent sending duplicate texture images Review of attachment 8564303 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me but some part seems too complicated than needed. ::: gfx/layers/LayerScope.cpp @@ +350,4 @@ > > // Packet data > Packet mPacket; > + uint32_t mContentUUID; Why not use serial number on Texture directly? @@ +439,4 @@ > bool mComposedByHwc; > }; > > +class ImageContentCache { Given the serial number of TextureHost is strictly increasing, we only need to tracking the last available serial for each texture. In that case, a hash maps TextureSourceOGL* to uint32_t should be fine and effective than a linear search here. (This map should be a member of a debug session if we have debug session abstraction.) @@ +506,5 @@ > + > +private: > + static StaticAutoPtr<ImageContentCache> sContentCache; > + > + uint32_t mContentUUID; What is this UUID for?
Attachment #8564303 -
Flags: review?(chung) → review-
Attachment #8564303 -
Attachment is obsolete: true
Attachment #8564893 -
Flags: review?(chung)
Updated•9 years ago
|
Attachment #8564893 -
Flags: review?(chung) → review+
Comment 7•9 years ago
|
||
Comment on attachment 8564316 [details] viewer side change LGTM. Only https://github.com/CJKu/layerscope/pull/1/files#diff-e2874a901f7b01f94f61d645c612e572R318 needs to be fixed as we discussed offline.
Attachment #8564316 -
Flags: review?(boris.chiou) → review+
Full try since touched a low-level gfx class - TexturedEffect.. https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c1633edbdcd
Oop(In reply to C.J. Ku[:cjku] from comment #8) > Full try since touched a low-level gfx class - TexturedEffect.. > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c1633edbdcd Oops... wrong place
Assignee | ||
Comment 10•9 years ago
|
||
Assignee | ||
Comment 11•9 years ago
|
||
Attachment #8612370 -
Attachment is obsolete: true
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8612371 -
Attachment is obsolete: true
Assignee | ||
Comment 13•9 years ago
|
||
Comment on attachment 8612377 [details] [diff] [review] Prevent duplicate texture package Hi Morris, here is a patch depend on what we discussed yesterday. Please have a look, thanks
Attachment #8612377 -
Flags: review?(mtseng)
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8564316 -
Attachment is obsolete: true
Attachment #8564893 -
Attachment is obsolete: true
Attachment #8612393 -
Flags: review?(mtseng)
Attachment #8612393 -
Flags: review?(boris.chiou)
Attachment #8612377 -
Flags: review?(boris.chiou)
Assignee | ||
Comment 15•9 years ago
|
||
Assignee | ||
Comment 16•9 years ago
|
||
Attachment #8612406 -
Flags: review?(mtseng)
Attachment #8612404 -
Flags: review?(boris.chiou)
Attachment #8612377 -
Attachment is obsolete: true
Attachment #8612377 -
Flags: review?(mtseng)
Attachment #8612377 -
Flags: review?(boris.chiou)
Assignee | ||
Comment 17•9 years ago
|
||
Attachment #8612404 -
Attachment is obsolete: true
Attachment #8612404 -
Flags: review?(boris.chiou)
Attachment #8612417 -
Flags: review?(boris.chiou)
Assignee | ||
Comment 18•9 years ago
|
||
Attachment #8612406 -
Attachment is obsolete: true
Attachment #8612406 -
Flags: review?(mtseng)
Attachment #8612418 -
Flags: review?(mtseng)
Assignee | ||
Comment 19•9 years ago
|
||
Assignee | ||
Comment 20•9 years ago
|
||
Attachment #8612417 -
Attachment is obsolete: true
Attachment #8612418 -
Attachment is obsolete: true
Attachment #8612417 -
Flags: review?(boris.chiou)
Attachment #8612418 -
Flags: review?(mtseng)
Assignee | ||
Comment 21•9 years ago
|
||
Comment on attachment 8612699 [details] [diff] [review] Create a singlton holder to put all singleton in Review of attachment 8612699 [details] [diff] [review]: ----------------------------------------------------------------- Hi Dan/Boris, I add a new singleton object, access via Singletons::Instnace(), to hold all singletons object in layer scope. So, ideally, there should be only one singleton in LayerScope. To keep as less singleton as possible is the original thought.
Attachment #8612699 -
Flags: review?(dglastonbury)
Attachment #8612699 -
Flags: review?(boris.chiou)
Assignee | ||
Comment 22•9 years ago
|
||
Comment on attachment 8612700 [details] [diff] [review] Send texture content only if it was altered Review of attachment 8612700 [details] [diff] [review]: ----------------------------------------------------------------- Hi Dan/Morris, On B2G, Since GrallocTextureHost is not a DataTextureSource, Attachment 8564893 [details] [diff] is not really useful on B2G. In this new patch, TextureHost will notify LayerScope that content been updated in TextureHost::Updated. So that LayerScope knows it need to re-send the content of that texture again since it's altered. The FPS boosts up from 1FPS to 30FPS after apply this change. Please help to review.
Attachment #8612700 -
Flags: review?(mtseng)
Attachment #8612700 -
Flags: review?(dglastonbury)
Comment 23•9 years ago
|
||
Comment on attachment 8612699 [details] [diff] [review] Create a singlton holder to put all singleton in Review of attachment 8612699 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/LayerScope.cpp @@ +193,5 @@ > + gfx::Rect mLayerRects[4]; > +}; > + > +// Hold all singleton objects used by LayerScope > +class Singletons I think we can use a better class name, ex. LayerScopeManager or others. @@ +242,2 @@ > { > return sWebSocketManager; Redundant code @@ +254,3 @@ > private: > + // Hide constructor. > + Singletons(){ } You can use "Singleton() = delete;" @@ +935,4 @@ > } > > // Transfer ownership to SocketManager. > + Singletons::Instance()->GetSocketManager()->AppendDebugData(package.release()); indent @@ +1645,5 @@ > new DebugGLMetaData(Packet::META, true)); > } > } > > +void typo ?
Comment 24•9 years ago
|
||
Comment on attachment 8612699 [details] [diff] [review] Create a singlton holder to put all singleton in Review of attachment 8612699 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/LayerScope.cpp @@ +935,4 @@ > } > > // Transfer ownership to SocketManager. > + Singletons::Instance()->GetSocketManager()->AppendDebugData(package.release()); Sorry. Please ignore this comment.
Comment 25•9 years ago
|
||
Comment on attachment 8612699 [details] [diff] [review] Create a singlton holder to put all singleton in Review of attachment 8612699 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/LayerScope.cpp @@ +254,3 @@ > private: > + // Hide constructor. > + Singletons(){ } Sorry. You still need this constructor, so please ignore this comment.
Assignee | ||
Comment 26•9 years ago
|
||
Attachment #8612699 -
Attachment is obsolete: true
Attachment #8612699 -
Flags: review?(dglastonbury)
Attachment #8612699 -
Flags: review?(boris.chiou)
Assignee | ||
Comment 27•9 years ago
|
||
Comment on attachment 8612818 [details] [diff] [review] Create a singlton holder to put all singleton objects in Review of attachment 8612818 [details] [diff] [review]: ----------------------------------------------------------------- Fixed what boris mentioned
Attachment #8612818 -
Flags: review?(dglastonbury)
Attachment #8612818 -
Flags: review?(boris.chiou)
Assignee | ||
Comment 28•9 years ago
|
||
rebase
Attachment #8612700 -
Attachment is obsolete: true
Attachment #8612700 -
Flags: review?(mtseng)
Attachment #8612700 -
Flags: review?(dglastonbury)
Attachment #8612823 -
Flags: review?(dglastonbury)
Attachment #8612823 -
Flags: review?(boris.chiou)
Attachment #8612823 -
Flags: review?(boris.chiou) → review?(mtseng)
Comment 29•9 years ago
|
||
Comment on attachment 8612818 [details] [diff] [review] Create a singlton holder to put all singleton objects in Review of attachment 8612818 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/LayerScope.cpp @@ +181,5 @@ > +class DrawSession { > +public: > + DrawSession() > + : mOffsetX(0.0), > + mOffsetY(0.0), nit: Format like this: DrawSession() : mOffsetX(0.0) , mOffsetY(0.0) , mRects(0) { } https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes @@ +198,5 @@ > { > public: > + static LayerScopeManager* Instance() { > + if (!sLayerScopeManager) { > + sLayerScopeManager = new LayerScopeManager(); The problem with this style of singleton is that we lose the ability to control when to deallocate the LayerScopeManager object. Is a static instance method really any better than having a global object `gLayerScopeManager`? (I don't think it is) @@ +301,5 @@ > > uint32_t size = aPacket.ByteSize(); > auto data = MakeUnique<uint8_t[]>(size); > aPacket.SerializeToArray(data.get(), size); > + return LayerScopeManager::Instance()->GetSocketManager()->WriteAll(data.get(), size); Stylistically, I really dislike this list of pointer derefs. @@ +646,4 @@ > return NS_OK; > > printf_stderr("*** LayerScope: Accepted connection\n"); > + LayerScopeManager::Instance()->GetSocketManager()->AddConnection(aTransport); Ditto.
Comment 30•9 years ago
|
||
Comment on attachment 8612823 [details] [diff] [review] Send texture content only if it was altered Review of attachment 8612823 [details] [diff] [review]: ----------------------------------------------------------------- r+ with nits fixed. ::: gfx/layers/LayerScope.cpp @@ +203,5 @@ > + mChangedHosts.AppendElement(host); > + } > + } > + > + void Clear(const TextureHost* host) { Clear sounds too much like Empty. Or rename using something more descriptive, such `RemoveChangedHost`. @@ +210,5 @@ > + } > + } > + > + bool IsChanged(const TextureHost* host) { > + if (THArray::NoIndex == mAllHosts.IndexOf(host)) { Is the point of AllHosts array that all texture hosts are marked as changed at least one? If so, I suggest changing `All` for a less generic name. Perhaps `mSeenHosts`. Also add a comment explaining why `IsChanged` returns true if hosts is not in the `mSeenHosts` array. @@ +215,5 @@ > + mAllHosts.AppendElement(host); > + return true; > + } > + > + if (decltype(mChangedHosts)::NoIndex != mChangedHosts.IndexOf(host)) { In the previous if, you used `THArray::NoIndex` and in this one `decltype(mChangedHosts)::NoIndex`. Please be consistent. Also the if and return false can be eliminated: return mChangedHosts.IndexOf(host) != THArray::NoIndex; @@ +436,5 @@ > + if (packData) { > + uint8_t* grallocData = nullptr; > + if (BAD_VALUE == buffer->lock(GRALLOC_USAGE_SW_READ_OFTEN | > + GRALLOC_USAGE_SW_WRITE_NEVER, > + reinterpret_cast<void**>(&grallocData))) { When in if statement has multiple lines, place the { at the start of the block, instead on on the end-of-line: if (BAD_VALUE == buffer->lock(GRALLOC_USAGE_SW_READ_OFTEN | GRALLOC_USAGE_SW_WRITE_NEVER, reinterpret_cast<void**>(&grallocData))) { return false; } If the condition fits on a single-line, keep the { at the end: if (simple) { return true; } ::: gfx/layers/LayerScope.h @@ +44,1 @@ > private: Empty line before `private:`
Attachment #8612823 -
Flags: review?(dglastonbury) → review+
Assignee | ||
Comment 31•9 years ago
|
||
Attachment #8612818 -
Attachment is obsolete: true
Attachment #8612823 -
Attachment is obsolete: true
Attachment #8612818 -
Flags: review?(dglastonbury)
Attachment #8612818 -
Flags: review?(boris.chiou)
Attachment #8612823 -
Flags: review?(mtseng)
Assignee | ||
Comment 32•9 years ago
|
||
Attachment #8613356 -
Flags: review+
Updated•9 years ago
|
Attachment #8612393 -
Flags: review?(boris.chiou) → review+
Assignee | ||
Comment 33•9 years ago
|
||
Hi Dan, This patch fixed problems that you mentioned in the review, please review again, thank.
Attachment #8613355 -
Attachment is obsolete: true
Attachment #8613373 -
Flags: review?(dglastonbury)
Attachment #8613373 -
Flags: review?(boris.chiou)
Assignee | ||
Comment 34•9 years ago
|
||
Attachment #8613356 -
Attachment is obsolete: true
Attachment #8613375 -
Flags: review+
Comment 35•9 years ago
|
||
Comment on attachment 8613373 [details] [diff] [review] Create a singlton holder to put all singleton in Review of attachment 8613373 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. Just wait for Dan's review
Attachment #8613373 -
Flags: review?(boris.chiou) → review+
Assignee | ||
Comment 36•9 years ago
|
||
Merged two patches and submitted a try Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=79d2257be5a3
Assignee | ||
Comment 37•9 years ago
|
||
Attachment #8613375 -
Attachment is obsolete: true
Attachment #8613461 -
Flags: review+
Assignee | ||
Comment 38•9 years ago
|
||
Fix build break on windows and push to try again. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b892ab13bd7b
Comment 39•9 years ago
|
||
Comment on attachment 8613373 [details] [diff] [review] Create a singlton holder to put all singleton in Review of attachment 8613373 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.
Attachment #8613373 -
Flags: review?(dglastonbury) → review+
Assignee | ||
Comment 40•9 years ago
|
||
Combine two patches.
Attachment #8613373 -
Attachment is obsolete: true
Attachment #8613461 -
Attachment is obsolete: true
Attachment #8614025 -
Flags: review+
Keywords: checkin-needed
Comment 41•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f86c710c4074
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f86c710c4074 https://hg.mozilla.org/mozilla-central/rev/13bb3c8762e3
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Updated•9 years ago
|
Attachment #8612393 -
Flags: review?(mtseng)
Blocks: LayerScope
No longer depends on: LayerScope
You need to log in
before you can comment on or make changes to this bug.
Description
•