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

[Aurelien Jacobs]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 08-avidec-demux-ass-and-srt-track.patch
Type: text/x-diff
Size: 7334 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20100725/fe610f3d/attachment.patch>