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

[Nicolas George]

L'octidi 8 brumaire, an CCXXI, Paul B Mahol a écrit :
> > +    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.

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.

Regards,

-- 
  Nicolas George
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20121029/c9e285b3/attachment.asc>