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

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

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.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Refactor RasterImage to use DrawableFrameRef and generally clean up

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).

Comment 2

[Timothy Nikkel (: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);

Comment 3

[Seth Fowler [:seth] [:s2h]]

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 4

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8478049 [details] [diff] [review]
Refactor RasterImage to use DrawableFrameRef and generally clean up

Oh, sorry I was confused. You are correct.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Thanks for the review! Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/606aef8e8c16

Comment 6

[Gary [:streetwolf52]]

Attachment: Tabs rendered incorrectly.

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

Comment 7

[Gary [:streetwolf52]]

Forgot to add that I am running with a dpi of 120.

Comment 8

[Gary [:streetwolf52]]

Set 'layout.css.devPixelsPerPx = 1' which is 96 dpi and the problem still exists.

Comment 9

[Gary [:streetwolf52]]

Opened up Bug 1067207

Comment 10

[Gary [:streetwolf52]]

Problem might be a different patch.  See Bug 1067207 for more info.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/606aef8e8c16

Comment 12

[Seth Fowler [:seth] [:s2h]]

(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.

Comment 13

[Seth Fowler [:seth] [:s2h]]

(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 14

[Seth Fowler [:seth] [:s2h]]

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.