Subject: Re: [AVTCORE] [Technical Errata Reported] RFC2435 (4095)

From: Magnus Westerlund <[email-masked]>
Date: Thu, 29 Jan 2015 14:00:35 +0100

WG,

I would like to have your feedback on this errata.

It appears to me that the code is hard coded for three components. That
should be pretty evident for anyone reading the comments.

So from that perspective Julius is correct that the code was not taking
single component images into account. However, it is difficult to
determine the intention here.

I also wonder if the change is correct, especially why the first
occurrence of this line:

*p++ = 3;               /* number of components */

Is still equals to 3.

So WG is this Held for update or Reject.

Cheers

Magnus

On 2014-09-04 16:52, RFC Errata System wrote:
> The following errata report has been submitted for RFC2435,
> "RTP Payload Format for JPEG-compressed Video".
> 
> --------------------------------------
> You may review the report below and at:
> http://www.rfc-editor.org/errata_search.php?rfc=2435&eid=4095
> 
> --------------------------------------
> Type: Technical
> Reported by: Julius Richard Friedman <[email-masked]>
> 
> Section: Appendix B
> 
> Original Text
> -------------
> int MakeHeaders(u_char *p, int type, int w, int h, u_char *lqt,
>                 u_char *cqt, u_short dri)
> {
>         u_char *start = p;
> 
>         /* convert from blocks to pixels */
>         w <<= 3;
>         h <<= 3;
> 
> 
> 
> Berc, et. al.               Standards Track                    [Page 19]
>  
> RFC 2435              RTP Payload Format for JPEG           October 1998
> 
> 
>         *p++ = 0xff;
>         *p++ = 0xd8;            /* SOI */
> 
>         p = MakeQuantHeader(p, lqt, 0);
>         p = MakeQuantHeader(p, cqt, 1);
> 
>         if (dri != 0)
>                 p = MakeDRIHeader(p, dri);
> 
>         *p++ = 0xff;
>         *p++ = 0xc0;            /* SOF */
>         *p++ = 0;               /* length msb */
>         *p++ = 17;              /* length lsb */
>         *p++ = 8;               /* 8-bit precision */
>         *p++ = h >> 8;          /* height msb */
>         *p++ = h;               /* height lsb */
>         *p++ = w >> 8;          /* width msb */
>         *p++ = w;               /* wudth lsb */
>         *p++ = 3;               /* number of components */
>         *p++ = 0;               /* comp 0 */
>         if (type == 0)
>                 *p++ = 0x21;    /* hsamp = 2, vsamp = 1 */
>         else
>                 *p++ = 0x22;    /* hsamp = 2, vsamp = 2 */
>         *p++ = 0;               /* quant table 0 */
>         *p++ = 1;               /* comp 1 */
>         *p++ = 0x11;            /* hsamp = 1, vsamp = 1 */
>         *p++ = 1;               /* quant table 1 */
>         *p++ = 2;               /* comp 2 */
>         *p++ = 0x11;            /* hsamp = 1, vsamp = 1 */
>         *p++ = 1;               /* quant table 1 */
>         p = MakeHuffmanHeader(p, lum_dc_codelens,
>                               sizeof(lum_dc_codelens),
>                               lum_dc_symbols,
>                               sizeof(lum_dc_symbols), 0, 0);
>         p = MakeHuffmanHeader(p, lum_ac_codelens,
>                               sizeof(lum_ac_codelens),
>                               lum_ac_symbols,
>                               sizeof(lum_ac_symbols), 0, 1);
>         p = MakeHuffmanHeader(p, chm_dc_codelens,
>                               sizeof(chm_dc_codelens),
>                               chm_dc_symbols,
>                               sizeof(chm_dc_symbols), 1, 0);
>         p = MakeHuffmanHeader(p, chm_ac_codelens,
>                               sizeof(chm_ac_codelens),
>                               chm_ac_symbols,
>                               sizeof(chm_ac_symbols), 1, 1);
> 
> 
> 
> 
> Berc, et. al.               Standards Track                    [Page 20]
>  
> RFC 2435              RTP Payload Format for JPEG           October 1998
> 
> 
>         *p++ = 0xff;
>         *p++ = 0xda;            /* SOS */
>         *p++ = 0;               /* length msb */
>         *p++ = 12;              /* length lsb */
>         *p++ = 3;               /* 3 components */
>         *p++ = 0;               /* comp 0 */
>         *p++ = 0;               /* huffman table 0 */
>         *p++ = 1;               /* comp 1 */
>         *p++ = 0x11;            /* huffman table 1 */
>         *p++ = 2;               /* comp 2 */
>         *p++ = 0x11;            /* huffman table 1 */
>         *p++ = 0;               /* first DCT coeff */
>         *p++ = 63;              /* last DCT coeff */
>         *p++ = 0;               /* sucessive approx. */
> 
>         return (p - start);
> };
> 
> 
> Corrected Text
> --------------
> int MakeHeaders(u_char *p, int type, int w, int h, u_char *lqt,
>                 u_char *cqt, u_short dri)
> {
>         u_char *start = p;
> 
>         /* convert from blocks to pixels */
>         w <<= 3;
>         h <<= 3;
>         *p++ = 0xff;
>         *p++ = 0xd8;            /* SOI */
> 
>         p = MakeQuantHeader(p, lqt, 0);
>         if(cqt != NULL) p = MakeQuantHeader(p, cqt, 1);
> 
>         if (dri != 0)
>                 p = MakeDRIHeader(p, dri);
> 
>         *p++ = 0xff;
>         *p++ = 0xc0;            /* SOF */
>         *p++ = 0;               /* length msb */
>         *p++ = 17;              /* length lsb */
>         *p++ = 8;               /* 8-bit precision */
>         *p++ = h >> 8;          /* height msb */
>         *p++ = h;               /* height lsb */
>         *p++ = w >> 8;          /* width msb */
>         *p++ = w;               /* wudth lsb */
>         *p++ = 3;               /* number of components */
>         *p++ = 1;               /* comp 1 */
>         if (type == 0)
>                 *p++ = 0x21;    /* hsamp = 2, vsamp = 1 */
>         else
>                 *p++ = 0x22;    /* hsamp = 2, vsamp = 2 */
>         *p++ = 0;               /* quant table 0 */
>         *p++ = 1;               /* comp 1 */
>         *p++ = 0x11;            /* hsamp = 1, vsamp = 1 */
>         *p++ = 1;               /* quant table 1 */
>         *p++ = 2;               /* comp 2 */
>         *p++ = 0x11;            /* hsamp = 1, vsamp = 1 */
>         *p++ = 1;               /* quant table 1 */
>         p = MakeHuffmanHeader(p, lum_dc_codelens,
>                               sizeof(lum_dc_codelens),
>                               lum_dc_symbols,
>                               sizeof(lum_dc_symbols), 0, 0);
>         p = MakeHuffmanHeader(p, lum_ac_codelens,
>                               sizeof(lum_ac_codelens),
>                               lum_ac_symbols,
>                               sizeof(lum_ac_symbols), 0, 1);
>         if(cqt != NULL)
>         {
>         p = MakeHuffmanHeader(p, chm_dc_codelens,
>                               sizeof(chm_dc_codelens),
>                               chm_dc_symbols,
>                               sizeof(chm_dc_symbols), 1, 0);
>         p = MakeHuffmanHeader(p, chm_ac_codelens,
>                               sizeof(chm_ac_codelens),
>                               chm_ac_symbols,
>                               sizeof(chm_ac_symbols), 1, 1);
>        }
> 
> 
> 
> 
> Berc, et. al.               Standards Track                    [Page 20]
>  
> RFC 2435              RTP Payload Format for JPEG           October 1998
> 
> 
>         *p++ = 0xff;
>         *p++ = 0xda;            /* SOS */
>         *p++ = 0;               /* length msb */
>         *p++ = cqt != NULL ? 0x12 : 0x0b;/* length lsb */
>         *p++ = cqt != NULL ? 0x03 : 0x01;/* 3 components */
>         *p++ = 0;               /* comp 0 */
>         *p++ = 0;               /* huffman table 0 */
>         *p++ = 0x01;               /* comp 1 */
>         *p++ = cqt != NULL ? 0x11 : 0x00;/* huffman table 1 */
>         if(cqt != NULL) *p++ = 2;/* comp 2 */
>         *p++ = cqt != NULL ? 0x11 : 0x00;/* huffman table 1 */
>         *p++ = 0;               /* first DCT coeff */
>         *p++ = 63;              /* last DCT coeff */
>         *p++ = 0;               /* sucessive approx. */
> 
>         return (p - start);
> };
> 
> 
> Notes
> -----
> Did not take into account cases with only 1 component was used.
> 
> Instructions:
> -------------
> This erratum is currently posted as "Reported". If necessary, please
> use "Reply All" to discuss whether it should be verified or
> rejected. When a decision is reached, the verifying party (IESG)
> can log in to change the status and edit the report, if necessary. 
> 
> --------------------------------------
> RFC2435 (no draft string recorded)
> --------------------------------------
> Title               : RTP Payload Format for JPEG-compressed Video
> Publication Date    : October 1998
> Author(s)           : L. Berc, W. Fenner, R. Frederick, S. McCanne, P. Stewart
> Category            : PROPOSED STANDARD
> Source              : Audio/Video Transport
> Area                : Real-time Applications and Infrastructure
> Stream              : IETF
> Verifying Party     : IESG
> 
> _______________________________________________
> Audio/Video Transport Core Maintenance
> [email-masked]
> https://www.ietf.org/mailman/listinfo/avt
> 

-- 

Magnus Westerlund

----------------------------------------------------------------------
Services, Media and Network features, Ericsson Research EAB/TXM
----------------------------------------------------------------------
Ericsson AB                 | Phone  +46 10 7148287
Färögatan 6                 | Mobile +46 73 0949079
SE-164 80 Stockholm, Sweden | mailto: [email-masked]
----------------------------------------------------------------------

---

From: [email-masked] (Dale R. Worley)
Date: Thu, 29 Jan 2015 21:28:38 -0500

> determine the intention here.

The code change in the erratum is:

    --- says	2015-01-29 21:21:01.578501194 -0500
    +++ should-say	2015-01-29 21:20:32.139678733 -0500
    @@ -1,4 +1,4 @@
    -Section Appendix B says:
    +It should say:

    @@ -12,7 +12,7 @@
             *p++ = 0xd8;            /* SOI */

             p = MakeQuantHeader(p, lqt, 0);
    -        p = MakeQuantHeader(p, cqt, 1);
    +        if(cqt != NULL) p = MakeQuantHeader(p, cqt, 1);

             if (dri != 0)
    @@ -27,7 +27,7 @@
    -        *p++ = 0;               /* comp 0 */
    +        *p++ = 1;               /* comp 1 */
    @@ -47,6 +47,8 @@
    +        if(cqt != NULL)
    +        {
    @@ -55,6 +57,7 @@
    +       }

    @@ -67,14 +70,14 @@
             *p++ = 0xff;
             *p++ = 0xda;            /* SOS */
             *p++ = 0;               /* length msb */
    -        *p++ = 12;              /* length lsb */
    -        *p++ = 3;               /* 3 components */
    +        *p++ = cqt != NULL ? 0x12 : 0x0b;/* length lsb */
    +        *p++ = cqt != NULL ? 0x03 : 0x01;/* 3 components */
             *p++ = 0;               /* comp 0 */
             *p++ = 0;               /* huffman table 0 */
    -        *p++ = 1;               /* comp 1 */
    -        *p++ = 0x11;            /* huffman table 1 */
    -        *p++ = 2;               /* comp 2 */
    -        *p++ = 0x11;            /* huffman table 1 */
    +        *p++ = 0x01;               /* comp 1 */
    +        *p++ = cqt != NULL ? 0x11 : 0x00;/* huffman table 1 */
    +        if(cqt != NULL) *p++ = 2;/* comp 2 */
    +        *p++ = cqt != NULL ? 0x11 : 0x00;/* huffman table 1 */
             *p++ = 0;               /* first DCT coeff */
             *p++ = 63;              /* last DCT coeff */
             *p++ = 0;               /* sucessive approx. */

I don't understand the code, but it appears that the "should-say" code is
intended to work as the "says" code works when cqt is not NULL.

However, this change is always operative:

    -        *p++ = 0;               /* comp 0 */
    +        *p++ = 1;               /* comp 1 */

And this change is peculiar, in that the first character assigned to p
changes from 12 to a choice between 18 (0x12) and 11 (0xB).  I wonder if
0x12 is intended to be 12?

    -        *p++ = 12;              /* length lsb */
    -        *p++ = 3;               /* 3 components */
    +        *p++ = cqt != NULL ? 0x12 : 0x0b;/* length lsb */
    +        *p++ = cqt != NULL ? 0x03 : 0x01;/* 3 components */

Dale

---

From: Julius Friedman <[email-masked]>
Date: Thu, 29 Jan 2015 21:35:55 -0500

Magnus has my input,   I will also include it here in case I was supposed
to.

Here is my input and explanation for 4095.

The implementation I have is similar to the annex code I changed and uses
the number of components corresponding to those from the jpeg SOS marker.

The functionality was already hardcoded for two tables and the case of 1
table all the data probably belongs to only the luma table but the table
class decided that.

The function definition could be changed to give an array of each
components but I believe the maximum number of components is 4 anyway and
because its a 4 bit value i ignored it.

This goes back to being compatible and why I opted to change the annex the
way I did.

Cases of 0 and 1 table are now handled.

Cases of 2 tables were previously handled (although if they use different
sizes tables without precision the given code will also fail although im
not sure this is typically seen as a table usually has either 64 or 128
octets and the same number for each other table generated. )

Cases of 3 also now handled. (With th above notion in mind about all tables
being the same length or requiring precision)

Cases of 4 tables should also be handled in my opinion if they are not ;
but you can't have a value 4 there...

I believe the code used the three tables to determine if there were any
more remaining but I may have omitted that code from the erratum.

In short it may be beneficial to change the function definition to give
those components and their lengths; if that decision is made the result
will be easier to follow but I also defer there for your opinion.

There is also a problem using the given standard table on progressive
encoded images.

I have answered another question related to this here which may make more
sense.

http://stackoverflow.com/questions/25662584/how-to-convert-a-jpeg-444-ycbcr-to-422-ycbcr/25676532#25676532

My code is which is working with this method is @
http://net7mma.codeplex.com/SourceControl/latest#RtspServer/MediaTypes/RFC2435Media.cs

The project also has a number of jpeg images to test on which fail in the
annex code, two of which are argb and cmyk.

The latter two would require some custom sdp and the header or other
informat to be given explicitly there and it should be noted that I am not
arguing for those formats, just the ones allowed by the baseline
specification which includes progressive.

I do however feel that any updates to the document should allow for any
jpeg, onvif does this differently and it may work looking at how they do it
to ensure compatible implementations are possible.

http://www.onvif.org/specs/stream/ONVIF-Streaming-Spec-v210.pdf

Thanks,
Julius

             if (dri != 0)

    @@ -67,14 +70,14 @@
             *p++ = 0xff;
             *p++ = 0xda;            /* SOS */

    -        *p++ = 0;               /* comp 0 */
    +        *p++ = 1;               /* comp 1 */

Dale