1150381 - [LayerScope]: Don't showing the same texture in the same frame on LayerScope.

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Vincent Liu[:vliu]]

In some situations, the device will paint the same texture in a frame. This case will also draw twice on Layerscope. This bug intends to come out a patch to remove one of them on Layerscope since we don't need to show it twice.

Comment 1

[Vincent Liu[:vliu]]

Attachment: WIP-1.patch

Hi Chiajung,

As our discussion offline, the proposed patch was attached. Could you have a feedback for the patch? Thanks

Comment 2

[Chiajung Hung [:chiajung]]

Comment on attachment 8587204 [details] [diff] [review]
WIP-1.patch

Review of attachment 8587204 [details] [diff] [review]:
-----------------------------------------------------------------

I think layer scope may need some refactoring to abstract Session and Frame submitting.
That way, we should able to avoid static object like the texture catch

::: gfx/layers/LayerScope.cpp
@@ +1376,4 @@
>  LayerScopeAutoFrame::LayerScopeAutoFrame(int64_t aFrameStamp)
>  {
>      // Do Begin Frame
> +    SenderHelper::ClearTextureIdList();

Move into BeginFrame would be better.

Comment 3

[Chiajung Hung [:chiajung]]

Comment on attachment 8587204 [details] [diff] [review]
WIP-1.patch

Review of attachment 8587204 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 4

[Vincent Liu[:vliu]]

Attachment: bug-1150381-fix.patch

Hi Chiajung,

Thanks for your feedback for my WIP. Based on your suggestion, I rearrange my patch to review. Can you please help me to review my patch?

Comment 5

[Chiajung Hung [:chiajung]]

Comment on attachment 8590587 [details] [diff] [review]
bug-1150381-fix.patch

Review of attachment 8590587 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Please remove redundant white space before uploading

::: gfx/layers/LayerScope.cpp
@@ +1388,5 @@
>  void
>  LayerScopeAutoFrame::BeginFrame(int64_t aFrameStamp)
>  {
> +    SenderHelper::ClearTextureIdList();
> +    

space.

Comment 6

[Vincent Liu[:vliu]]

(In reply to Chiajung Hung [:chiajung] from comment #5)
> Comment on attachment 8590587 [details] [diff] [review]
> bug-1150381-fix.patch
> 
> Review of attachment 8590587 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM. Please remove redundant white space before uploading
> 
> ::: gfx/layers/LayerScope.cpp
> @@ +1388,5 @@
> >  void
> >  LayerScopeAutoFrame::BeginFrame(int64_t aFrameStamp)
> >  {
> > +    SenderHelper::ClearTextureIdList();
> > +    
> 
> space.

Thanks for the review. I will remove the space when I land it.

Comment 7

[Vincent Liu[:vliu]]

Attachment: bug-1150381-v2.patch

(In reply to Vincent Liu[:vliu] from comment #4)
> Created attachment 8590587 [details] [diff] [review]
> bug-1150381-fix.patch
> 
> Hi Chiajung,
> 
> Thanks for your feedback for my WIP. Based on your suggestion, I rearrange
> my patch to review. Can you please help me to review my patch?

The first version of patch caused the try server fail since the declaration of List<> can't find its definition in non-Android platform. I tried to change to std::list instead. Talked with review on IRC, and he is fine with this change.

Comment 8

[Vincent Liu[:vliu]]

try server result:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=177deec61d4d

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ebc6fb05d55

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7ebc6fb05d55