If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 36

Status

()

Core
ImageLib
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

(Blocks: 1 bug)

unspecified
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed)

Details

Attachments

(5 attachments, 4 obsolete attachments)

8.94 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
14.94 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
7.92 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
5.02 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
25.47 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
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.
(Assignee)

Updated

3 years ago
Blocks: 1079627
(Assignee)

Comment 1

3 years ago
Created attachment 8538226 [details] [diff] [review]
(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.
Attachment #8538226 - Flags: review?(tnikkel)
(Assignee)

Comment 2

3 years ago
Created attachment 8538229 [details] [diff] [review]
(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.
Attachment #8538229 - Flags: review?(tnikkel)
(Assignee)

Comment 3

3 years ago
Created attachment 8538230 [details] [diff] [review]
(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.
Attachment #8538230 - Flags: review?(tnikkel)
(Assignee)

Comment 4

3 years ago
Created attachment 8538231 [details] [diff] [review]
(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.
Attachment #8538231 - Flags: review?(tnikkel)
(Assignee)

Comment 5

3 years ago
Created attachment 8538235 [details] [diff] [review]
(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.
Attachment #8538235 - Flags: review?(tnikkel)
(Assignee)

Comment 6

3 years ago
Here's a try job:

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

Comment 7

3 years ago
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.
(Assignee)

Comment 8

3 years ago
Created attachment 8538786 [details] [diff] [review]
(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.
Attachment #8538786 - Flags: review?(tnikkel)
(Assignee)

Updated

3 years ago
Attachment #8538229 - Attachment is obsolete: true
Attachment #8538229 - Flags: review?(tnikkel)
(Assignee)

Comment 9

3 years ago
Here's a new try job:

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

Comment 10

3 years ago
Created attachment 8538938 [details] [diff] [review]
(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
Attachment #8538938 - Flags: review?(tnikkel)
(Assignee)

Updated

3 years ago
Attachment #8538786 - Attachment is obsolete: true
Attachment #8538786 - Flags: review?(tnikkel)
(Assignee)

Updated

3 years ago
Blocks: 1113465
(Assignee)

Comment 11

3 years ago
Created attachment 8538967 [details] [diff] [review]
(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.
Attachment #8538967 - Flags: review?(tnikkel)
(Assignee)

Updated

3 years ago
Attachment #8538938 - Attachment is obsolete: true
Attachment #8538938 - Flags: review?(tnikkel)
(Assignee)

Comment 12

3 years ago
Created attachment 8538975 [details] [diff] [review]
(Part 2) - Add MultipartImage and use it for multipart/x-mixed-replace image loading

Comment tweaks.
Attachment #8538975 - Flags: review?(tnikkel)
(Assignee)

Updated

3 years ago
Attachment #8538967 - Attachment is obsolete: true
Attachment #8538967 - Flags: review?(tnikkel)
(Assignee)

Comment 13

3 years ago
Apparently try got reset, but for future reference, the try job in this bug was green.
Attachment #8538226 - Flags: review?(tnikkel) → review+
Attachment #8538230 - Flags: review?(tnikkel) → review+
Attachment #8538231 - Flags: review?(tnikkel) → review+
Attachment #8538235 - Flags: review?(tnikkel) → review+
Attachment #8538975 - Flags: review?(tnikkel) → review+
(Assignee)

Comment 14

3 years ago
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
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
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
(Assignee)

Updated

3 years ago
Blocks: 987135
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)
(Assignee)

Comment 17

3 years ago
(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.

Updated

3 years ago
Depends on: 1120621

Comment 18

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

Could you backed out this?
Flags: needinfo?(seth)
(Assignee)

Comment 19

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

We should fix the problem, not back it out.
Flags: needinfo?(seth)
(Assignee)

Comment 20

3 years ago
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.
Attachment #8538226 - Flags: approval-mozilla-beta?
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → fixed
(Assignee)

Updated

3 years ago
status-firefox37: affected → fixed
Attachment #8538226 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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
status-firefox36: affected → fixed
status-firefox38: fixed → ---
(Assignee)

Updated

3 years ago
Depends on: 1151309

Updated

3 years ago
Depends on: 1153527

Updated

3 years ago
No longer depends on: 1153527
You need to log in before you can comment on or make changes to this bug.