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

[Magnus Westerlund <[e-mail]>]

WG,

My decision is to recommend to the AD that this is Rejected based on
that the code change attempts to address handling images that the
payload format can't support as currently specified. Instead a new
update RTP payload specification that has extended functionality are
needed to address this issue.

Cheers

Magnus Westerlund

On 2015-01-30 03:35, Julius Friedman wrote:
> 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]
> <mailto:[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


-- 

Magnus Westerlund

----------------------------------------------------------------------
Services, Media and Network features, Ericsson Research EAB/TXM
----------------------------------------------------------------------
Ericsson AB                 | Phone  [phone]
Färögatan 6                 | Mobile [phone]
SE-164 80 Stockholm, Sweden | mailto: [e-mail]
----------------------------------------------------------------------