1112972 - Reimplement multipart/x-mixed-replace images using an ImageWrapper

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

Currently, the logic for multipart/x-mixed-replace images is spread out in many places in ImageLib. It's highly desirable to consolidate that for maintenance reasons; see this dev-platform post for some discussion:

https://groups.google.com/forum/#!topic/mozilla.dev.platform/d49l-T9q3kM

It's also the case that the current implementation of multipart images is standing in the way of implementing multiple decoders for a single RasterImage (bug 1079627). This in turn blocks downscale-during-decode (bug 1045926). It would be possible to solve these problems, but the solution adds more complexity than I think is justified. I'd rather move us in the direction of keeping the code simple by changing how multipart images are implemented.

This bug is the first and most important of two bugs where I'll tackle this. In this bug, I'll reimplement multipart images using a new ImageWrapper, MultipartImage. This will maintain all of the functionality that we have right now. In fact, in my tests it produces much smoother results, since it eliminates some glitchy invalidations that we get right now. After this bug, most but not all of the unwanted multipart logic in other parts of ImageLib will be eliminated. In a followup bug I'll remove support for everything but MJPEG, as discussed in the dev-platform post, which will enable me to completely encapsulate multipart image support to MultipartImage and the image loading code which interacts with it.

Comment 1

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

Attachment: (Part 1) - Minor refactoring to prepare for MultipartImage

This patch starts things off with some minor up-front refactoring that will make the code in subsequent parts a little cleaner. These changes didn't feel like they deserved their own bug.

Comment 2

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

Attachment: (Part 2) - Add MultipartImage and use it for multipart/x-mixed-replace image loading

This patch is the meat of this bug. It adds MultipartImage and alters the image loading code to use it.

Comment 3

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

Attachment: (Part 3) - Remove almost all special handling of multipart images in RasterImage

Now we can begin to remove some code that was obsoleted by part 2. This part removes almost all special handling of multipart images in RasterImage. The only things that remain are:

- An |mTransient| flag, which replaces |mMultipart| and simply indicates that we
  don't expect the image to stick around for long. I can see other uses for this
  besides multipart appearing in the future. When this flag is set, we don't
  optimize or HQ scale the image's frames.

- A stub for |OnNewSourceData| sticks around for the moment. It will get removed
  in the next part.

Comment 4

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

Attachment: (Part 4) - Remove Image::OnNewSourceData

This part just removes Image::OnNewSourceData. Since it was a method on Image, this requires touching a lot of files, so I put this in its own part.

Comment 5

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

Attachment: (Part 5) - Remove almost all special handling of multipart images in ProgressTracker

Finally, this part removes almost all special handling of multipart images in ProgressTracker. What's left is |ResetForNewRequest| and code related to the issue of whether we've gotten the final part of a multipart request. Those remaining bits of code should be removed in the followup where we switch to supporting MJPEG only.

Comment 6

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

Here's a try job:

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

Comment 7

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

Try revealed two issues: a missing header in MultipartImage.cpp (only triggered for Linux debug builds; not sure why) and an ASAN use-after-free. I'll address those issues shortly.

Comment 8

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

Attachment: (Part 2) - Add MultipartImage and use it for multipart/x-mixed-replace image loading

This new version of part 2 should solve all of the problems that showed up on try.

Comment 9

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

Here's a new try job:

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

Comment 10

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

Attachment: (Part 2) - Add MultipartImage and use it for multipart/x-mixed-replace image loading

That previous try job looked good, but I'm going to push another one because I managed to cause a crash which was fixed with a tweak to OnImageDataAvailable and OnImageDataComplete; they now keep a strong reference to mNextPart when sending those notifications, since the notifications can trigger the release of mNextPart indirectly.

Also, the previous try job didn't end up running an ASAN job, which I intended. This new one does:

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

Comment 11

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

Attachment: (Part 2) - Add MultipartImage and use it for multipart/x-mixed-replace image loading

Static analysis quite correctly pointed out a bad implicit conversion constructor for NextPartObserver. I've fixed it.

Comment 12

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

Attachment: (Part 2) - Add MultipartImage and use it for multipart/x-mixed-replace image loading

Comment tweaks.

Comment 13

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

Apparently try got reset, but for future reference, the try job in this bug was green.

Comment 14

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

Thanks for the reviews!

Pushed:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a94ab57658c0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/daf5c861dee8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d97d45fa892d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/82ff19dfa592
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/da5084c9e3e6

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a94ab57658c0
https://hg.mozilla.org/mozilla-central/rev/daf5c861dee8
https://hg.mozilla.org/mozilla-central/rev/d97d45fa892d
https://hg.mozilla.org/mozilla-central/rev/82ff19dfa592
https://hg.mozilla.org/mozilla-central/rev/da5084c9e3e6

Comment 16

[Daniel Kabs, reporting bugs since 2002]

I haven't checked nightly builds but according to the discussion, I gleaned that size changes will not be supported any longer. That is sad and might break current web pages and web applications using MJPEG.

I'd like to point out that current versions of Firefox (e.g., FF35) support size changes during streaming!

Try, e.g., Firefox 35 on 

  http://130.89.113.150/control/faststream.jpg?stream=full&stream=full

and set different images sizes while watching the stream:

  http://130.89.113.150/control/control?section=general&size=640x480
or
  http://130.89.113.150/control/control?section=general&size=1024x768

(Please reset size to 1024x768 after testing)

Comment 17

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

(In reply to Daniel Kabs, reporting bugs since 2002 from comment #16)
> I haven't checked nightly builds but according to the discussion, I gleaned
> that size changes will not be supported any longer. That is sad and might
> break current web pages and web applications using MJPEG.

You're posting in the wrong bug about this issue. That's bug 1113465.

Comment 18

[Alice0775 White]

@:seth
This caused Bug 1120621.
Firefox consumes network resources and not release it after open MJPEG stream.

Could you backed out this?

Comment 19

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

(In reply to Alice0775 White from comment #18)
> Could you backed out this?

We should fix the problem, not back it out.

Comment 20

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

Comment on attachment 8538226 [details] [diff] [review]
(Part 1) - Minor refactoring to prepare for MultipartImage

Approval Request Comment
[Feature/regressing bug #]: Unknown.
[User impact if declined]: MJPEG streams and webcams stutter and don't render correctly.
[Describe test coverage new/current, TreeHerder]: Already in Firefox 38 and 37.
[Risks and why]: Given that the new code has been around for so long, should be fairly low risk. One bug was discovered in this code, and it has already been fixed. I'll request uplift for the fix as well.
[String/UUID change made/needed]: None.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/28f4806f60ee
https://hg.mozilla.org/releases/mozilla-beta/rev/438fed84c7c3
https://hg.mozilla.org/releases/mozilla-beta/rev/c858f34b8153
https://hg.mozilla.org/releases/mozilla-beta/rev/c4689eff54db
https://hg.mozilla.org/releases/mozilla-beta/rev/274a8354e230