Closed Bug 975346 Opened 7 years ago Closed 7 years ago

[LayerScope] LayerScope is not work while HWC is chosen

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla33

People

(Reporter: u459114, Assigned: boris)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 25 obsolete files)

Part 1: General functions for Effects
26.33 KB, patch
djg
: review+
jerry
: feedback+
Details | Diff | Splinter Review
Part 2: Support LayerScope for HwcComposer2D. r=dglastonbury (carry dglastonbury's r+)
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
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.
Sorry, I should correct my comment. All the compositors will set "GLContext" to 0 by default.
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: mtseng → boris.chiou
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
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
Status: NEW → ASSIGNED
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
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)
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?
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.
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+
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
Attachment #8420842 - Flags: feedback?(hshih)
Attachment #8420842 - Flags: feedback?(cku)
Attachment #8418653 - Flags: feedback?(hshih)
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+
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;
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
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
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.
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.
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.)
I recently update a way of gralloc buffer binding by Bug 1005908. It seem to simplify the binding problem except tiling layer.
(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)
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.
(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)
Attachment #8427566 - Flags: feedback?(cku)
(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.
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 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().
Attachment #8418653 - Flags: feedback?(hshih)
Attachment #8439665 - Flags: feedback?(hshih)
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
1. Retrieve buffer from GPU.
2. Refactor - use SenderHelper class to manage the Sender
   static function structure.
Attachment #8427566 - Attachment is obsolete: true
(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.
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 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+
Attachment #8444193 - Flags: review?(dglastonbury)
Attachment #8439686 - Flags: review?(dglastonbury)
Attachment #8444193 - Flags: review?(dglastonbury) → review+
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+
(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.
Keywords: checkin-needed
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
(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.
Keywords: checkin-needed
All I see in this Try run are builds. Did we verify that tests pass too?
Keywords: checkin-needed
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+
Keywords: checkin-needed
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.