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

[Aurelien Jacobs]

On Thu, Jul 29, 2010 at 03:51:03AM +0200, Michael Niedermayer wrote:
> On Sun, Jul 25, 2010 at 12:09:19AM +0200, Aurelien Jacobs wrote:
> > On Fri, Jul 23, 2010 at 06:13:33PM +0200, Reimar D?ffinger wrote:
> > > On Thu, Jul 22, 2010 at 11:48:27PM +0200, Aurelien Jacobs wrote:
> > > > On Thu, Jul 22, 2010 at 07:04:48PM +0200, Reimar D?ffinger wrote:
> > > > > On Thu, Jul 22, 2010 at 12:39:51AM +0200, Aurelien Jacobs wrote:
> > > > > > >
> > > > > > > Also it might be nicer to malloc the destination buffer to
> > > > > > > 7*name_size+1 (after limiting name_size) to avoid the check
> > > > > > > in the inner loop and allow longer metadata.
> > > > > >
> > > > > > I will give this a try.
> > 
> > I tryed it and it was significantly uglier for no real gain.
> > So I left it as it was.
> > 
> > > > > > > > +        memmove(pkt->data, ptr, size);
> > > > > > > 
> > > > > > > Seems like a very inefficient way to do
> > > > > > > pkt->data += pkt->size - size;
> > > > > > 
> > > > > > But this is a way that won't crash when doing av_free_packet(pkt).
> > > > > > pkt->data points to av_malloced memory, so we don't want to loose this
> > > > > > pointer.
> > > > > 
> > > > > But you memset it to 0 in the end anyway, that's why it seemed
> > > > > pointless.
> > > > 
> > > > Oops... Indeed my explanation was wrong. I just remembered that this
> > > > memmove was necessary to be able to free data afterward. But it is not
> > > > thru av_free_packet(). The pkt->data is passed to av_alloc_put_byte()
> > > > and is freed with url_fclose() (called by av_close_input_file).
> > > 
> > > I admit I was wondering where it was freed.
> > > Maybe a comment would be a good idea?
> > 
> > I see that it was not obvious. And maybe even abusing a little bit the
> > API by relying on the internal of av_close_input_file(), and trusting it
> > to work on a context opened with av_alloc_put_byte() + av_open_input_stream().
> > 
> > > > > But even if I misunderstood that there should be a lot of simple
> > > > > ways to avoid the memmove without a lot of effort.
> > > > 
> > > > Sure. It should be possible to save the pkt->data pointers for each
> > > > stream, and manually free them when in avi_read_close(). I feared it
> > > > would be kind of ugly, but I will give it a try.
> > > 
> > > Well, I may have underestimate how much effort it would be.
> > > Don't waste too much time on it, just a comment explaining the why
> > > is an ok solution, too.
> > 
> > Wasn't much of an effort anyway. The code is just 2 or 3 lines bigger,
> > but easier to follow, and safer regarding API usage. So I followed this
> > suggestion.
> > 
> > I also fixed all the other points you noticed.
> > Improved patch attached.
> > 
> > Aurel
> >  avidec.c |  127 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 125 insertions(+), 2 deletions(-)
> > 5d1a467ff090e446e620f02209e53a29955a084b  08-avidec-demux-ass-and-srt-track.patch
> > avidec: demux ASS and SRT tracks
> > 
> > From: Aurelien Jacobs <aurel at gnuage.org>
> [...]
> > @@ -1141,6 +1254,9 @@ static int avi_read_seek(AVFormatContext *s, int stream_index, int64_t timestamp
> >          ast2->packet_size=
> >          ast2->remaining= 0;
> >  
> > +        if (seek_subtitle(st, st2, timestamp))
> > +            continue;
> 
> i prefer the subtitle condition in the if like
> if(subtitle / ast2->sub_ctx or whateve){
>     seek_subtitle()
>     continue;
> }

OK. Modified.

> > +
> >          if (st2->nb_index_entries <= 0)
> >              continue;
> >  
> 
> > @@ -1178,7 +1294,14 @@ static int avi_read_close(AVFormatContext *s)
> >  
> >      for(i=0;i<s->nb_streams;i++) {
> >          AVStream *st = s->streams[i];
> > +        AVIStream *ast = st->priv_data;
> >          av_free(st->codec->palctrl);
> > +        if (ast->sub_ctx) {
> > +            av_free(ast->sub_ctx->pb);
> > +            av_close_input_stream(ast->sub_ctx);
> > +        }
> 
> please use av_freep() as precoution against double frees

OK.

> with these changes and if tested, patch ok

Applied.

Aurel