[MPlayer-dev-eng] [PATCH] libass support in demux_lavf

[Evgeniy Stepanov]

On Sunday 30 December 2007 01:40:23 Michael Niedermayer wrote:
> On Sun, Dec 30, 2007 at 01:21:34AM +0300, Evgeniy Stepanov wrote:
> > On Sunday 30 December 2007 00:38:26 Diego Biurrun wrote:
> > > On Sun, Dec 30, 2007 at 12:15:26AM +0300, Evgeniy Stepanov wrote:
> > > > this patch adds libass support for SSA/ASS tracks in libavformat
> > > > demuxer. Depends on ffmpeg r11352. Embedded fonts are not supported
> > > > yet.
> > >
> > > Please do not use tabs in a file that contains only spaces for
> > > indentation.
> >
> > Which does not apply to demux_lavf.c :)
> >
> > > > --- libmpdemux/demux_lavf.c	(revision 25540)
> > > > +++ libmpdemux/demux_lavf.c	(working copy)
> > > > @@ -396,13 +401,21 @@
> > > >              if(priv->sub_streams >= MAX_S_STREAMS)
> > > >                  break;
> > > >              /* only support text subtitles for now */
> > > > -            if(codec->codec_id != CODEC_ID_TEXT)
> > > > +            if(codec->codec_id != CODEC_ID_TEXT &&
> > > > +	       codec->codec_id != CODEC_ID_SSA)
> > >
> > > This could be nicely aligned.
> >
> > Anyway, here is untabified and reindented patch.
>
> [...]
>
> > +#ifdef USE_ASS
> > +            if (ass_enabled && codec->codec_id == CODEC_ID_SSA) {
> > +                sh_sub->ass_track = ass_new_track(ass_library);
> >
> > +                ass_process_codec_private(sh_sub->ass_track,
> > codec->extradata, codec->extradata_size);
>
> What does this function do?
> And why is it called from demuxers?
> Does this mean that every demuxer which supports ASS needs such calls?

Yes, a bad design. Until now there was only one demuxer capable of SSA/ASS 
subtitle tracks. I'm going changing it with my other patch.

> And does this design not break stream copy (that is demuxer->muxer)?

It would, if there were at least one muxer for subtitles.

>
> [...]
>
> > +#ifdef USE_ASS
> > +    if (id==demux->sub->id) {
> > +        ass_track_t* track = ((sh_sub_t*)demux->sub->sh)->ass_track;
> > +        if (ass_enabled && track) {
> >
> > +            ass_process_chunk(track, pkt.data, pkt.size, (long
> > long)(pts*1000 + 0.5), (long long)((endpts-pts)*1000 + 0.5));
>
> same question
>
> Also please document the awnser in libass/ass.c or another appropriate
> place there are doxygen comments but they as well just say "process" and
> thats a very vague thing.

process == parse in this case. I'll clarify it in the comment.