[FFmpeg-devel] [PATCH 4/4] avidec: demux ASS and SRT tracks

[Michael Niedermayer]

On Thu, Jul 08, 2010 at 12:12:00AM +0200, Aurelien Jacobs wrote:
> On Wed, Jul 07, 2010 at 09:27:30PM +0200, Michael Niedermayer wrote:
> > On Tue, Jul 06, 2010 at 10:55:04PM +0200, Aurelien Jacobs wrote:
> > > 
> > > ---
> > >  libavformat/avidec.c |   42 ++++++++++++++++++++++++++++++++++++++++--
> > >  libavformat/utils.c  |    2 ++
> > >  2 files changed, 42 insertions(+), 2 deletions(-)
> > 
> > >  avidec.c |   42 ++++++++++++++++++++++++++++++++++++++++--
> > >  utils.c  |    2 ++
> > >  2 files changed, 42 insertions(+), 2 deletions(-)
> > > e528c97d940b4d16e1a96f56dd4686a3b8f3b83e  avidec-demux-ass-and-srt-track.patch
> > > diff --git a/libavformat/avidec.c b/libavformat/avidec.c
> > > index cdf8307..ac1e3b6 100644
> > > --- a/libavformat/avidec.c
> > > +++ b/libavformat/avidec.c
> > > @@ -598,6 +598,14 @@ static int avi_read_header(AVFormatContext *s, AVFormatParameters *ap)
> > >                          ast->dshow_block_align = 0;
> > >                      }
> > >                      break;
> > > +                case AVMEDIA_TYPE_SUBTITLE:
> > > +                    st->codec->codec_type = AVMEDIA_TYPE_SUBTITLE;
> > > +                    st->codec->codec_id   = CODEC_ID_PROBE;
> > 
> > > +                    st->codec->codec_tag  = 0;
> > 
> > why ?
> 
> Good question... I don't remember why I did this, but this looks like a
> copy/paste of the default case a bit below. It's useless anyway, so removed.
> 
> > > @@ -687,6 +695,26 @@ static int avi_read_header(AVFormatContext *s, AVFormatParameters *ap)
> > >      return 0;
> > >  }
> > >  
> > > +static void read_gab2_sub(AVStream *st, AVPacket *pkt) {
> > > +    if(!strcmp(pkt->data, "GAB2") && AV_RL16(pkt->data+5) == 2) {
> > > +        uint8_t tmp, desc[256], *d = desc;
> > > +        const uint8_t *ptr = pkt->data+7;
> > > +        int i, size, name_size = bytestream_get_le32(&ptr);
> > > +
> > > +        for(i=0; i<name_size; i+=2)
> > > +            PUT_UTF8(bytestream_get_le16(&ptr), tmp,
> > > +                     if(d < desc+sizeof(desc)-1)  *d++ = tmp;);
> > 
> > no end of array checks for the input
> 
> Fixed.
> 
> > > +        *d = 0;
> > > +        av_metadata_set2(&st->metadata, "title", desc, 0);
> > > +
> > > +        ptr += 2;
> > > +        size = bytestream_get_le32(&ptr);
> > > +        size = FFMIN(size, pkt->size+pkt->data-ptr);
> > > +        memmove(pkt->data, ptr, size);
> > > +        pkt->size = size;
> > 
> > that could be negative
> 
> Fixed.
> 
> > > @@ -801,6 +829,10 @@ resync:
> > >                  av_log(s, AV_LOG_ERROR, "Failed to append palette\n");
> > >          }
> > >  
> > > +        if (st->codec->codec_type == AVMEDIA_TYPE_SUBTITLE
> > > +            && !st->codec->codec_tag)
> > > +            read_gab2_sub(st, pkt);
> > 
> > does this really belong in the avi demuxer layer?
> 
> I have no idea what you have in mind...
> AFAICT the gab2 blob is specific to avi. It's not used in any other
> container. And it belongs to the demuxer layer as it contains a title
> metadata (which belongs to AVStream).

if its just used in avi then ok, though containing title metadata is not
a good argument, this can exist at codec layer too.


> 
> > > @@ -1138,6 +1170,12 @@ static int avi_read_seek(AVFormatContext *s, int stream_index, int64_t timestamp
> > >          ast2->packet_size=
> > >          ast2->remaining= 0;
> > >  
> > > +        if (st2->codec->codec_type == AVMEDIA_TYPE_SUBTITLE
> > > +            && !st2->codec->codec_tag) {
> > > +            ast2->frame_offset = 0;
> > > +            continue;
> > > +        }
> > 
> > this doesnt feel correct, all sees go to point 0 ?
> 
> Well, actually gab2 subtitles are composed of one and only one
> packet. After seeking, subtitles contained in this packet may be needed
> (or not, but the avi demuxer has no way to know). So the only thing the
> avi demuxer can do after a seek is to output this single packet.
> Did I missed anything ?

this design is sick
it reminds me of these avis that have hours of audio in a single packet


[...]
> @@ -598,6 +598,13 @@ static int avi_read_header(AVFormatContext *s, AVFormatParameters *ap)
>                          ast->dshow_block_align = 0;
>                      }
>                      break;
> +                case AVMEDIA_TYPE_SUBTITLE:
> +                    st->codec->codec_type = AVMEDIA_TYPE_SUBTITLE;
> +                    st->codec->codec_id   = CODEC_ID_PROBE;
> +                    st->need_parsing      = AVSTREAM_PARSE_FULL;
> +                    av_set_pts_info(st, 64, 1, 1000);

> +                    url_fskip(pb, size);

what is in this data we skip?

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

Into a blind darkness they enter who follow after the Ignorance,
they as if into a greater darkness enter who devote themselves
to the Knowledge alone. -- Isha Upanishad
-------------- 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/20100708/bc86b815/attachment.pgp>