Closed
Bug 975346
Opened 7 years ago
Closed 7 years ago
[LayerScope] LayerScope is not work while HWC is chosen
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: u459114, Assigned: boris)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 25 obsolete files)
26.33 KB,
patch
|
djg
:
review+
jerry
:
feedback+
|
Details | Diff | Splinter Review |
33.80 KB,
patch
|
boris
:
review+
|
Details | Diff | Splinter Review |
Since we dump image in DrawQuad, LayerScope can not dump image correctly while HWC is chosen. I think we should either make it work or always enable GLCompositor while there is a viewer attached.
Blocks: LayerScope
Assignee | ||
Comment 1•7 years ago
|
||
As we know, LayerScope Tool needs "GLContext" and "EffectChain". However, after the discussion with Morris this morning, we don't need "GLContext" on the view tool now, so I will set the address to 0 by default in other compositors, such as HwcComposer2D, BasicCompositor, CompositorD3D9, and CompositorD3D11.
Assignee | ||
Comment 2•7 years ago
|
||
Sorry, I should correct my comment. All the compositors will set "GLContext" to 0 by default.
Assignee | ||
Comment 3•7 years ago
|
||
Now, I can implement a general function, GenEffectChain(), in: 1. Image Layer 2. Thebes Layer 3. Color Layer 4. Canvas Layer to generate the correct EffectChain for LayerScope tool, and also another general function, GenEffect(), which can generate a primary effect according to the different CompositableHosts in: 1. ImageHost, Deprecated ImageHost 2. ContentHostBase, Deprecated ContentHostBase BTW, according to the discussion with Morris last week, we still need GLContext for HwcComposer2D, so I am going to trying to investigate how to collect layer data (to retrieve GLContext for HwcComposer2D), handle TextureHost lock/unlock problems, and generate the correct EffectChains from the generation functions above.
Assignee | ||
Updated•7 years ago
|
Assignee: mtseng → boris.chiou
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Comment 6•7 years ago
|
||
Part 1: Implement general function, GetEffectChain(). 1. Add GetEffectChain() for (Image|Thebes|Color|Canvas)LayerComposite. 2. Add an auto lock mechanism for CompositibleHost. Now we can lock the TextureHost in ImageHost or ContentHost by the autolock. 3. For each CompositibleHost, add GenEffect() to generate the primary effects for each host.
Attachment #8408849 -
Attachment is obsolete: true
Comment hidden (obsolete) |
Assignee | ||
Comment 8•7 years ago
|
||
Part 2: Support LayerScope for Hardware Composer 1. This design still uses GLContext to read texture source for HwcComposer2D. 2. Use auto begin/end frame to handle the frame stamp.
Attachment #8408863 -
Attachment is obsolete: true
Comment on attachment 8409558 [details] [diff] [review] Bug-975346-Support-LayerScope-for-HwcComposer2D.patch Review of attachment 8409558 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/LayerScope.cpp @@ +735,3 @@ > > gLayerScopeWebSocketManager->AppendDebugData( > new DebugGLTextureData(aGLContext, aLayerRef, textureTarget, if (aGLContext) { RefPtr<DataSourceSurface> img = aGLContext->ReadTexImageHelper()->ReadTexImage(0, textureTarget, size, shaderConfig, aFlipY); gLayerScopeWebSocketManager->AppendDebugData( new DebugGLTextureData(aGLContext, aLayerRef, textureTarget, textureId, img)); } else { // What happens here? } ::: gfx/layers/LayerScope.h @@ +33,5 @@ > +// Perfoem BeginFrame and EndFrame automatically > +class LayerScopeAutoFrame { > +public: > + LayerScopeAutoFrame(int64_t aFrameStamp); > + virtual ~LayerScopeAutoFrame(); No need to be virtual And this new class is cool
Assignee | ||
Comment 10•7 years ago
|
||
feedback |
Thanks for your review, CJ. I should discuss the "else" part with Morris. BTW, I am trying to retrieve raw texture data from ANativeWindowBuffer (from LayerRenderState), instead of GLContext. If this method can work, I will revise the APIs in LayerScope to handle the specific Hardware Composer case. (In reply to C.J. Ku[:CJKu] from comment #9) > Comment on attachment 8409558 [details] [diff] [review] > Bug-975346-Support-LayerScope-for-HwcComposer2D.patch > > Review of attachment 8409558 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/LayerScope.cpp > @@ +735,3 @@ > > > > gLayerScopeWebSocketManager->AppendDebugData( > > new DebugGLTextureData(aGLContext, aLayerRef, textureTarget, > > if (aGLContext) { > RefPtr<DataSourceSurface> img = > aGLContext->ReadTexImageHelper()->ReadTexImage(0, textureTarget, size, > > shaderConfig, aFlipY); > > gLayerScopeWebSocketManager->AppendDebugData( > new DebugGLTextureData(aGLContext, aLayerRef, textureTarget, > textureId, img)); > } > else { > // What happens here? > } > > ::: gfx/layers/LayerScope.h > @@ +33,5 @@ > > +// Perfoem BeginFrame and EndFrame automatically > > +class LayerScopeAutoFrame { > > +public: > > + LayerScopeAutoFrame(int64_t aFrameStamp); > > + virtual ~LayerScopeAutoFrame(); > > No need to be virtual > > And this new class is cool
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Comment 19•7 years ago
|
||
Hi, CJ, Could you please give me some early feedback for patch 2? Thanks.
Attachment #8417919 -
Attachment is obsolete: true
Attachment #8418662 -
Flags: feedback?(cku)
Reporter | ||
Comment 20•7 years ago
|
||
feedback |
Comment on attachment 8418662 [details] [diff] [review] [WIP] Part 2: Support LayerScope for HwcComposer2D Review of attachment 8418662 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/LayerScope.cpp @@ +464,5 @@ > return true; > } > > +private: > + void packing(RefPtr<DataSourceSurface> aImage) { no need to use smart pointer here. void packing(DataSourceSurface *aImage) @@ +663,5 @@ > + mGraphicBuffer->unlock(); > + } > + } > + > + bool Failed() const { return !mSucceed; } Do we really need this function? Can we use return value of GetBuffer to verify lock buffer result? For exp if (!buffer.GetBuffer()) return false; We can remove a data member and a function be this change @@ +664,5 @@ > + } > + } > + > + bool Failed() const { return !mSucceed; } > + void* GetBuffer() { return mBuffer; } This one can be const as well @@ +668,5 @@ > + void* GetBuffer() { return mBuffer; } > + > +private: > + AutoLockGraphicBuffer() > + : mGraphicBuffer(nullptr), mGraphicBuffer(nullptr) << no need. It's an object, not built-in c pointer type @@ +751,2 @@ > aSource->GetFormat()); > int shaderConfig = config.mFeatures; Since aGLContext should not be null. If that happen, we will see a blank layer on the viewer. in the beginning of SendTextureSource(...) MOZ_ASSERT(aGLContext); if (aGLContext) return; @@ +954,5 @@ > +} > + > +void > +LayerScope::SendLayerData(LayerComposite* aLayer) > +{ MOZ_ASSERT(aLayer && aLayer->GetLayer()); @@ +973,5 @@ > + // Handle RGBAX format > + SendRawDataSource(aLayer->GetLayer()); > + break; > + case gfx::SurfaceFormat::R5G6B5: > + // TODO: 16bit High color case break @@ +975,5 @@ > + break; > + case gfx::SurfaceFormat::R5G6B5: > + // TODO: 16bit High color case > + case gfx::SurfaceFormat::YUV: > + // TODO: YUV case break @@ +977,5 @@ > + // TODO: 16bit High color case > + case gfx::SurfaceFormat::YUV: > + // TODO: YUV case > + case gfx::SurfaceFormat::UNKNOWN: > + default: { default: { ::: gfx/layers/LayerScope.h @@ +28,5 @@ > static void SendEffectChain(gl::GLContext* aGLContext, > const EffectChain& aEffectChain, > int aWidth, int aHeight); > + static void SendLayerData(LayerComposite* aLayer); > + static bool CheckSender(); Why we need this be a public function?
Reporter | ||
Comment 21•7 years ago
|
||
feedback |
Comment on attachment 8418662 [details] [diff] [review] [WIP] Part 2: Support LayerScope for HwcComposer2D Review of attachment 8418662 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/LayerScope.cpp @@ +926,5 @@ > + const gfx::SurfaceFormat format = ConvertSurfaceFormat(andoridFormat); > + const int32_t stride = graphicBuffer->getStride() * BytesPerPixel(format); > + const gfx::IntSize size(graphicBuffer->getWidth(), > + graphicBuffer->getHeight()); > + MOZ_ASSERT(!size.IsEmpty() && !stride); @@ +997,5 @@ > + } > +} > + > +bool > +LayerScope::CheckSender() Should we call this function at the begin of all SendXXX functions? ::: gfx/layers/LayerScope.h @@ +23,5 @@ > public: > static void CreateServerSocket(); > static void DestroyServerSocket(); > static void BeginFrame(gl::GLContext* aGLContext, int64_t aFrameStamp); > static void EndFrame(gl::GLContext* aGLContext); Since you add LayerScopeAutoFrame, suggest to move these two functions out of header.
Reporter | ||
Comment 22•7 years ago
|
||
feedback |
Comment on attachment 8418662 [details] [diff] [review] [WIP] Part 2: Support LayerScope for HwcComposer2D Review of attachment 8418662 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/gonk/HwcComposer2D.cpp @@ +795,5 @@ > +{ > + if (!LayerScope::CheckSender()) { > + return; > + } > + Can we do check in SendLayerData? @@ +816,5 @@ > + break; > + case Layer::TYPE_CONTAINER: > + default: > + break; > + } Can we move this switch into LayerScope::SendLayerData?
Attachment #8418662 -
Flags: feedback?(cku) → feedback+
Assignee | ||
Comment 23•7 years ago
|
||
feedback |
Comment on attachment 8418662 [details] [diff] [review] [WIP] Part 2: Support LayerScope for HwcComposer2D Review of attachment 8418662 [details] [diff] [review]: ----------------------------------------------------------------- Hi CJ, Thanks for your feedback. As per our discussion earlier, I will refactor some parts of LayerScope to make the structure of "SendXXX()" clearer, and then upload a new patch. ::: gfx/layers/LayerScope.cpp @@ +464,5 @@ > return true; > } > > +private: > + void packing(RefPtr<DataSourceSurface> aImage) { Got it @@ +663,5 @@ > + mGraphicBuffer->unlock(); > + } > + } > + > + bool Failed() const { return !mSucceed; } I should check whether buffer is null or not when lock has errors. It's a good idea. @@ +664,5 @@ > + } > + } > + > + bool Failed() const { return !mSucceed; } > + void* GetBuffer() { return mBuffer; } Got it @@ +668,5 @@ > + void* GetBuffer() { return mBuffer; } > + > +private: > + AutoLockGraphicBuffer() > + : mGraphicBuffer(nullptr), OK! @@ +751,2 @@ > aSource->GetFormat()); > int shaderConfig = config.mFeatures; OK. @@ +926,5 @@ > + const gfx::SurfaceFormat format = ConvertSurfaceFormat(andoridFormat); > + const int32_t stride = graphicBuffer->getStride() * BytesPerPixel(format); > + const gfx::IntSize size(graphicBuffer->getWidth(), > + graphicBuffer->getHeight()); > + OK @@ +954,5 @@ > +} > + > +void > +LayerScope::SendLayerData(LayerComposite* aLayer) > +{ Got it @@ +973,5 @@ > + // Handle RGBAX format > + SendRawDataSource(aLayer->GetLayer()); > + break; > + case gfx::SurfaceFormat::R5G6B5: > + // TODO: 16bit High color case Sorry, I should add a comment, "fall through". @@ +977,5 @@ > + // TODO: 16bit High color case > + case gfx::SurfaceFormat::YUV: > + // TODO: YUV case > + case gfx::SurfaceFormat::UNKNOWN: > + default: { I move the braces here according to the controll structure of coding style in MDN: switch (...) { case 1: { // When you need to declare a variable in a switch, put the block in braces int var; break; } case 2: ... break; default: break; } @@ +997,5 @@ > + } > +} > + > +bool > +LayerScope::CheckSender() As per our discussion earlier, I will re-position all the CheckSender(). Thanks. ::: gfx/layers/LayerScope.h @@ +23,5 @@ > public: > static void CreateServerSocket(); > static void DestroyServerSocket(); > static void BeginFrame(gl::GLContext* aGLContext, int64_t aFrameStamp); > static void EndFrame(gl::GLContext* aGLContext); Good idea. ::: widget/gonk/HwcComposer2D.cpp @@ +795,5 @@ > +{ > + if (!LayerScope::CheckSender()) { > + return; > + } > + In order to prevent the for loop, I should add a check here. However, I will change the function name to avoid our confusion. @@ +816,5 @@ > + break; > + case Layer::TYPE_CONTAINER: > + default: > + break; > + } Of course
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Updated•7 years ago
|
Attachment #8420842 -
Flags: feedback?(hshih)
Attachment #8420842 -
Flags: feedback?(cku)
Assignee | ||
Updated•7 years ago
|
Attachment #8418653 -
Flags: feedback?(hshih)
Reporter | ||
Comment 28•7 years ago
|
||
feedback |
Comment on attachment 8420842 [details] [diff] [review] [WIP] Part 2: Support LayerScope for HwcComposer2D Review of attachment 8420842 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/LayerScope.cpp @@ +714,5 @@ > + * (Other format) | (RGBA) |-> SendTextureSource --| > + * --> SendYCbCrEffect ----- | > + * | (YUV) | > + * --> SendColor ----------------------------------- > + * (Solid color) Make this diagram simpler. Our code change rapidly, keep it simpler make less effort while change need. * SendRawData ** RGBA format * SendXX ** XXXX *** XXX @@ +724,5 @@ > + *****************************************************************************/ > +class SenderHelper { > +// Sender public APIs > +public: > + static void SendLayerHelp(LayerComposite* aLayer); No need "help" post-fix @@ +753,5 @@ > +private: > + static gfx::SurfaceFormat ConvertSurfaceFormat(android::PixelFormat aFormat, > + bool aSwapRB = false); > + static GLenum ConvertTextureTarget(android::PixelFormat aFormat); > +}; This class looks great! ::: widget/xpwidgets/nsBaseWidget.cpp @@ +894,5 @@ > return; > } > > + // Initialize LayerScope on the main thread. > + LayerScope::Init(); Can this function become lazy Init one?
Attachment #8420842 -
Flags: feedback?(cku) → feedback+
Reporter | ||
Comment 29•7 years ago
|
||
feedback |
Comment on attachment 8420842 [details] [diff] [review] [WIP] Part 2: Support LayerScope for HwcComposer2D Review of attachment 8420842 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/LayerScope.cpp @@ +343,5 @@ > + gLayerScopeWebSocketManager->RemoveAllConnections(); > + } > + } > +}; > + If you want to have a new class for socket creation and destroy... please also add a function to access gLayerScopeWebSocketManager and make it be a private static member of this class public LayerScopeWebSocketManager *GetSocketManager(); private: static StaticAutoPtr<LayerScopeWebSocketManager> mWebSocketManager;
Reporter | ||
Comment 30•7 years ago
|
||
feedback |
Comment on attachment 8420842 [details] [diff] [review] [WIP] Part 2: Support LayerScope for HwcComposer2D Review of attachment 8420842 [details] [diff] [review]: ----------------------------------------------------------------- Not relative to change in this patch. But, do you know why DebugDataSender::mList need to be a pointer instead of an object? If no special reason existed, suggest change it to an object member
Assignee | ||
Comment 31•7 years ago
|
||
feedback |
According to the implementation, I think we can use a LinkedList object here. (In reply to C.J. Ku[:cjku] from comment #30) > Comment on attachment 8420842 [details] [diff] [review] > [WIP] Part 2: Support LayerScope for HwcComposer2D > > Review of attachment 8420842 [details] [diff] [review]: > ----------------------------------------------------------------- > > Not relative to change in this patch. But, do you know why > DebugDataSender::mList need to be a pointer instead of an object? If no > special reason existed, suggest change it to an object member
Assignee | ||
Comment 32•7 years ago
|
||
feedback |
Comment on attachment 8420842 [details] [diff] [review] [WIP] Part 2: Support LayerScope for HwcComposer2D Review of attachment 8420842 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/LayerScope.cpp @@ +343,5 @@ > + gLayerScopeWebSocketManager->RemoveAllConnections(); > + } > + } > +}; > + OK, I should also wrap this static variable. @@ +714,5 @@ > + * (Other format) | (RGBA) |-> SendTextureSource --| > + * --> SendYCbCrEffect ----- | > + * | (YUV) | > + * --> SendColor ----------------------------------- > + * (Solid color) OK. Just keep it simpler. @@ +724,5 @@ > + *****************************************************************************/ > +class SenderHelper { > +// Sender public APIs > +public: > + static void SendLayerHelp(LayerComposite* aLayer); OK. ::: widget/xpwidgets/nsBaseWidget.cpp @@ +894,5 @@ > return; > } > > + // Initialize LayerScope on the main thread. > + LayerScope::Init(); Good question. I should discuss this with Morris.
Assignee | ||
Comment 33•7 years ago
|
||
feedback |
Comment on attachment 8420842 [details] [diff] [review] [WIP] Part 2: Support LayerScope for HwcComposer2D Review of attachment 8420842 [details] [diff] [review]: ----------------------------------------------------------------- ::: widget/xpwidgets/nsBaseWidget.cpp @@ +894,5 @@ > return; > } > > + // Initialize LayerScope on the main thread. > + LayerScope::Init(); It may become lazy init one, but I think we shouldn't revise its position in the patch. If the Init() does more things in the future, I will try to change it. Thanks.
Assignee | ||
Comment 34•7 years ago
|
||
Updated: Actually, I got some gen lock failed issues after using lock/unlock of graphicBuffer in HwcComposer. ex. ContentClientDoubleBuffered::FinializeFrame() { DebugOnly<bool> locked = mTextureClient->Lock(Openmode::Open_READ_WRITE); MOZ_ASSERT(locked); // <-- assertion failure } errors: > libgenlock: perform_lock_unlock_operation: GENLOCK_IOC_DEADLOCK failed (lockType0x1, err=Connection timed out fd=125) > msm7627a.gralloc: gralloc_lock: genlock_lock_buffer (lockType=0x2) failed > GraphicBufferMapper: lock(...) failed -22 (Invalid argument) > MOZ_ASSERT: Assert: Assertion failure: locked, at /.../gecko/gfx/layers/client/ContentClient.cpp:451 Sotaro mentioned this problem in Bug 898919 Comment 44 and Bug 912134 Comment 18. According to these comments: When using gralloc buffers, the user of the buffers need to grab genlock. OpenGL rendering and HwCompser rendering could switch dynamically in each rendering. If HwComposer or OpenGL continue to keep gralloc buffer's genlock, another one can not get genlock and wait until got the genlock is freed. If the wait time is longer than genlock timeout, "genlock failure" log printed to logcat. I wrote this genlock owner ship competiion as "gralloc usage conflicts". This "gralloc usage conflicts" has 2 patterns. -(1) multiple user try to use gralloc buffers simultaneously. -(2) genlock is not released after user. I indeed unlock each graphic buffer after locking it, so I think it's the pattern (1). In my unagi, OpenGL and HwcComposer switch often, and I usually lock/unlock the graphic buffer in HwcComposer, so it's easy to get a gen lock failed error. In the leo, the frequency of switch between OpenGL and HwcComposer is not very high, so it's not easy to reproduce the gen lock failed. (Actually, I only got one in these two days.)
Comment 35•7 years ago
|
||
I recently update a way of gralloc buffer binding by Bug 1005908. It seem to simplify the binding problem except tiling layer.
Comment 36•7 years ago
|
||
(In reply to Boris Chiou [:boris] from comment #34) > Updated: > > Actually, I got some gen lock failed issues after using lock/unlock of > graphicBuffer in HwcComposer. > ex. > ContentClientDoubleBuffered::FinializeFrame() > { > DebugOnly<bool> locked = mTextureClient->Lock(Openmode::Open_READ_WRITE); > MOZ_ASSERT(locked); // <-- assertion failure > } > errors: > > libgenlock: perform_lock_unlock_operation: GENLOCK_IOC_DEADLOCK failed (lockType0x1, err=Connection timed out fd=125) > > msm7627a.gralloc: gralloc_lock: genlock_lock_buffer (lockType=0x2) failed > > GraphicBufferMapper: lock(...) failed -22 (Invalid argument) > > MOZ_ASSERT: Assert: Assertion failure: locked, at /.../gecko/gfx/layers/client/ContentClient.cpp:451 > > Sotaro mentioned this problem in Bug 898919 Comment 44 and Bug 912134 > Comment 18. > > According to these comments: > When using gralloc buffers, the user of the buffers need to grab genlock. > OpenGL rendering and HwCompser rendering could switch dynamically in each > rendering. If HwComposer or OpenGL continue to keep gralloc buffer's > genlock, another one can not get genlock and wait until got the genlock is > freed. If the wait time is longer than genlock timeout, "genlock failure" > log printed to logcat. I wrote this genlock owner ship competiion as > "gralloc usage conflicts". > > This "gralloc usage conflicts" has 2 patterns. > -(1) multiple user try to use gralloc buffers simultaneously. > -(2) genlock is not released after user. > > I indeed unlock each graphic buffer after locking it, so I think it's the > pattern (1). In my unagi, OpenGL and HwcComposer switch often, and I usually > lock/unlock the graphic buffer in HwcComposer, so it's easy to get a gen > lock failed error. In the leo, the frequency of switch between OpenGL and > HwcComposer is not very high, so it's not easy to reproduce the gen lock > failed. (Actually, I only got one in these two days.) diego, any comment or suggestion for lock fail?
Flags: needinfo?(dwilson)
Assignee | ||
Comment 37•7 years ago
|
||
Sotaro, Thanks for your information. Actually, my gecko already had your patch. I think the root cause was the graphicBuffer lock/unlock I added in HwcComposer (for LayerScope tool), even if I did nothing between the lock() and unlock(). They may affect the read & write locks in ContentClient double buffering. (In reply to Sotaro Ikeda [:sotaro] from comment #35) > I recently update a way of gralloc buffer binding by Bug 1005908. It seem to > simplify the binding problem except tiling layer.
Comment 38•7 years ago
|
||
(In reply to peter chang[:pchang][:peter] from comment #36) > diego, any comment or suggestion for lock fail? I think Boris' comment 34 is spot on. If you have any questions specific to HWC then sushilc (cc'ed) or I can try to answer them.
Flags: needinfo?(dwilson)
Comment hidden (obsolete) |
Assignee | ||
Updated•7 years ago
|
Attachment #8427566 -
Flags: feedback?(cku)
Reporter | ||
Comment 40•7 years ago
|
||
(In reply to Boris Chiou [:boris] from comment #39) > (Actually, Flame almost doesn't use HwcComposer on the master branch. > Maybe there are some bugs about it.) Please keep look into this problem and file a bug if necessary.
Reporter | ||
Comment 41•7 years ago
|
||
feedback |
Comment on attachment 8427566 [details] [diff] [review] Part 2: Support LayerScope for HwcComposer2D Review of attachment 8427566 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/LayerScope.cpp @@ +325,5 @@ > }; > > +// Create and Destory LayerScopeWebSocketManager object > +class WebSocketHelper > +{ Is this class needed? @@ +329,5 @@ > +{ > +public: > + static void CreateServerSocket() > + { > + // Create Web Server Socket (which has to be on the main thread) NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); @@ +497,5 @@ > return true; > } > > +private: > + void packing(DataSourceSurface* aImage) { void pack @@ +923,5 @@ > + if (!aLayer || !aLayer->GetLayer()) { > + return; > + } > + > + switch (aLayer->GetLayer()->GetType()) { why not just move switch statement into SnederHelper::SendLayer?
Attachment #8427566 -
Flags: feedback?(cku) → feedback+
Comment 42•7 years ago
|
||
feedback |
Comment on attachment 8418653 [details] [diff] [review] Part 1: General functions for EffectChains Since you add GenEffect() to all XXXXComposite class, I think you can also reuse this function to generate the effect used in CompostiorOGL::DrawQuad().
Updated•7 years ago
|
Attachment #8418653 -
Flags: feedback?(hshih)
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Comment hidden (obsolete) |
Assignee | ||
Updated•7 years ago
|
Attachment #8439665 -
Flags: feedback?(hshih)
Assignee | ||
Comment 47•7 years ago
|
||
Comment on attachment 8427566 [details] [diff] [review] Part 2: Support LayerScope for HwcComposer2D Review of attachment 8427566 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/LayerScope.cpp @@ +325,5 @@ > }; > > +// Create and Destory LayerScopeWebSocketManager object > +class WebSocketHelper > +{ Many classes access the LayerScopeWebSocketManager in this file, so I wrapped it as a static class. @@ +329,5 @@ > +{ > +public: > + static void CreateServerSocket() > + { > + // Create Web Server Socket (which has to be on the main thread) OK @@ +497,5 @@ > return true; > } > > +private: > + void packing(DataSourceSurface* aImage) { OK @@ +923,5 @@ > + if (!aLayer || !aLayer->GetLayer()) { > + return; > + } > + > + switch (aLayer->GetLayer()->GetType()) { OK
Assignee | ||
Comment 48•7 years ago
|
||
1. Retrieve buffer from GPU. 2. Refactor - use SenderHelper class to manage the Sender static function structure.
Attachment #8427566 -
Attachment is obsolete: true
Assignee | ||
Comment 49•7 years ago
|
||
(In reply to Jerry Shih[:jerry] from comment #42) > Comment on attachment 8418653 [details] [diff] [review] > Part 1: General functions for EffectChains > > Since you add GenEffect() to all XXXXComposite class, I think you can also > reuse this function to generate the effect used in CompostiorOGL::DrawQuad(). I can reuse the GenEffectChain() for ColorLayerComposite. However, (ThebesLayer|CanvasLayer|ImageLayer)Composite needs CompositableHost and should lock it first (by autolock). If I want to reuse GenEffectChain for these layers, Changing the scope of the lock of ContentHost or ImageHost is necessary. I am unsure the impact of it, so just keep the original code in my new patch. If changing the scope doesn't affect anything, I could generate the primary effect out of the composite function.
Assignee | ||
Comment 50•7 years ago
|
||
1. Support GenEffectChain() for LayerComposite. Each layer can use this API to gen the EffectChain (only primary effect now) 2. Support GenEffect() for CompositableHost. 3. Move AutoLock to compositeHost. Rebased.
Attachment #8439665 -
Attachment is obsolete: true
Attachment #8439665 -
Flags: feedback?(hshih)
Attachment #8444193 -
Flags: feedback?(hshih)
Comment hidden (obsolete) |
Comment 52•7 years ago
|
||
Comment on attachment 8444193 [details] [diff] [review] Part 1: General functions for Effects It looks good to me. I will check the part2 later.
Attachment #8444193 -
Flags: feedback?(hshih) → feedback+
Assignee | ||
Updated•7 years ago
|
Attachment #8444193 -
Flags: review?(dglastonbury)
Assignee | ||
Updated•7 years ago
|
Attachment #8439686 -
Flags: review?(dglastonbury)
Updated•7 years ago
|
Attachment #8444193 -
Flags: review?(dglastonbury) → review+
Comment 53•7 years ago
|
||
Comment on attachment 8439686 [details] [diff] [review] Part 2: Support LayerScope for HwcComposer2D Review of attachment 8439686 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/layers/LayerScope.cpp @@ +1009,5 @@ > +{ > + if (!LayerScope::CheckSendable()) { > + return; > + } > + #if 0 Remove dead code or put a comment why it's disabled and when it should be enabled. Please outdent so #if start at column 0.
Attachment #8439686 -
Flags: review?(dglastonbury) → review+
Assignee | ||
Comment 54•7 years ago
|
||
(In reply to Dan Glastonbury :djg :kamidphish from comment #53) > Comment on attachment 8439686 [details] [diff] [review] > Part 2: Support LayerScope for HwcComposer2D > > Review of attachment 8439686 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/layers/LayerScope.cpp > @@ +1009,5 @@ > > +{ > > + if (!LayerScope::CheckSendable()) { > > + return; > > + } > > + #if 0 > > Remove dead code or put a comment why it's disabled and when it should be > enabled. > > Please outdent so #if start at column 0. OK, I will remove this dead code in my next patch. Thanks.
Comment hidden (obsolete) |
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 56•7 years ago
|
||
Hi Boris, part 2 fails to apply for checkin with: applying Bug-975346---Part-2-Support-LayerScope-for-HwcComp.patch patching file gfx/layers/LayerScope.cpp Hunk #4 succeeded at 458 with fuzz 2 (offset 0 lines). patching file widget/gonk/HwcComposer2D.h Hunk #1 FAILED at 87 1 out of 1 hunks FAILED -- saving rejects to file widget/gonk/HwcComposer2D.h.rej patch failed, unable to continue (try -v) patch failed, rejects left in working dir errors during apply, please fix and refresh Bug-975346---Part-2-Support-LayerScope-for-HwcComp.patch could you take a look into this ? Thanks!
Keywords: checkin-needed
Assignee | ||
Comment 57•7 years ago
|
||
(In reply to Carsten Book [:Tomcat] from comment #56) > Hi Boris, > > part 2 fails to apply for checkin with: > > applying Bug-975346---Part-2-Support-LayerScope-for-HwcComp.patch > patching file gfx/layers/LayerScope.cpp > Hunk #4 succeeded at 458 with fuzz 2 (offset 0 lines). > patching file widget/gonk/HwcComposer2D.h > Hunk #1 FAILED at 87 > 1 out of 1 hunks FAILED -- saving rejects to file > widget/gonk/HwcComposer2D.h.rej > patch failed, unable to continue (try -v) > patch failed, rejects left in working dir > errors during apply, please fix and refresh > Bug-975346---Part-2-Support-LayerScope-for-HwcComp.patch > > could you take a look into this ? Thanks! OK, maybe I can rebase it.
Comment hidden (obsolete) |
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 59•7 years ago
|
||
All I see in this Try run are builds. Did we verify that tests pass too?
Keywords: checkin-needed
Comment hidden (obsolete) |
Assignee | ||
Comment 61•7 years ago
|
||
1. Retrieve buffer from GPU. 2. Refactor - use SenderHelper class to manage the Sender static function structure. Rebased. Try result: https://tbpl.mozilla.org/?tree=Try&rev=1b351bc9b24f
Attachment #8449305 -
Attachment is obsolete: true
Attachment #8450040 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 62•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/95020e2c631d https://hg.mozilla.org/integration/mozilla-inbound/rev/2b4e420f2cfd
Keywords: checkin-needed
Comment 63•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/95020e2c631d https://hg.mozilla.org/mozilla-central/rev/2b4e420f2cfd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•