[FFmpeg-devel] [PATCH 2/4] add ASS parser

[Aurelien Jacobs]

On Sun, Jul 11, 2010 at 01:50:58AM +0200, Michael Niedermayer wrote:
> On Sat, Jul 10, 2010 at 03:04:23PM +0200, Aurelien Jacobs wrote:
> > On Fri, Jul 09, 2010 at 04:03:05PM +0200, Michael Niedermayer wrote:
> > > On Thu, Jul 08, 2010 at 10:06:14AM +0200, Aurelien Jacobs wrote:
> > > > On Thu, Jul 08, 2010 at 12:46:17AM +0200, Michael Niedermayer wrote:
> > > > > On Thu, Jul 08, 2010 at 12:22:33AM +0200, Aurelien Jacobs wrote:
> > > > > > On Wed, Jul 07, 2010 at 09:13:38PM +0200, Michael Niedermayer wrote:
> > > > > > > On Tue, Jul 06, 2010 at 10:54:54PM +0200, Aurelien Jacobs wrote:
> > > > > > > [...]
> > > > > > > > -static int read_packet(AVFormatContext *s, AVPacket *pkt)
> > > > > > > > +static int ass_parse(AVCodecParserContext *s, AVCodecContext *avctx,
> > > > > > > > +                     const uint8_t **poutbuf, int *poutbuf_size,
> > > > > > > > +                     const uint8_t *buf, int buf_size)
> > > > > > > >  {
> > > > > > > >      ASSContext *ass = s->priv_data;
> > > > > > > > +    if (!ass->event_buffer)
> > > > > > > > +        ass_read_header(s, avctx, buf, buf_size);
> > > > > > > >  
> > > > > > > > +    if (ass->event_index >= ass->event_count) {
> > > > > > > > +        *poutbuf = NULL;
> > > > > > > > +        *poutbuf_size = 0;
> > > > > > > > +        return buf_size;
> > > > > > > > +    }
> > > > > > > >  
> > > > > > > > +    *poutbuf = ass->event[ass->event_index++];
> > > > > > > > +    *poutbuf_size = strlen(*poutbuf);
> > > > > > > > +    s->pts = s->dts = get_pts(*poutbuf);
> > > > > > > > +    if (ass->event_index >= ass->event_count)
> > > > > > > > +        return buf_size;
> > > > > > > > +    return *poutbuf_size;
> > > > > > > >  }
> > > > > > > 
> > > > > > > i dont see how this would work with seeking?
> > > > > > 
> > > > > > I'm not sure what you have in mind. Actually, I don't really know how a
> > > > > > parser can handle seeking... And especially the ASS parser.
> > > > > > 
> > > > > > Just for the record, the ASS parser currently require to be feeded with
> > > > > > the *whole* ASS file/stream before it can start outputing the first
> > > > > > packet. This is due to this stupidity in the ASS spec allowing events to
> > > > > > be stored out of order.
> > > > > > 
> > > > > > The only alternative I could see to this, would be to do the reordering
> > > > > > at a higher level, in av_read_frame() for example. This would also allow
> > > > > > to properly interleave the ASS packets with the other streams, but this
> > > > > > seems slightly hackish...
> > > > > 
> > > > > what is the problem with reordering in the demuxer, as done currently?
> > > > 
> > > > The AVI demuxer don't do reordering currently... It only contains one
> > > > blob that contains some subtitles, that it passes as a single AVPacket
> > > > to the parser. The AVI demuxer don't even know if the subtitle packet
> > > > contains ASS or SRT...
> > > 
> > > the avi demuxer can just call probe() of ass/srt, it can just call the
> > > ass demuxer. One day demuxer chaining could be used of course ...
> > > 
> > > the design you suggest isnt scaleable as for every seek all subtitles
> > > will always be passed through the whole chain.
> > > that is seeking would be O(n), this may be ok for desktops or normal
> > > 2 hour movies but reduce cpu capability and increase file length and
> > > you hit a point where this breaks down, and i dont think a design
> > > like this is good even if it wont affect many real world cases.
> > 
> > Hum... Well...
> > So basically you're suggesting that I go back to something similar to
> > what I proposed almost a year ago [1], and that I forget about your
> > AVParser usage suggestion ? Is that right ?
> > If so, I will try to rework this patchset in this direction...
> 
> oops, iam sorry for having pushed into this direction, i honestly
> thought it would be alot simpler and require few if any changes to
> the (in svn) code. I was unaware  back then that subtitle packets could
> be randomly ordered in a single avi chunk.

Don't be sorry. I actually think using a parser was at least worth a
try.

> Though to my defense, my excuse, i said:
> ------
> > > i wouldnt mind havng ass split in the demuxer if it could be done naturally
> >
> > What do you mean ? Should the ass spliting be implemented as a parser instead
> > of a demuxer ? And assdec.c would just set need_parsing and do nothing else ?
> 
> iam not sure ... maybe, maybe not
> 
> 
> > Is it OK for a parser to receive a packet that needs to be split accross
> > several hours ?
> 
> if it works and the alternative if too complex i dont see a problem
> ------

Yes, you didn't pushed that hard toward using a parser.
When I started working on this code again, I just remembered that I
should try to implement this using a parser, and I didn't read this old
thread again. So I actually pushed myself in the parser way to see if it
would be possible to implement this way.

> but considering the whole, yes i think your original patch is the better
> design

I agree.

> i would suggest though that ass_demuxer is used and not ff_* functions
> exportet _where_doing_so_is_simpler_.

Right. Anyway, as we also have to support SRT in AVI, direct call to
ff_* functions is much too ugly.

> again, iam sorry for this, it is entirely my mistake

Don't be. It was worth a try.
I have a new patch series ready to send...

Aurel