Randell Jesup [:jesup] (needinfo me) Assignee
	Description • 10 years ago

Our libjpeg-turbo doesn't include a patch applied to Chromium in 2011 (apparently not upstreamed...) to support Motion JPEG:

https://chromium.googlesource.com/chromium/deps/libjpeg_turbo/+/11a02ed81bfa320425301cb3439ea21bdaa1f5e7%5E!/

This patch allows JPEGs to omit the default huffman table, which is extremely common in USB cameras (apparently more so above 640x480 for some reason, perhaps to minimize bandwidth use).

To test, try a webcam at high resolution using media.navigator.video.default_width and media.navigator.video.default_height.  A Logitech C615 HD webcam fails at all resolutions above 640x480 without this patch.  (Note that the test page will show garbage data when the decode fails without a separate patch I've created to fix libyuv's MJPG decoder - it ignores errors from the decode process.)  With this patch it works; though with my Logitech it produces occasional errors like:

Corrupt JPEG data: 1 extraneous bytes before marker 0xd2
(libjpeg ignores these errors)

These seem to be the MJPEG encoder taking some shortcuts and/or bugs, and seems to have no impact on the frames.

(search for "mjpeg default huffman table" for other examples of why this default table is needed)

I would assume it's valid license-wise to import the patch to libjpeg (Gerv?)

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 2 • 10 years ago

Comment on attachment 8365492 [details] [diff] [review]
Add MJPEG support (default huffman tables) to libjpeg

r? to milan (and ted for the trivial moz.build change)

	(not currently active) Ted Mielczarek
	Comment 3 • 10 years ago

Comment on attachment 8365492 [details] [diff] [review]
Add MJPEG support (default huffman tables) to libjpeg

Review of attachment 8365492 [details] [diff] [review]:
-----------------------------------------------------------------

Feel free to use your judgement on moz.build changes, this is fairly trivial and I would have been fine without reviewing it (but I appreciate you erring on the side of caution).

	Gervase Markham [:gerv]
	Comment 4 • 10 years ago

No problem with importing this code.

Gerv

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 5 • 10 years ago

Comment on attachment 8365492 [details] [diff] [review]
Add MJPEG support (default huffman tables) to libjpeg

f+, and to Jeff for the actual review. FWIW, the "not using the first bit, so we'll just specify the rest and then copy from &bits[1]" may be worth a comment where the bits table is defined, though I probably would have done the array of 17 elements (what ffmpeg does). However, this is how the change went into chromium, so I guess matching them is good.

	Jeff Muizelaar [:jrmuizel]
	Comment 6 • 10 years ago

Comment on attachment 8365492 [details] [diff] [review]
Add MJPEG support (default huffman tables) to libjpeg

DRC,

How do you feel about taking this upstream?

	DRC
	Comment 7 • 10 years ago

This seems like a duplication of effort.  Can't you reuse the std_huff_tables() function in jcparam.c?  The function would need to be made non-static, of course.

	DRC
	Comment 8 • 10 years ago

Also, can you attach an image or two that exhibits this phenomenon?  If this is a general problem with decoding MJPEG frames, then I think libjpeg-turbo should support decoding them.  I don't see any reason to #ifdef the code.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 9 • 10 years ago

drc: the std_huff_tables function would need to be modified (it takes an encoder structure), or refactored to split out the table setup.

The ifdefs were because the Chromium patch had them (apparently they didn't try to upstream).  It is common for MJPEG, though I believe from reading that it in theory violates the spec.  But I think it makes sense to support.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 10 • 10 years ago

Alternate patch that reuses the std_huff_tables code and exposes it

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 12 • 10 years ago

Comment on attachment 8367533 [details] [diff] [review]
Add MJPEG support (default huffman tables) to libjpeg (alternate patch)

This patch may be better for upstreaming if we want to bake in MJPEG support.

Tested and works.  I'll note the saved frame loads in Chrome, and not in FF without this patch.

	DRC
	Comment 13 • 10 years ago

Comment on attachment 8367533 [details] [diff] [review]
Add MJPEG support (default huffman tables) to libjpeg (alternate patch)

This patch unfortunately breaks ABI compatibility.  I'll look at it in more detail and get back to you.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 14 • 10 years ago

I don't have to make it work this way - I can make jpeg_default_enc_huff_tables() and the same for debug, and keep the encode/decode structures unchanged.  That's definitely easier to justify (even if the code/API is less 'clean'); I wasn't thinking about ABI when I did this patch.  I also could just expose the static tables, and have the init functions LOCAL() to encode and decode.  Just say what you'd like.

Or we can live with a few extra bytes and take the Chromium patch.

	DRC
	Comment 15 • 10 years ago

I am working on a modified patch.  It's easy to avoid the ABI compatibility issue by casting either j_compress_ptr or j_decompress_ptr to j_common_ptr, passing it to jpeg_default_huff_table() like you are doing, but then reading the is_decompress field to determine whether to cast it back to j_compress_ptr or j_decompress_ptr.  You can then access the existing structure members for the Huffman tables.

To validate the patch, though, I need to successfully decode an MJPEG frame.  I tried the one you attached up there, but it gives:

Corrupt JPEG data: 6 extraneous bytes before marker 0xd1

This is true regardless of whether I use my patch or yours or the original one.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 16 • 10 years ago

And that's true: the camera appears to generate extraneous bytes at times.  I would expect others do too.  The image decodes correctly as the extraneous bytes are ignored.  That is what at least this MJPEG camera generates, and it's one of the primary manufacturers (Logitech) so I imagine many others do too.  It might be "accidental", or it might be on purpose to simplify or speed up internal processes.

	DRC
	Comment 17 • 10 years ago

OK, now that I know that, I can verify that it decodes properly after the warning is printed.

Before I submit my patch for review, though, I need to ask an existential question:  my understanding of motion JPEG is that it will send a frame with tables, then it will send one or more "abbreviated JPEG streams" without tables.  The decoder is expected to add the tables from the last full frame to the subsequent abbreviated streams.

Unless I'm mistaken about that, then it seems incorrect to set the tables to default values when an abbreviated stream is encountered.  Those tables should instead be set to the values from the last full frame, but since libjpeg-turbo has no idea what the application intends-- no idea whether it is going to do multiple decodes, no idea whether the frame being decoded is a full frame, no idea whether the lack of Huffman tables is intentional or an error-- it seems out of scope for us to be second guessing the application.  If an MJPEG decoder application intends to "do the right thing" and insert the tables itself using the ones from the last full frame, then setting them inside the library as well is just a waste of CPU time and memory.

If you are just trying to decode MJPEG frames using some reasonable guess, in the context of a browser, then the patch is harmless enough.  If you're trying to decode an actual video, then my understanding of the MJPEG spec is that this isn't doing the right thing, and if it's not doing the right thing, I don't want to include it upstream.

It would ease my mind to see what, for instance, ffmpeg does to handle this.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 18 • 10 years ago

From Wikipedia: 

    Criticisms

    Unlike the video formats specified in international standards such as MPEG-2 and the format specified in the JPEG still-picture coding standard, there is no document that defines a single exact format that is universally recognized as a complete specification of “Motion JPEG” for use in all contexts. This raises compatibility concerns about file outputs from different manufacturers.

and
http://www.faqs.org/faqs/jpeg-faq/part1/section-20.html
Most usefully: http://www.digitalpreservation.gov/formats/fdd/fdd000063.shtml

"Avery Lee, writing in the rec.video.desktop newsgroup in 2001, commented that "MJPEG, or at least the MJPEG in AVIs having the MJPG fourcc, is restricted JPEG with a fixed -- and *omitted* -- Huffman table. The JPEG must be YCbCr colorspace, it must be 4:2:2, and it must use basic Huffman encoding, not arithmetic or progressive. . . . You can indeed extract the MJPEG frames and decode them with a regular JPEG decoder, but you have to prepend the DHT segment to them, or else the decoder won't have any idea how to decompress the data. The exact table necessary is given in the OpenDML spec.'"

a) Chrome has done this since 2011 to support Motion JPEG cameras
b) other sources I found do the same thing

I'll try to check ffmpeg

	DRC
	Comment 19 • 10 years ago

As far as I can tell, ffmpeg uses the default tables unless instructed not to.  That's good enough for me.  Attaching a new patch.

	DRC
	Comment 20 • 10 years ago

The attached patch has been checked in upstream.  Please review it.  Issues that it corrects relative to the previous patches:

-- Reuses existing libjpeg code so as to avoid any licensing ambiguity.  It is difficult for us to accept code that is not properly attributed, because we don't know whose IP that code actually represents and whether the owner (usually a corporation) intended for it to be open sourced.  The libjpeg license is such that it requires any modifications relative to the original IJG code to be indicated as such, and I satisfy this by adjusting the copyright headers for the modified files accordingly.

-- Modifies the std_huff_tables() function so that it can work with both compressor and decompressor instances.

-- Moves that function and its associated tables into a separate C file that is included statically in both the compressor and decompressor.  I did this mainly to avoid any additional global symbols, because there are people in the community who monitor API/ABI changes from release to release and will flag libjpeg-turbo as having a changed API if additional globals show up, even if those aren't exposed in the API.  Actually adding the function to the API, as the alternate patch did, is problematic, because if applications decided to use the new function, then they would no longer be backward ABI compatible with libjpeg.

-- Does not modify the existing structures, so ABI compatibility is maintained.

-- Modifies the std_huff_tables() function so that it only initializes the tables if they aren't already initialized.  The original Chrome patch did this, but the alternate patch didn't.

-- Moves the call to std_huff_tables() into the Huffman decoder initialization.  This is done for two reasons:  (a) so that it won't be called whenever an arithmetic-codec JPEG is decompressed, and (b) so that it will be called in the body of jpeg_start_decompress() rather than jpeg_read_header().  This gives the application a chance to override the Huffman tables on its own by calling jpeg_read_header() and then examining whether the tables are NULL.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 21 • 10 years ago

Comment on attachment 8368666 [details] [diff] [review]
libjpeg-turbo-mjpeg-drc.patch

Review of attachment 8368666 [details] [diff] [review]:
-----------------------------------------------------------------

::: jstdhuff.c
@@ +8,5 @@
> +* For conditions of distribution and use, see the accompanying README file.
> +*
> +* This file contains routines to set the default Huffman tables, if they are
> +* not already set.
> +*/ 

Nit: trailing space (we care in our code, you may not)

@@ +21,5 @@
> +/* Define a Huffman table */
> +{
> +  int nsymbols, len;
> +
> +  if (*htblptr == NULL)

Maybe comment that this allows the application using this to override the standard tables

@@ +42,5 @@
> +
> +  MEMCOPY((*htblptr)->huffval, val, nsymbols * SIZEOF(UINT8));
> +
> +  /* Initialize sent_table FALSE so table will be written to JPEG file. */
> +  (*htblptr)->sent_table = FALSE;

Is this only relevant to the encoder?  Doesn't really matter though.

@@ +57,5 @@
> +  static const UINT8 bits_dc_luminance[17] =
> +    { /* 0-base */ 0, 0, 1, 5, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0 };
> +  static const UINT8 val_dc_luminance[] =
> +    { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11 };
> +  

ditto

@@ +62,5 @@
> +  static const UINT8 bits_dc_chrominance[17] =
> +    { /* 0-base */ 0, 0, 3, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, 0, 0 };
> +  static const UINT8 val_dc_chrominance[] =
> +    { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11 };
> +  

ditto

@@ +87,5 @@
> +      0xd5, 0xd6, 0xd7, 0xd8, 0xd9, 0xda, 0xe1, 0xe2,
> +      0xe3, 0xe4, 0xe5, 0xe6, 0xe7, 0xe8, 0xe9, 0xea,
> +      0xf1, 0xf2, 0xf3, 0xf4, 0xf5, 0xf6, 0xf7, 0xf8,
> +      0xf9, 0xfa };
> +  

ditto

	DRC
	Comment 22 • 10 years ago

Not sure what you mean by "ditto".  You are correct that sent_tables is only relevant to the encoder, but the tables are relevant to both the encoder and decoder, and the Chromium patch includes them as well.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 23 • 10 years ago

"ditto" was other trailing-space instances (they were immediately following it when I was making up the response, then I added some comments in the middle).  Nothing to be concerned with.

Jeff?  I'm ready to land this before the uplift.

	DRC
	Comment 24 • 10 years ago

Ah, OK.  I removed the trailing spaces upstream.  Several of those were inherited from the original libjpeg code.

	Jeff Muizelaar [:jrmuizel]
	Comment 25 • 10 years ago

(In reply to Randell Jesup [:jesup] from comment #23)
> 
> Jeff?  I'm ready to land this before the uplift.

Go for it.

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 26 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/fac849dd7be9

	Phil Ringnalda (:philor)
	Comment 27 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/fac849dd7be9

	Jean-Yves Perrier [:teoli]
	Comment 28 • 10 years ago

Just to understand for documentation purpose: we support the display of "static", individual MJPEG images, but not video streamed using multiple MJPEG frames over HTTP. Is this correct? Also do you think it is worth a relnote?

	Randell Jesup [:jesup] (needinfo me) Assignee
	Comment 29 • 10 years ago

This supports decoding of MJPEG frames.  This is separate from any issues around reception of multiple MJPEG frames that make up a video.  Depending on how they're delivered, that might or might not work (better chance than before).  This was to support captures of frames from MJPEG and to support MJPEG cameras for webrtc/getUserMedia()

	[SV Manager] Florin Mezei, QA (:FlorinMezei)
	Comment 30 • 10 years ago

I've reproduced the issue, using the attached picture, on Nightly from January 11 and Firefox 28.
The image loads properly though on Firefox 29 Beta 8, on Win 7 x64, Win 8 x64, Mac OS X 10.9.2 and Ubuntu 13.04 x64.