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

[Michael Niedermayer]

On Sat, Feb 07, 2009 at 03:33:12PM +0100, Bj?rn Axelsson wrote:
> 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).

ok


> 
> [...]
> 
> > > +
> > > +/** 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.

forget my comment


> 
> [...]
> 
> > > +    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).

ok


> 
> [...]
> 
> > > +    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?

i guess so.


> 
> [...]
> 
> > > +    bytestream_put_le16(&rlelenptr, put_bits_count(&pb)/8); // Len of first field
> >
> > >>8
> 
> I hope you mean >>3?

i hope that too :)


> 
> [...]
> 
> > > 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?

this might indeed be approximately correct

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090208/b576d169/attachment.pgp>