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

[Julius Friedman <[e-mail]>]

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
Magnus Westerlund <[e-mail]> writes:
> 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.

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:

     int MakeHeaders(u_char *p, int type, int w, int h, u_char *lqt,
                     u_char *cqt, u_short dri)
    @@ -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)
                     p = MakeDRIHeader(p, dri);
    @@ -27,7 +27,7 @@
             *p++ = w >> 8;          /* width msb */
             *p++ = w;               /* wudth lsb */
             *p++ = 3;               /* number of components */
    -        *p++ = 0;               /* comp 0 */
    +        *p++ = 1;               /* comp 1 */
             if (type == 0)
                     *p++ = 0x21;    /* hsamp = 2, vsamp = 1 */
             else
    @@ -47,6 +47,8 @@
                                   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,
    @@ -55,6 +57,7 @@
                                   sizeof(chm_ac_codelens),
                                   chm_ac_symbols,
                                   sizeof(chm_ac_symbols), 1, 1);
    +       }



    @@ -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