[FFmpeg-devel] [PATCH]Basic XSUB encoder (take 5)

[Björn Axelsson]

On Fri, 6 Feb 2009, Michael Niedermayer wrote:

[...]

> a patch adding a check in avcodec_encode_subtitle() that start_display_time is
> 0 is welcome!

Patch attached (subtitle_check_start_display_time.diff).

[...]

> > +
> > +/** Encode a single color run. At most 16 bits will be used. */
> > +static void put_xsub_rle(PutBitContext *pb, int len, int color)
> > +{
> > +    if (len <= 255)
>
> > +        put_bits(pb, 2 + ((ff_log2_tab[len] >> 1) << 2), len);
>

> do you really have to access the table directly? cant av_log2() be used?

I don't have to, I guess. But both av_log2() and even av_log2_16bit() adds
some extra checks that I don't need and that I don't know if the compiler
can optimize away.

[...]

> > +    if (h->num_rects == 0 || h->rects == NULL)
>
> !h->rects
>
>
> > +        return -1;
>
> also this check can be moved to avcodec_encode_subtitle() i think

Patch attached (subtitle_check_rects.diff).

[...]

> > +    if (make_tc(startTime, start_tc) || make_tc(endTime, end_tc)) {
> > +        av_log(avctx, AV_LOG_WARNING, "Time code >= 100 hours.\n");
> > +        return -1;
> > +    }
>
> do you have a official player? i so what happens with a larger timecode ?

It crashes the official player on my PS3. Case closed?

[...]

> > +    bytestream_put_le16(&rlelenptr, put_bits_count(&pb)/8); // Len of first field
>
> >>8

I hope you mean >>3?

[...]

> > Index: libavformat/avienc.c
> > ===================================================================
> > --- libavformat/avienc.c.orig	2009-02-05 21:17:03.000000000 +0100
> > +++ libavformat/avienc.c	2009-02-05 21:17:36.000000000 +0100
> > @@ -82,6 +82,9 @@
> >      if (type == CODEC_TYPE_VIDEO) {
> >          tag[2] = 'd';
> >          tag[3] = 'c';
> > +    } else if (type == CODEC_TYPE_SUBTITLE) {
> > +        tag[2] = 's';
> > +        tag[3] = 'b';
> >      } else {
> >          tag[2] = 'w';
> >          tag[3] = 'b';
> > @@ -213,8 +216,10 @@
> >          case CODEC_TYPE_AUDIO: put_tag(pb, "auds"); break;
> >  //        case CODEC_TYPE_TEXT : put_tag(pb, "txts"); break;
> >          case CODEC_TYPE_DATA : put_tag(pb, "dats"); break;
> > +        case CODEC_TYPE_SUBTITLE: put_tag(pb, "vids"); break;
> >          }
> > -        if(stream->codec_type == CODEC_TYPE_VIDEO)
>
> > +        if(stream->codec_type == CODEC_TYPE_VIDEO
> > +                || stream->codec_type == CODEC_TYPE_SUBTITLE)
> >              put_le32(pb, stream->codec_tag);
> >          else
> >              put_le32(pb, 1);
>
> this may be wrong as we have a ('t', 'x', 't', 's') in avidec as well so
> not all might be vids

At the moment "txts" is handled as CODEC_TYPE_DATA in avidec, so there's
no risk for immediate breakage. I think...

My knowledge about the avi (de)muxer is very limited, but maybe I could
add CODEC_CAP_SUBTITLE_{BITMAP | TEXT | ASS} and do something like

case CODEC_TYPE_SUBTITLE:
    if(stream->capabilities & CODEC_CAP_SUBTITLE_BITMAP)
       put_tag(pb, "vids");
    else
       put_tag(pb, "txts");
    break;

and something similar with the header later on?

-- 
Bj?rn Axelsson
-------------- next part --------------
Index: libavcodec/utils.c
===================================================================
--- libavcodec/utils.c.orig	2009-02-07 12:41:57.000000000 +0100
+++ libavcodec/utils.c	2009-02-07 15:04:18.000000000 +0100
@@ -494,6 +494,10 @@
                             const AVSubtitle *sub)
 {
     int ret;
+    if(sub->start_display_time) {
+        av_log(avctx, AV_LOG_ERROR, "start_display_time must be 0.\n");
+        return -1;
+    }
     ret = avctx->codec->encode(avctx, buf, buf_size, (void *)sub);
     avctx->frame_number++;
     return ret;
-------------- next part --------------
Index: libavcodec/utils.c
===================================================================
--- libavcodec/utils.c.orig	2009-02-07 15:14:12.000000000 +0100
+++ libavcodec/utils.c	2009-02-07 15:19:30.000000000 +0100
@@ -498,6 +498,8 @@
         av_log(avctx, AV_LOG_ERROR, "start_display_time must be 0.\n");
         return -1;
     }
+    if(sub->num_rects == 0 || !sub->rects)
+        return -1;
     ret = avctx->codec->encode(avctx, buf, buf_size, (void *)sub);
     avctx->frame_number++;
     return ret;