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

Michael Niedermayer michaelni
Thu Jul 29 03:51:03 CEST 2010


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;
}


> +
>          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

with these changes and if tested, patch ok

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 190 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100729/6ed7b5ae/attachment.pgp>



More information about the ffmpeg-devel mailing list