1113465 - Support only MJPEG streams and no other kind of multipart/x-mixed-replace content in ImageLib

Status: NEW

(Core :: Graphics: ImageLib, defect)

Description

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

It's desirable to switch to only supporting MJPEG streams and not arbitrary multipart/x-mixed-replace image streams in ImageLib. This is discussed in bug 1112972 and in more detail in this dev-platform post:

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

In this bug we'll enforce MJPEG-only for multipart/x-mixed-replace in ImageLib and make some ImageLib-level simplifications that are enabled by that change. Although it will also be possible to make simplifications in code outside of ImageLib, that's out of scope for this bug.

It used to be possible that the content type, size, and animation status of an image could change with every part. This new implementation will enforce only JPEG parts (taking care of both content type and animation) and will forbid size changes once the size of the initial part is known. This not only eliminates the need for content and layout code to handle these changes, it also enables us to treat a multipart image as just another type of animation. Very little needs to change notification-wise to enable this; the main differences are that we won't resend notifications like SIZE_AVAILABLE for each part, and that we'll only send LOAD_COMPLETE once, at the end of the stream.

Comment 1

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

Attachment: (Part 1) - Support only MJPEG streams and no other kind of multipart images in ImageLib

The patches for this bug proved a little bit hard to split up and still produce something that compiles. I decided to split them in two; this patch contains the "important" changes, while the second part contains the simple things like changes that only involve function signatures. I'll fold the two patches back into one patch before committing, since they don't build separately.

There are a few things going on here. We enforce the MJPEG constraints by checking the content type in ImageFactory and the size in MultipartImage. Because we don't want to send duplicate notifications anymore (including LOAD_COMPLETE), we don't reset the ProgressTracker in MultipartImage anymore, and we no longer need the aLastPart argument to OnLoadComplete (since we only send LOAD_COMPLETE for the last part now). Those changes propagate through ProgressTracker, IProgressObserver, Image, and imgRequestProxy, allowing us to simplify a lot of things related to LOAD_COMPLETE. We do need some way of knowing that we've arrived at the last part, and because this is an issue specific to MultipartImage we just tell it directly via a new MultipartImage::GotLastPart method, instead of going through ProgressTracker.

After this patch, nothing in ImageLib needs to know anything about multipart images except for imgRequest, MultipartImage, and ImageFactory. Nothing outside of ImageLib does either - multipart images will just behave like animations.

Comment 2

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

Attachment: (Part 2) - Propagate signature changes throughout ImageLib

This patch contains the uninteresting bits - mostly just propagating signature changes from part 1 through the rest of ImageLib.

Comment 3

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

Here's a try job:

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

Comment 4

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

(Ack, I just realized that I didn't update the multipart/x-mixed-replace tests, which will undoubtedly fail since they test things that we now explicitly don't support. I'll update those tests. The try job is still useful as a sanity check, though.)

Comment 5

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

Looks like the failing tests are |image/test/mochitest/test_bug733553.html| and |image/test/reftest/gif/webcam.html|, which are exactly the tests that need to be updated, as mentioned in comment 4.

|image/test/reftest/gif/webcam.html| can just be removed, because we're not going to support multipart GIFs anymore.

|image/test/mochitest/test_bug733553.html| tests for a bunch of things we no longer support, but we still want something similar that tests our error handling. My plan is to break it into several smaller tests that test our handling of various kinds of errors in the multipart stream.

Comment 6

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

Attachment: (Part 1) - Support only MJPEG streams and no other kind of multipart images in ImageLib

I built a new suite of tests (which I'm just about to post as part 3) and they helped me discover a couple of issues with part 1. Primarily:

- We have to be sure not to notify things like FLAG_HAS_ERROR and
  FLAG_LOAD_COMPLETE from individual parts, since the fact that an individual
  part has loaded or contains errors no longer affects the overall
  MultipartImage, which just keeps on loading until the end of the stream is
  reached.

- We also don't want to cancel multipart imgRequests just because of a bad
  status from Necko in imgRequest::OnStopRequest, because those bad statuses
  really only refer to a single part. (Even if the data is truncated, that's
  true - only the last part is in error.)

I've made appropriate modifications to MultipartImage::FinishTransition and imgRequest::OnStopRequest in this new version of the patch.

Comment 7

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

Attachment: (Part 3) - Remove non-MJPEG multipart tests and add new MJPEG tests

This patch removes old tests which check for things we don't support anymore (see comment 5). It adds a number of new tests inspired by the old |test_bug733553.html|, which check that the multipart image code behaves well in situations like bad content types, bad sizes for individual parts, and invalid or corrupt parts.

I intended to use query parameters to generate a lot of tests easily here, which is a trick I've used frequently with reftests, but I ran into trouble because mochitests don't support query parameters. Sharing code in .SJS files also turns out to be awkward. I've used |hg cp| to duplicate the same test many times for now. If you look at one |.html| and one |.sjs| file, you'll get the idea; the only thing that varies is the |.sjs| file the |.html| file points to, and the sequence of parts at the top of each |.sjs| file.

Comment 8

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

Here's a new try job:

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

Comment 9

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

Hmm. Everything looks green except a leak, which is almost certainly caused by failing to call Cancel in imgRequest::OnStopRequest. I'll have to revisit that decision. Shouldn't be a big change, though. At one point, instead of not cancelling, I simply didn't send a FLAG_HAS_ERROR notification for multipart images, which might be all that is really needed.

Comment 10

[Daniel Kabs, reporting bugs since 2002]

I haven't checked nightly builds but according to the discussion, I gleaned that the new implementation will not support JPEG image size changes during streaming. 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 11

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

(In reply to Daniel Kabs, reporting bugs since 2002 from comment #10)
> I haven't checked nightly builds but according to the discussion, I gleaned
> that the new implementation will not support JPEG image size changes during
> streaming. That is sad and might break current web pages and web
> applications using MJPEG.

Daniel, thanks for letting us know about your concerns here! When making decisions like this we're weighing a lot of factors, and it really helps us to hear about how people are using these features.

That said, this change isn't as broad as you may be thinking it is.

The changes in this bug will not break the testcase you've linked, because you're linking directly to the image document. We will continue to support generalized multipart/x-mixed-replace for *documents*, including image documents. So navigating directly to the MJPEG stream, or embedding it in an <iframe>, will continue to work in the same way and support arbitrary changes like size changes.

We are planning to remove support for size changes for multipart/x-mixed-replace *content and CSS images*, which means basically that you will not be able to change the intrinsic size of a multipart/x-mixed-replace image in an <img> element or CSS background image.

In cases where you would've used an <img> element, if you need to support size changes you can embed the multipart/x-mixed-replace image document as an <iframe> instead.

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

That's true, and as I mentioned above, we will continue to support it for image documents.

There were many factors I considered when proposing this change for CSS and content images. Most importantly, supporting this feature requires a *tremendous* amount of complexity that makes debugging and maintenance much harder and makes it more difficult for us to implement new features. It's really not as simple as it might seem. (Consider, for example, that there is no other way for the intrinsic size of a CSS background image to change.) It's also not standardized (unlike multipart/x-mixed-replace support for documents, which we're continuing to support) and not universally supported across browsers, so we were to some extent "making up" what the behavior should be, and people who want a cross-browser solution can't use this approach no matter what.

Again, thanks for your feedback on this. I have not seen many uses of size changes together with MJPEG streams in the wild, so it helps a lot to hear from people that are actually using it and get an idea about what they're using it for.

Comment 12

[Daniel Kabs, reporting bugs since 2002]

Seth, 
Thanks for your detailed explanations. Just for the avoidance of misunderstanding, when you are talking about "image documents" and "image content": "Image document" means viewing the MJPEG stream directly (as in the testcase provided in comment #10). "image content" means embedding the image in a document using an <img> element. Of course, we would rather use the latter. To display the stream as an image in a document, use this link:

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

Now, how will the new implementation respond to size changes during streaming,
a) when the intrinsic JPEG stream size changes (server side change)
b) when the <img> element size is changed by manipulating its CSS height/width properties (client side change)?
As you know, FF35 (and before) respects both changes.

Thanks!

Comment 13

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

(In reply to Daniel Kabs, reporting bugs since 2002 from comment #12)
> a) when the intrinsic JPEG stream size changes (server side change)

The intrinsic size on the client side will definitely not change. There has been debate about what to do in this case; the current plan is to scale all incoming frames to the size of the first frame, but it's possible this will be revisited.

Options that would permit the intrinsic size to change include using <iframe> instead of <img>, or changing the <img> element's 'src' attribute (for example, from 'foo.jpg?100x100' to 'foo.jpg?200x200').

> b) when the <img> element size is changed by manipulating its CSS
> height/width properties (client side change)?

That will behave exactly the same as it does now.

Comment 14

[Daniel Kabs, reporting bugs since 2002]

Scaling all incoming frames to the size of the first frame, as you indicated in a), is fine, as the client (i.e. Firefox browser) usually initiates the change of the frame size. The client therefore has the information about the size change and can resize the <img> element, see b), accordingly. At least that's my understanding.

Comment 15

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

(In reply to Daniel Kabs, reporting bugs since 2002 from comment #14)
> Scaling all incoming frames to the size of the first frame, as you indicated
> in a), is fine, as the client (i.e. Firefox browser) usually initiates the
> change of the frame size. The client therefore has the information about the
> size change and can resize the <img> element, see b), accordingly. At least
> that's my understanding.

That would be a bad idea, because the frames will not be scaled to the size of the <img> element, but to the size of the first frame in the MJPEG stream. In other words, if the first frame of the MJPEG stream was 100x100, but you change the size of the stream so that later frames are 200x200 and change the <img> element's width to 200x200, here's what will happen:

1. Each incoming 200x200 frame is internally scaled down to 100x100.
2. When the 200x200 <img> element gets painted to the screen, the 100x100, scaled down version of the frame gets scaled up to 200x200.

The result is that you get no additional detail at all. You may as well just keep the size of the stream the same and just change the size of the <img> element.

The best way to handle size changes when displaying an MJPEG stream in an <img> element is to change the  'src' attribute whenever you change the size, as I suggested in comment 13. If you don't want to develop new code to handle this, you can just use the code that you use for supporting IE. I've taken a look at how your webcam software is implemented, and the code that you use for streaming JPEG support in IE will work perfectly fine with Firefox, and will be totally unaffected by this change.

Comment 16

[Daniel Kabs, reporting bugs since 2002]

Seth, Thanks for your explanations of the intricacies of the new implementation.

What exactly do you mean with "streaming JPEG support in IE"? To the best of my knowledge, IE does not support MJPEG based on multipart/x-mixed-replace content type. Therefore on IE, client side code has to "poll" (i.e. request) new images in order to update the displayed image. I believe it is obvious that polling images from a server is not as efficient as pushing images from the server to the client when a new image is available. In this respect, Firefox support for MJPEG is one of its advantages over IE.

Regarding the new implementation, can Javascript code (embedded in the webpage) query the current dimensions of the incoming MJPEG stream? This would allow for responding to size changes, e.g., as you suggested by changing the 'src' attribute of the <img> element (for example, in the simplest form from 'stream.jpg?1' to 'stream.jpg?2').

Comment 17

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

(In reply to Daniel Kabs, reporting bugs since 2002 from comment #16)
> What exactly do you mean with "streaming JPEG support in IE"? To the best of
> my knowledge, IE does not support MJPEG based on multipart/x-mixed-replace
> content type. Therefore on IE, client side code has to "poll" (i.e. request)
> new images in order to update the displayed image.

Yes, I'm referring to your code for streaming JPEG support in IE, which is implemented with polling. That approach will also work perfectly in Firefox. (Unlike IE, of course, you only need to change the 'src' attribute when a size change occurs. But you can simply change it for every frame to get the IE behavior.) In general I recommend that if you do not want to write any new code, just use your IE implementation.

If you don't want to use the IE behavior, another alternative would be to use Server-Sent Events to notify clients when the size changes, and update the 'src' attribute at that time. That approach does not require polling.

> Regarding the new implementation, can Javascript code (embedded in the
> webpage) query the current dimensions of the incoming MJPEG stream? This
> would allow for responding to size changes, e.g., as you suggested by
> changing the 'src' attribute of the <img> element (for example, in the
> simplest form from 'stream.jpg?1' to 'stream.jpg?2').

No, that will be impossible I'm afraid. The point of this change is to never allow the content type or intrinsic size of images to change, even if they're displaying an multipart/x-mixed-replace stream. That implies that JavaScript will also not be able to perceive such changes.

Comment 18

[Timothy Nikkel (:tnikkel)]

The assertion that multipart-mixed-replace is only used for jpeg is false. There are live sites that use it for gif at minimum.

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.