Closed Bug 1112972 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: Graphics: ImageLib, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox36 --- fixed
firefox37 --- fixed

People

(Reporter: seth, Assigned: seth)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 4 obsolete files)

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
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.
Blocks: 1079627
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)
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)
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)
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)
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)
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.
This new version of part 2 should solve all of the problems that showed up on try.
Attachment #8538786 - Flags: review?(tnikkel)
Attachment #8538229 - Attachment is obsolete: true
Attachment #8538229 - Flags: review?(tnikkel)
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)
Attachment #8538786 - Attachment is obsolete: true
Attachment #8538786 - Flags: review?(tnikkel)
Blocks: 1113465
Static analysis quite correctly pointed out a bad implicit conversion constructor for NextPartObserver. I've fixed it.
Attachment #8538967 - Flags: review?(tnikkel)
Attachment #8538938 - Attachment is obsolete: true
Attachment #8538938 - Flags: review?(tnikkel)
Attachment #8538967 - Attachment is obsolete: true
Attachment #8538967 - Flags: review?(tnikkel)
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+
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)
(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.
Depends on: 1120621
@: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)
(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)
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?
Attachment #8538226 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Depends on: 1151309
Depends on: 1153527
No longer depends on: 1153527
You need to log in before you can comment on or make changes to this bug.