[FFmpeg-devel] [PATCH] Decoding of raw UTF-8 text from Ogg streams

[Benoit Fouet]

Hi,

On 2009-07-27 14:21, ogg.k.ogg.k at googlemail.com wrote:
> Feedback applied.
> Also, I set format to 1, but it seems to me that this field may be unneeded,
> as each AVSubtitleRect has its own type, allowing for mixed rectangles.
> If nobody knows that it's actually needed (except for API compatibility),
> it could maybe be removed ?
> 

> diff --git a/doc/APIchanges b/doc/APIchanges
> index 9434d55..37c26b7 100644
> --- a/doc/APIchanges
> +++ b/doc/APIchanges
> @@ -12,6 +12,9 @@ libavutil:   2009-03-08
>
>  API changes, most recent first:
>
> +2009-06-27 - ???? - lavf 52.35.0 - avcodec_free_subtitle
> +  New avcodec_free_subtitle API.
> +

why are you increasing lavf minor ? you're adding a decoder in lavc,
aren't you ?

> diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
> index 8eb7c3c..a2eee1b 100644
> --- a/libavcodec/avcodec.h
> +++ b/libavcodec/avcodec.h
> @@ -30,7 +30,7 @@
>  #include "libavutil/avutil.h"
>
>  #define LIBAVCODEC_VERSION_MAJOR 52
> -#define LIBAVCODEC_VERSION_MINOR 32
> +#define LIBAVCODEC_VERSION_MINOR 35
>  #define LIBAVCODEC_VERSION_MICRO  0
>

32 + 1 = 33 ;)

> diff --git a/libavcodec/ssadec.c b/libavcodec/ssadec.c
> new file mode 100644
> index 0000000..6f0e956
> --- /dev/null
> +++ b/libavcodec/ssadec.c
> @@ -0,0 +1,98 @@

[...]

> +static av_cold int ssa_decode_init(AVCodecContext *avccontext)
> +{
> +    uint8_t *headers = avccontext->extradata;
> +    int headers_len  = avccontext->extradata_size;
> +
> +    if (!headers_len || !headers) {
> +        av_log(avccontext, AV_LOG_ERROR, "Extradata corrupt.\n");
> +        return -1;
> +    }
> +
> +    /* not much to do with it, we're a simple decoder */
> +
> +    return 0 ;
> +}
>

I think you can remove this function

[...]

> +    sub->rects              = av_mallocz(sizeof(*sub->rects));
> +    if (!sub->rects)
> +        return buf_size;
> +    sub->rects[0]           = av_mallocz(sizeof(*sub->rects[0]));
> +    if (!sub->rects[0]) {
> +        av_free(sub->rects);
> +        return buf_size;
> +    }

for both: return AVERROR(ENOMEM);

> +    rect       = sub->rects[0];
> +    rect->type = SUBTITLE_ASS;
> +    rect->ass  = av_malloc(buf_size+1);
> +    if (!rect->ass) {
> +        av_free(sub->rects[0]);
> +        av_free(sub->rects);

add a return AVERROR(ENOMEM); here...

> +    }
> +    memcpy(rect->ass, buf, buf_size);

or you could get a segfault there.

> diff --git a/libavcodec/utils.c b/libavcodec/utils.c
> index 43147a5..beab9d3 100644
> --- a/libavcodec/utils.c
> +++ b/libavcodec/utils.c
> @@ -655,6 +655,32 @@ int avcodec_decode_subtitle2(AVCodecContext
> *avctx, AVSubtitle *sub,
>      return ret;
>  }
>
> +int avcodec_free_subtitle(AVCodecContext *avctx, AVSubtitle *sub)
> +{
> +    unsigned int n, d;
> +

you can move the declaration of d closer to where it's used

> +    if (!sub || !sub->num_rects || !sub->rects)
> +        return 0;
> +

IMHO, one of the two last tests is useless.

> +    for (n = 0; n < sub->num_rects; n++) {
> +        AVSubtitleRect *r = sub->rects[n];
> +        AVPicture *pic;
> +        if (r) {
> +            if (r->text)
> +                av_free(r->text);
> +            if (r->ass)
> +                av_free(r->ass);

leave the two if() out

> +            pic = &r->pict;
> +            for (d = 0; d < sizeof(pic->data) / sizeof(pic->data[0]);
> d++) {
> +                if (pic->data[n])
> +                    av_free(pic->data[n]);

same here, you can leave the if out.

Ben