1057904 - Refactor FrameBlender, FrameSequence, and related classes to use RawAccessFrameRef and clean things up

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

Like bug 1057903, this bug is about taking one of the new RAII smart handles for imgFrame added in bug 1057894 - in this case RawAccessFrameRef - and refactoring existing code to use it. Also like bug 1057903, I expect there will be a lot of opportunities for cleanup while I do this, so the scope of this bug includes cleaning up and simplifying the code that gets touched wherever its reasonable to do so.

The key change in this bug is that, since imgFrame objects are now ref-counted and we have a smart handle to keep them locked, we probably no longer need FrameSequence at all. An nsTArray of RawAccessFrameRef objects, for example, may be sufficient to replace FrameSequence completely. (Though that would probably require adding a copy constructor to RawAccessFrameRef.) I suspect that making that change will reveal many other opportunities for code cleanup.

Comment 1

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

Attachment: (Part 1) - Use RawAccessRef in FrameBlender and related classes and clean up

Here's the patch. We don't need FrameSequence at all anymore; an nsTArray<RawAccessFrameRef> gives us pretty much everything that FrameSequence did. Even nicer, we don't need to manually call LockImageData() and UnlockImageData() at all anymore.

Reading the comments in the code can be a little misleading as to what the existing behavior of the code was. RasterImage did a lot of locking and unlocking that was pretty much totally redundant. In practice, FrameSequence locked *all* of the frames of any animated image, and kept them locked. The only situation where we'd ever unlock a frame is for non-animated images.

This patch changes that behavior: we lock all frames, all the time. That's not desirable, because we can't optimize or discard locked frames, so after this patch we won't be able to do that even in the non-animated image case. This is intentional, though. Bug 1060869 will stop storing the frames of non-animated images in a FrameBlender and start storing them in the SurfaceCache. That will restore the current behavior, but in a much cleaner and more flexible manner. To ensure that there's no regression, I won't land this patch separately from the one in bug 1060869.

Comment 2

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

Attachment: (Part 2) - Make imgFrame::LockImageData and UnlockImageData private

After the patch in part 1, nothing actually calls LockImageData or UnlockImageData directly, so this patch makes them private. They're now just implementation details of RawAccessFrameRef.

Comment 3

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8481884 [details] [diff] [review]
(Part 1) - Use RawAccessRef in FrameBlender and related classes and clean up

> private: // data
>   //! All the frames of the image
>-  nsRefPtr<FrameSequence> mFrames;
>+  nsTArray<RawAccessFrameRef> mFrames;

So this means that now we hold a raw access ref to all frames of all images at all times after they are decoded? (As opposed to currently where we only do that for animated images.)

Comment 4

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

Thanks for the reviews!

(In reply to Timothy Nikkel (:tn) from comment #3)
> So this means that now we hold a raw access ref to all frames of all images
> at all times after they are decoded? (As opposed to currently where we only
> do that for animated images.)

Yup! See comment 1. I'll land this together with bug 1060869, which fixes that problem by not storing non-animated image frames in a FrameBlender.

Comment 5

[Timothy Nikkel (:tnikkel)]

Sorry, I missed comment 1.

Comment 6

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8481884 [details] [diff] [review]
(Part 1) - Use RawAccessRef in FrameBlender and related classes and clean up

>@@ -1395,18 +1376,17 @@ RasterImage::DecodingComplete()
>   // Double-buffer our frame in the multipart case, since we'll start decoding
>   // into the first frame again immediately and this produces severe tearing.
>   if (mMultipart) {
>     if (GetNumFrames() == 1) {
>-      mMultipartDecodedFrame = mFrameBlender.SwapFrame(GetCurrentFrameIndex(),
>-                                                       mMultipartDecodedFrame);
>+      mMultipartDecodedFrame = mFrameBlender.GetFrame(GetCurrentFrameIndex());

Is this still going to double buffer without swapping frames? It's not immediately clear.

Otherwise looks good.

Comment 7

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

(In reply to Timothy Nikkel (:tn) from comment #6)
> Is this still going to double buffer without swapping frames? It's not
> immediately clear.

Yeah, we're still double buffering. The two buffers here are the normal buffer we decode into (the buffer that mFrameBlender holds) and the extra buffer we store in mMultipartDecodedFrame. That extra buffer is the last complete frame we decoded.

SwapFrame() was used in the past because we needed to both store a reference to the just-decoded frame in mMultipartDecodedFrame and remove the frame from mFrameBlender. The reason we needed to remove it is that we destroyed the old frame when we redecoded (and getting the next frame in a multipart/x-mixed-replace situation *is* redecoding). That destruction happened unconditionally, since imgFrame's weren't reference counted.

Now that we reference count imgFrame objects, we don't care if the imgFrame stays in mFrameBlender; as long as mMultipartDecodedFrame holds a reference to it, it stays alive.

Comment 8

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

Here's a try job, since we're getting closer to being able to push this:

https://tbpl.mozilla.org/?tree=Try&rev=b8a213495d82

Comment 9

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8481884 [details] [diff] [review]
(Part 1) - Use RawAccessRef in FrameBlender and related classes and clean up

Okay, thanks for the explanation.

Comment 10

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

Thanks for the review, Timothy!

So I need to debug the try failures above in more detail, but after looking through them I think there are probably two problems:

1. I've made a mistake somewhere with animation playback. Probably the culprit is in ResetAnimation(), although I'm not sure.

2. We're getting a segfault in the HQ scaling code which probably indicates that I'm failing to keep something alive that needs to be kept alive. (Almost certainly the imgFrame's buffer.) Not sure exactly how that's happening yet, but it's probably a bug in either ScaleRunner or RawAccessRef.

I'll investigate in more detail and update the patch once I figure this stuff out.

Comment 11

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

Here's a new try job based on top of bug 1069652, which may fix issue #2.

https://tbpl.mozilla.org/?tree=Try&rev=840d491011b0

Comment 12

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

Attachment: (Part 1) - Use RawAccessRef in FrameBlender and related classes and clean up

I found the animation bug; it looks like FrameDataPair::GetFrameData() returned imgFrame::GetImageData() for normal images, but imgFrame::GetPaletteData() for paletted images. I added imgFrame::GetRawData() to do the same thing (that's essentially what it does, return a pointer to the start of the imgFrame's raw data) and switched to using it in FrameBlender.

Local tests suggest the bug is now fixed. This might be ready to go, but we need another try job to be sure.

Comment 13

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

Here's the new try job:

https://tbpl.mozilla.org/?tree=Try&rev=07f682a6169b

Comment 14

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

Here's another try job, since things have changed quite a bit:

https://tbpl.mozilla.org/?tree=Try&rev=327b21860888

Comment 15

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

Attachment: (Part 1) - Use RawAccessRef in FrameBlender and related classes and clean up

This modified version of part 1 fixes the invalidation issue seen on try by returning |false| from imgFrame::GetNeedsBackground() if we know the frame has no alpha, even if we haven't changed the surface type yet. This was an issue because bug 1070426 made us not change the surface type until we unlock the imgFrame, but in this bug, temporarily, we leave all imgFrame objects locked permanently! It's generally a desireable change, though.

Comment 16

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

Here's a new try job, hopefully the last for this bug:

https://tbpl.mozilla.org/?tree=Try&rev=bb16f0063b55

Comment 17

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

Alright, it looks like try's all green, so this is ready to land. However, it needs to land together with bug 1060869, and I'm still tweaking that one.

Comment 18

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

Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/320de51add94
https://hg.mozilla.org/integration/mozilla-inbound/rev/60d6c5549706

Comment 19

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

Backed out:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e388678a9211

Comment 20

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

Attachment: (Part 1) - Use RawAccessRef in FrameBlender and related classes and clean up

After rebasing, this patch busted 'test_animation_operators.html'. I've fixed it by making sure that we always retrieve the frame disposal method from the *real* underlying frame, and not from the compositing frame. I don't really understand why this is now necessary, though. It seems like that should've bitten us with or without this patch...

Comment 21

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

OK, looks like this is green on try:

https://tbpl.mozilla.org/?tree=Try&rev=08d45823e56f

Comment 22

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

Pushed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/62c4e3d64b7d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b42bc7db8a8e

Comment 23

[Carsten Book [:Tomcat]]

sorry seth had to back this out in https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=e04692266f17 since one of this changes caused test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4191914&repo=mozilla-inbound

Seth i noticed this failure also on https://tbpl.mozilla.org/?tree=Try&rev=90124f3bc2a0 and did a retrigger, maybe this helps

Comment 24

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

Pushed again after (hopefully) resolving that problem:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2fd42e5e2920
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/351d96ab9475

Comment 25

[Carsten Book [:Tomcat]]

sorry had to back this out again in https://treeherder.mozilla.org/ui/#/jobs?repo=mozilla-inbound&revision=b5dda6f01770 since one of this changes caused https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4220043&repo=mozilla-inbound

Comment 26

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

Had I still been awake I would've pointed out that that failure was definitely from bug 1065818 =(

Repushed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/63c554369c55
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/25bf6420a6ad

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/63c554369c55
https://hg.mozilla.org/mozilla-central/rev/25bf6420a6ad