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

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

unspecified
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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.
Blocks: 1060869
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.
Attachment #8481884 - Flags: review?(tnikkel)
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.
Attachment #8481885 - Flags: review?(tnikkel)
Attachment #8481885 - Flags: review?(tnikkel) → review+
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.)
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.
Sorry, I missed comment 1.
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.
(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.
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 on attachment 8481884 [details] [diff] [review]
(Part 1) - Use RawAccessRef in FrameBlender and related classes and clean up

Okay, thanks for the explanation.
Attachment #8481884 - Flags: review?(tnikkel) → review+
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.
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
Depends on: 1070340
Depends on: 1070426
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.
Attachment #8481884 - Attachment is obsolete: true
Here's another try job, since things have changed quite a bit:

https://tbpl.mozilla.org/?tree=Try&rev=327b21860888
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.
Attachment #8492548 - Attachment is obsolete: true
Here's a new try job, hopefully the last for this bug:

https://tbpl.mozilla.org/?tree=Try&rev=bb16f0063b55
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.
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...
Attachment #8496364 - Attachment is obsolete: true
OK, looks like this is green on try:

https://tbpl.mozilla.org/?tree=Try&rev=08d45823e56f
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
Flags: needinfo?(seth)
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
Flags: needinfo?(seth)
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
https://hg.mozilla.org/mozilla-central/rev/63c554369c55
https://hg.mozilla.org/mozilla-central/rev/25bf6420a6ad
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Target Milestone: mozilla37 → mozilla36
Depends on: 1106252
Depends on: 1106423
Depends on: 1107060
No longer depends on: 1107060
No longer depends on: 1106252
You need to log in before you can comment on or make changes to this bug.