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

[Paul B Mahol]

On 10/28/12, Nicolas George <nicolas.george at normalesup.org> wrote:
> Requested in trac ticket #1857.
>
> Signed-off-by: Nicolas George <nicolas.george at normalesup.org>
> ---
>  libavcodec/pngdec.c |  131
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 131 insertions(+)
>
[...]
> +    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

> +    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 ?
> +
> +    if (compressed) {
> +        if (data == data_end)
> +            return AVERROR_INVALIDDATA;
> +        method = *(data++);
> +        if (method)
> +            return AVERROR_INVALIDDATA;
> +        if ((ret = decode_zbuf(&bp, data, data_end)) < 0)
> +            return ret;
> +        text_len = bp.len;
> +        av_bprint_finalize(&bp, (char **)&text);
> +        if (!text)
> +            return AVERROR(ENOMEM);
> +    } else {
> +        text = (uint8_t *)data;
> +        text_len = data_end - text;
> +    }
> +
> +    kw_utf8  = iso88591_to_utf8(keyword, keyword_end - keyword);
> +    txt_utf8 = iso88591_to_utf8(text, text_len);
> +    if (text != data)
> +        av_free(text);
> +    if (!(kw_utf8 && txt_utf8)) {
> +        av_free(kw_utf8);
> +        av_free(txt_utf8);
> +        return AVERROR(ENOMEM);
> +    }
> +
> +    av_dict_set(dict, kw_utf8, txt_utf8,
> +                AV_DICT_DONT_STRDUP_KEY | AV_DICT_DONT_STRDUP_VAL);

Looks wrong.
> +    return 0;
> +}
> +
>  static int decode_frame(AVCodecContext *avctx,
>                          void *data, int *data_size,
>                          AVPacket *avpkt)
> @@ -395,6 +512,7 @@ static int decode_frame(AVCodecContext *avctx,
>      PNGDecContext * const s = avctx->priv_data;
>      AVFrame *picture = data;
>      AVFrame *p;
> +    AVDictionary *metadata = NULL;
>      uint8_t *crow_buf_base = NULL;
>      uint32_t tag, length;
>      int64_t sig;
> @@ -617,6 +735,16 @@ static int decode_frame(AVCodecContext *avctx,
>                  bytestream2_skip(&s->gb, 4); /* crc */
>              }
>              break;
> +        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.

>          case MKTAG('I', 'E', 'N', 'D'):
>              if (!(s->state & PNG_ALLIMAGE))
>                  av_log(avctx, AV_LOG_ERROR, "IEND without all image\n");
> @@ -712,6 +840,8 @@ static int decode_frame(AVCodecContext *avctx,
>          }
>      }
>
> +    s->current_picture->metadata = metadata;
> +    metadata = NULL;
>      *picture= *s->current_picture;
>      *data_size = sizeof(AVFrame);
>
> @@ -724,6 +854,7 @@ static int decode_frame(AVCodecContext *avctx,
>      av_freep(&s->tmp_row);
>      return ret;
>   fail:
> +    av_dict_free(&metadata);
>      ret = -1;
>      goto the_end;
>  }
> --
> 1.7.10.4
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>