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

[[e-mail] (Dale R. Worley)]

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