[vlc-devel] [PATCH] subtitle: fix AQT subtitle parsing

[Alexandre Janniaux]

Ok, thanks for details ;)

Regards,
--
Alexandre Janniaux
Videolabs

On Mon, Jan 18, 2021 at 05:36:19PM +0100, Steve Lhomme wrote:
> On 2021-01-18 17:05, Alexandre Janniaux wrote:
> > Hi,
> >
> > LGTM if the nit below is solved, but it probably needs
> > comment in the code too, as shown by the comment:
> >
> >      /* 1/10th of second ? Frame based ? FIXME */
>
> That line is for PJS (which I didn't fix). It's not related to this patch.
>

That's mostly the second then, where time is unit-less!

> > and:
> >
> >      int t; /* Time */
> >
> > > -                TextPreviousLine( txt );
> >
> > Is the removal of this line voluntary? It seems to remove
> > the initial intent in this code specified by the comment
> > right above:
> >
> >      /* We have been too far: end of the subtitle, begin of next */
> >
> > And there is not explanation of this removal in the commit
> > message.
>
> That's one of the fix, with the wrong use of frame numbers. There is a start
> time and an end time, as with proper subtitles. The discarding of the line
> with the second time is bogus.
>
> > Regards,
> > --
> > Alexandre Janniaux
> > Videolabs
> >
> > On Mon, Jan 18, 2021 at 03:46:59PM +0100, Steve Lhomme wrote:
> > > The numbers we read are frame numbers. There's a start and stop frame number.
> > > (based on http://web.archive.org/web/20070210095721/http://www.volny.cz/aberka/czech/aqt.html
> > > and libavformat)
> > > ---
> > >   modules/demux/subtitle.c | 6 ++----
> > >   1 file changed, 2 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/modules/demux/subtitle.c b/modules/demux/subtitle.c
> > > index 968a356a596..f117bda2b76 100644
> > > --- a/modules/demux/subtitle.c
> > > +++ b/modules/demux/subtitle.c
> > > @@ -1599,18 +1599,16 @@ static int ParseAQT(vlc_object_t *p_obj, subs_properties_t *p_props, text_t *txt
> > >           /* Data Lines */
> > >           if( sscanf (s, "-->> %d", &t) == 1)
> > >           {
> > > -            p_subtitle->i_start = (int64_t)t; /* * FPS*/
> > > -            p_subtitle->i_stop  = -1;
> > > -
> > >               /* Starting of a subtitle */
> > >               if( i_firstline )
> > >               {
> > > +                p_subtitle->i_start = t * p_props->i_microsecperframe;
> > >                   i_firstline = 0;
> > >               }
> > >               /* We have been too far: end of the subtitle, begin of next */
> > >               else
> > >               {
> > > -                TextPreviousLine( txt );
> > > +                p_subtitle->i_stop  = t * p_props->i_microsecperframe;
> > >                   break;
> > >               }
> > >           }
> > > --
> > > 2.29.2
> > >
> > > _______________________________________________
> > > vlc-devel mailing list
> > > To unsubscribe or modify your subscription options:
> > > https://mailman.videolan.org/listinfo/vlc-devel
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> >
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel