After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 700773 - gstrtpjpegdepay.c -- index numbering of image components in SOF header off by one
gstrtpjpegdepay.c -- index numbering of image components in SOF header off by...
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
git master
Other Linux
: Normal critical
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2013-05-21 11:27 UTC by midu
Modified: 2013-06-07 08:07 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
header (2.12 KB, application/octet-stream)
2013-05-22 16:49 UTC, midu
Details
source (27.09 KB, application/octet-stream)
2013-05-22 16:50 UTC, midu
Details
polished source (24.05 KB, application/octet-stream)
2013-05-23 14:51 UTC, midu
Details
JPEG with standard luma and chroma coefficients (572.39 KB, image/png)
2013-06-06 11:34 UTC, midu
Details
JPEG with luma/chroma coefficients taken from live555 (514.34 KB, image/png)
2013-06-06 11:36 UTC, midu
Details
rtspsrc ! gdppay ! filesink (637.15 KB, application/octet-stream)
2013-06-06 14:46 UTC, midu
Details
fixed: rtspsrc ! gdppay ! filesink (743.68 KB, application/octet-stream)
2013-06-06 14:47 UTC, midu
Details

Description midu 2013-05-21 11:27:29 UTC
Applies to both gstreamer 0.10 and 1.0

Error description
-----------------
 
GStreamer fails to play MJPEG videos over RTP, 
i.e. RTP+JPEG according to RFC2435.

Sample display pipelines
------------------------

-- RTSP, RTP unicast
$ gst-launch rtspsrc debug=true location=rtsp://bla ! rtpjpegdepay ! jpegdec ! xvimagesink
$ gst-launch rtspsrc debug=true location=rtsp://bla ! rtpjpegdepay ! ffdec_mjpeg ! xvimagesink

-- RTP unicast
$ gst-launch rtpsrc uri=rtp://bla ! rtpjpegdepay ! jpegdec ! xvimagesink
$ gst-launch rtpsrc uri=rtp://bla ! rtpjpegdepay ! ffdec_mjpeg ! xvimagesink


Remedy
------

in gst-plugins-good/gst/rtp/gstrtpjpegdepay.c:MakeHeaders()

-  *p++ = 0;                     /* comp 0 */
+  *p++ = 1;                     /* comp 0 */
  if ((type & 0x3f) == 0)
    *p++ = 0x21;                /* hsamp = 2, vsamp = 1 */
  else
    *p++ = 0x22;                /* hsamp = 2, vsamp = 2 */
  *p++ = 0;                     /* quant table 0 */
-  *p++ = 1;                     /* comp 1 */
+  *p++ = 2;                     /* comp 1 */
  *p++ = 0x11;                  /* hsamp = 1, vsamp = 1 */
  *p++ = 1;                     /* quant table 1 */
-  *p++ = 2;                     /* comp 2 */
+  *p++ = 3;                     /* comp 2 */
  *p++ = 0x11;                  /* hsamp = 1, vsamp = 1 */
  *p++ = 1;                     /* quant table 1 */

Component table indexing starts at One, not Zero, at least mjpegdec.c of libav
expects it this way. The standard (ISO/IEC 10918-1) depicts start of indexing at One, albeit only graphically. 
Frame header parameters as stated in Table B.2 allow component index values in the range from 0-255 nonetheless. 

Remaining Bugs
--------------
Still poor image quality in comparison to vlc visualization. 
Chroma seems to be off (displaced by 1-4 pix)
Double checked AC luma/chroma coefficients, Quantization tables and the likes to no avail.
Comment 1 midu 2013-05-22 16:48:22 UTC
-- RTP unicast (another one)
$ gst-launch uridecodebin uri=rtp://bla ! xvimagesink


Poor Image Quality
------------------
Apparently luma/chroma quantization coefficients other than defined 
by ISO/IEC 10918-1 are more suitable.

Did some copy+paste from tables found within the live lib that is employed
by vlc. These work much better, don't know about their rationale, though.
see: http://www.live555.com/liveMedia, JPEGVideoRTPSource.cpp


Remaining Bugs
--------------
none.
Comment 2 midu 2013-05-22 16:49:55 UTC
Created attachment 245069 [details]
header
Comment 3 midu 2013-05-22 16:50:38 UTC
Created attachment 245071 [details]
source
Comment 4 midu 2013-05-23 13:35:19 UTC
In gst_rtp_jpeg_depay_process():

Throw out everything between ..

/* start scan for leading JPEG header garbage
.. and ..
/* end scan for leading JPEG header garbage */


.. and out-comment superfluous debugging tidbits:

    guint8 header[1024];
/*    
    guchar strheader[1024 * 3 + 1];
    memset (strheader, 0, sizeof (strheader));
    GST_INFO_OBJECT (rtpjpegdepay,
        "JPEG header param type %d, precision %d, dri %d", type, precision,
        dri);
*/        
    size = MakeHeaders (header, type, width, height, qtable, precision, dri);
    g_byte_array_append (rtpjpegdepay->hold_buf, header,
        size < 1024 ? size : 1024);
/*
    GST_INFO_OBJECT (rtpjpegdepay, "JPEG header size %d, start of image? %s",
        size, header[0] == 0xff && header[1] == 0xd8 ? "Yup" : "Nope");
*/
    rtpjpegdepay->len_header = size;

/*    
    for (int k = 0; k < size; k++)
      sprintf (strheader + k * 3, "%02x ", header[k]);
    GST_INFO_OBJECT (rtpjpegdepay, "%s", strheader);
*/
Comment 5 midu 2013-05-23 14:51:21 UTC
Created attachment 245141 [details]
polished source
Comment 6 Wim Taymans 2013-05-27 12:52:25 UTC
> Sample display pipelines
> ------------------------
>
> -- RTP unicast
> $ gst-launch rtpsrc uri=rtp://bla ! rtpjpegdepay ! jpegdec ! xvimagesink
> $ gst-launch rtpsrc uri=rtp://bla ! rtpjpegdepay ! ffdec_mjpeg ! xvimagesink

These pipelines don't make sense, rtspsrc does not have a uri property and the rtp protocol is not defined in GStreamer.

>
> component table indexing starts at One, not Zero, at least mjpegdec.c of libav
> expects it this way. The standard (ISO/IEC 10918-1) depicts start of indexing
> at One, albeit only graphically. 
> Frame header parameters as stated in Table B.2 allow component index values in
> the range from 0-255 nonetheless. 

They are just IDs, any number is fine, the RFC example code starts from 0, I see many encoders start from 1. Either way it should work.

> Poor Image Quality
> ------------------
> Apparently luma/chroma quantization coefficients other than defined 
> by ISO/IEC 10918-1 are more suitable.
>
> Did some copy+paste from tables found within the live lib that is employed
> by vlc. These work much better, don't know about their rationale, though.
> see: http://www.live555.com/liveMedia, JPEGVideoRTPSource.cpp

Those tables are already in zigzag order. We store the ISO tables and apply the
zigzag order later. So, this code is equivalent.

Let's start from the start. Could you make available the output (out.gdp) of the following pipeline:

gst-launch rtspsrc debug=true location=rtsp://bla ! gdppay ! filesink location=out.gdp
Comment 7 midu 2013-06-06 11:34:05 UTC
Created attachment 246146 [details]
JPEG with standard luma and chroma coefficients
Comment 8 midu 2013-06-06 11:36:09 UTC
Created attachment 246147 [details]
JPEG with luma/chroma coefficients taken from live555
Comment 9 Marc Leeman 2013-06-06 11:40:21 UTC
> > -- RTP unicast
> > $ gst-launch rtpsrc uri=rtp://bla ! rtpjpegdepay ! jpegdec ! xvimagesink
> > $ gst-launch rtpsrc uri=rtp://bla ! rtpjpegdepay ! ffdec_mjpeg ! xvimagesink
> 
> These pipelines don't make sense, rtspsrc does not have a uri property and the
> rtp protocol is not defined in GStreamer.

Yes, the bug should be adjusted to use udpsrc; the effect will remain the same. rtpsrc (and rtpsink) are bins we wrote around udpsrc in order to be able to reuse the sockets on port+1 for RTCP. Those bins register the rtp:// protocol.
Comment 10 midu 2013-06-06 14:46:25 UTC
Created attachment 246166 [details]
rtspsrc ! gdppay ! filesink
Comment 11 midu 2013-06-06 14:47:34 UTC
Created attachment 246168 [details]
fixed: rtspsrc ! gdppay ! filesink
Comment 12 Wim Taymans 2013-06-06 15:24:03 UTC
Both streams work fine for me, what's the problem? I tried:

gst-launch-1.0 filesrc location=/home/wim/Downloads/fixed-out.gdp ! gdpdepay ! decodebin ! xvimagesink
gst-launch-1.0 filesrc location=/home/wim/Downloads/fixed-out.gdp ! gdpdepay ! rtpjpegdepay ! jpegdec ! xvimagesink
gst-launch-1.0 filesrc location=/home/wim/Downloads/fixed-out.gdp ! gdpdepay ! rtpjpegdepay ! avdec_mjpeg ! xvimagesink

same with the faulty-out.gdp. What's the difference between those two files?
Comment 13 midu 2013-06-07 07:19:00 UTC
Seems that the issue only applies to GStreamer 0.10. In fact, quantization tables in 1.0 have changed to those I have found in live555, presumably defined in 10918-5. Currently I cannot reproduce my notion that 1.0 is affected to. Way to much repositories I'm pulling from, I guess. Component indexing starting from One is a non-issue in 1.0 as well. Iirc a possibly outdated jpeg lib decremented the component numbering by one to denote the maximum index. 
You may close that bug as resolved / bogus at this point.