[FFmpeg-devel] [PATCH 2/2] lavc/pngdec: decode textual data (tEXt and zTXt).

[Paul B Mahol]

On 10/29/12, Nicolas George <nicolas.george at normalesup.org> wrote:
> L'octidi 8 brumaire, an CCXXI, Paul B Mahol a ecrit :
>> > +    const uint8_t *data        = s->gb.buffer;
>> > +    const uint8_t *data_end    = data + length;
>> > +    const uint8_t *keyword     = data;
>> > +    const uint8_t *keyword_end = memchr(keyword, 0, data_end -
>> > keyword);
>> Why such complexity? data_end - keyword = length
>
> That is true in this particular case, but I want to keep the code generic
> because I am thinking of the iTXt chunk (I want to implement it if I can
> get
> my hands on a sample or produce one), where there are several 0-terminated
> strings.
>
> More generically, I believe it is good practice, in such a case, to work
> with a pointer to the end rather than a length: as we progress in the input
> data, the length needs to be updated while the pointer to the end is
> constant.
>
> I grant you it makes some overhead for the very simple cases.
>
>> > +    uint8_t *kw_utf8 = NULL, *text, *txt_utf8 = NULL;
>> > +    unsigned text_len;
>> > +    AVBPrint bp;
>> > +
>> > +    if (!keyword_end)
>> > +        return AVERROR_INVALIDDATA;
>> > +    data = keyword_end + 1;
>> Can keyword_end be same as data_end ?
>
> Unless I am mistaken, no, it can not: keyword_end is the result of a memchr
> ending at data_end: it either points to a byte in the buffer or is NULL.
>
>> > +        case MKTAG('t', 'E', 'X', 't'):
>> > +            if (decode_text_chunk(s, length, 0, &metadata) < 0)
>> > +                av_log(avctx, AV_LOG_WARNING, "Broken tEXt chunk\n");
>> > +            bytestream2_skip(&s->gb, length + 4);
>> > +            break;
>> > +        case MKTAG('z', 'T', 'X', 't'):
>> > +            if (decode_text_chunk(s, length, 1, &metadata) < 0)
>> > +                av_log(avctx, AV_LOG_WARNING, "Broken zTXt chunk\n");
>> > +            bytestream2_skip(&s->gb, length + 4);
>> > +            break;
>> nit: you could merge those two.
>
> I do not like the idea of testing the chunk type twice, once in the switch
> and a second time to distinguish the merged cases.

I did not meant that, but this:

case ztxt
  compressed =1
  /* fallthrough */
case text
  if (decode....
  ......
>
> Also, the iTXt chunk will likely require a slightly different treatment.
>
> I hope I have answered your objections: if there are no more remarks, I
> will
> take Michael's advice and push.

They are not objections, you could push long time ago....