Closed Bug 1057903 Opened 6 years ago Closed 6 years ago

Refactor RasterImage methods to use DrawableFrameRef's and generally clean up use of imgFrame

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(2 files)

Bug 1057894 adds a RAII smart handle for imgFrame called DrawableFrameRef, which we should use when we want to lock an imgFrame for drawing. RasterImage currently performs this locking through implicit calls to GetSurface (either in RasterImage itself or in code that it calls like imgFrame::Draw).

I want us to move away from this pattern, and only call GetSurface when we actually want a surface. I wrote a patch to refactor RasterImage to allow this. In the course of writing the patch, a number of opportunities for simplification and cleanup in the code I was touching jumped out, so I want to use this bug not just to mechanically replace GetSurface calls with DrawableRef but to improve the involved code while I'm at it.
Here's the patch. In addition to the switch to DrawableFrameRef there are a lot of small style and code quality issues fixed here, including a consolidation of the various GetImgFrame/GetDrawableImgFrame/etc methods which were mostly useless (and all needed to be touched in this patch anyway).
Attachment #8478049 - Flags: review?(tnikkel)
Comment on attachment 8478049 [details] [diff] [review]
Refactor RasterImage to use DrawableFrameRef and generally clean up

>@@ -839,20 +839,23 @@ RasterImage::CopyFrame(uint32_t aWhichFr
>+  DrawableFrameRef frameRef = GetFrame(GetRequestedFrameIndex(aWhichFrame));
>+  if (!frameRef) {
>+    // The OS threw this frame away.
>+    if (aFlags & FLAG_SYNC_DECODE) {
>+      ForceDiscard();
>+      return CopyFrame(aWhichFrame, aFlags);
>+    }
>     return nullptr;
>   }

Shouldn't this go
  if (!frameRef) {
    ForceDiscard();
    WantDecodedFrames();
    if (aFlags & FLAG_SYNC_DECODE) {
      return CopyFrame(aWhichFrame, aFlags);
    }
  }

We have the same logic in GetFrame, so same comment for GetFrame.

>@@ -1482,19 +1473,20 @@ RasterImage::StartAnimation()
>-  nsRefPtr<imgFrame> currentFrame = GetCurrentImgFrame();
>+  nsRefPtr<imgFrame> currentFrame = GetFrameNoDecode(GetCurrentFrameIndex());

This goes from asking for a decode to not asking for a decode, is that okay?

>@@ -2743,31 +2724,22 @@ RasterImage::Draw(gfxContext* aContext,
>+  DrawableFrameRef ref = GetFrame(GetRequestedFrameIndex(aWhichFrame));
>+  if (!ref) {
>     return NS_OK; // Getting the frame (above) touches the image and kicks off decoding
>   }

This if is now checking if we can get a drawable ref to the frame, before it was checking if we could get an imgFrame (even if it's buffer had been thrown away). So I think the comment is wrong and you need the force discard want decoded frames dance here that you remove below.

> 
>-  bool drawn = DrawWithPreDownscaleIfNeeded(frame, aContext, aSize,
>-                                            aRegion, aFilter, aFlags);
>-  if (!drawn) {
>-    // The OS threw out some or all of our buffer. Start decoding again.
>-    ForceDiscard();
>-    WantDecodedFrames();
>-    return NS_OK;
>-  }
>+  DrawWithPreDownscaleIfNeeded(Move(ref), aContext, aSize, aRegion, aFilter, aFlags);
Thanks for the review, Timothy!

(In reply to Timothy Nikkel (:tn) from comment #2)
> Shouldn't this go
>   if (!frameRef) {
>     ForceDiscard();
>     WantDecodedFrames();
>     if (aFlags & FLAG_SYNC_DECODE) {
>       return CopyFrame(aWhichFrame, aFlags);
>     }
>   }

Nah, because GetFrame() calls ForceDiscard() and WantDecodedFrames() automatically.

(BTW, I noticed after writing this patch that we ended up with two methods named GetFrame(), differing only by their argument types. This is confusing and I apologize. =\ I rename the new one, which replaces methods like GetDecodedImgFrame(), to LookupFrame() in one of the next patches up for review. Unfortunately the rebase looks like it'd be pretty ugly so I'd prefer to just live with this for these few patches where it's a problem.)


> >@@ -1482,19 +1473,20 @@ RasterImage::StartAnimation()
> >-  nsRefPtr<imgFrame> currentFrame = GetCurrentImgFrame();
> >+  nsRefPtr<imgFrame> currentFrame = GetFrameNoDecode(GetCurrentFrameIndex());
> 
> This goes from asking for a decode to not asking for a decode, is that okay?

I don't think asking for a decode here adds much value since animated frames aren't discardable. It has the disadvantage that if we ask for a DrawableFrameRef then we do additional locking that we don't really need here since we aren't going to draw.

(Additionally, this code gets replaced in a later one of the patches up for review right now.)

> So I think the comment is wrong and you need the force discard
> want decoded frames dance here that you remove below.

The comment's correct; GetFrame() (the new one, the one being called here) calls ForceDiscard() and WantDecodedFrames() automatically.
Comment on attachment 8478049 [details] [diff] [review]
Refactor RasterImage to use DrawableFrameRef and generally clean up

Oh, sorry I was confused. You are correct.
Attachment #8478049 - Flags: review?(tnikkel) → review+
Regression testing points to this patch drawing the top portion of tabs lower on the tab. 

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Forgot to add that I am running with a dpi of 120.
Set 'layout.css.devPixelsPerPx = 1' which is 96 dpi and the problem still exists.
Opened up Bug 1067207
Blocks: 1067207
No longer blocks: 1067207
Problem might be a different patch.  See Bug 1067207 for more info.
https://hg.mozilla.org/mozilla-central/rev/606aef8e8c16
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
(In reply to Gary [:streetwolf] from comment #10)
> Problem might be a different patch.  See Bug 1067207 for more info.

There's pretty much no chance it's this patch, since this patch doesn't touch SVG drawing and the tabs are SVGs. I think the other patch you suggest in that bug is much more likely.
Depends on: 1067207
(In reply to Seth Fowler [:seth] from comment #12)
> There's pretty much no chance it's this patch, since this patch doesn't
> touch SVG drawing and the tabs are SVGs.

Actually I was wrong about this; the tabs are now PNGs, not SVGs, so this patch is the culprit.
Comment on attachment 8478049 [details] [diff] [review]
Refactor RasterImage to use DrawableFrameRef and generally clean up

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

::: image/src/RasterImage.cpp
@@ +2643,5 @@
>  
> +  nsIntMargin padding(finalFrameRect.y,
> +                      mSize.width - finalFrameRect.XMost(),
> +                      mSize.height - finalFrameRect.YMost(),
> +                      finalFrameRect.x);

So I think this is the problem: we're computing the padding based upon the rect of the final frame, but it actually needs to be computed based upon the original, unscaled frame.

I'll create a patch accordingly. It'll be posted in bug 1067207; I won't post further about it here.
Depends on: 1068881
No longer depends on: 1068881
Depends on: 1069369
Depends on: 1072539
You need to log in before you can comment on or make changes to this bug.