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

RESOLVED FIXED in Firefox 40

Status

()

Core
Graphics
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: vliu, Assigned: vliu)

Tracking

unspecified
mozilla40
ARM
Linux
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Summary: [LayerScope]: Don't showing the same texture ID in Layerscope in the same frame. → [LayerScope]: Don't showing the same texture in the same frame on LayerScope.
(Assignee)

Updated

3 years ago
Assignee: nobody → vliu
(Assignee)

Comment 1

3 years ago
Created attachment 8587204 [details] [diff] [review]
WIP-1.patch

Hi Chiajung,

As our discussion offline, the proposed patch was attached. Could you have a feedback for the patch? Thanks
Attachment #8587204 - Flags: feedback?(chung)
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 on attachment 8587204 [details] [diff] [review]
WIP-1.patch

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

Looks good to me.
Attachment #8587204 - Flags: feedback?(chung) → feedback+
(Assignee)

Comment 4

3 years ago
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?
Attachment #8590587 - Flags: review?(chung)
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.
Attachment #8590587 - Flags: review?(chung) → review+
(Assignee)

Comment 6

3 years ago
(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.
(Assignee)

Comment 7

3 years ago
Created attachment 8592088 [details] [diff] [review]
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.
Attachment #8587204 - Attachment is obsolete: true
Attachment #8590587 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7ebc6fb05d55
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.