[FFmpeg-devel] [PATCH 2/3] avformat/microdvd: set AV_DISPOSITION_FRAME_BASED

[Clément Bœsch]

On Tue, Feb 18, 2014 at 09:58:21PM +0100, wm4 wrote:
> On Tue, 18 Feb 2014 21:51:56 +0100
> Clément Bœsch <u at pkh.me> wrote:
> 
> > On Mon, Feb 17, 2014 at 09:49:35PM +0100, wm4 wrote:
> > > ---
> > >  libavformat/microdvddec.c | 7 ++++++-
> > >  1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > 
> > See comment in previous patch.
> > 
> > > diff --git a/libavformat/microdvddec.c b/libavformat/microdvddec.c
> > > index 7b49e43..51f7f48 100644
> > > --- a/libavformat/microdvddec.c
> > > +++ b/libavformat/microdvddec.c
> > > @@ -78,6 +78,7 @@ static int microdvd_read_header(AVFormatContext *s)
> > >      AVStream *st = avformat_new_stream(s, NULL);
> > >      int i = 0;
> > >      char line[MAX_LINESIZE];
> > > +    int has_real_fps = 0;
> > >  
> > >      if (!st)
> > >          return AVERROR(ENOMEM);
> > > @@ -98,8 +99,10 @@ static int microdvd_read_header(AVFormatContext *s)
> > >  
> > >              if ((sscanf(line, "{%d}{}%6lf",    &frame, &fps) == 2 ||
> > >                   sscanf(line, "{%d}{%*d}%6lf", &frame, &fps) == 2)
> > > -                && frame <= 1 && fps > 3 && fps < 100)
> > > +                && frame <= 1 && fps > 3 && fps < 100) {
> > >                  pts_info = av_d2q(fps, 100000);
> > > +                has_real_fps = 1;
> > 
> > Note: btw, didn't you want to ignore the fps line from appearing in the
> > subtitle events?
> 
> Actually, this happened because the BOM messed with line parsing in
> mysterious ways, so the timestamps weren't read correctly. With my
> other patch applied, it's parsed as subtitle event with duration 0.
> 
> I'll add another patch to skip the event.
> 
> > > +            }
> > >              if (!st->codec->extradata && sscanf(line, "{DEFAULT}{}%c", &c) == 1) {
> > >                  st->codec->extradata = av_strdup(line + 11);
> > >                  if (!st->codec->extradata)
> > > @@ -131,6 +134,8 @@ static int microdvd_read_header(AVFormatContext *s)
> > >      avpriv_set_pts_info(st, 64, pts_info.den, pts_info.num);
> > >      st->codec->codec_type = AVMEDIA_TYPE_SUBTITLE;
> > >      st->codec->codec_id   = AV_CODEC_ID_MICRODVD;
> > > +    if (!has_real_fps)
> > > +        st->disposition |= AV_DISPOSITION_FRAME_BASED;
> > 
> > Whatever the solution, I think a comment would be welcome somewhere in the
> > code (maybe in an additional commit?). I'm probably not very smart, but
> > it's not obvious what you are trying to solve. Maybe something like:
> > 
> > "If no FPS are specified in the file, applications should ignore the
> > stream time base and consider the PTS as frame number. Otherwise, it will
> > only work if the video is at the same FPS as the default time base.  On
> > the other hand, if the FPS are specified in that microdvd sub file, the
> > pts should be honored using the stream time base, just like any other time
> > base subtitle format."
> 
> This looks good. Can I copy and paste it? And where should I insert it?

Sure, feel free to copy paste and fix my clumsy wording. Any place should
do; maybe above FPS parsing, wherever you think it's appropriate and won't
be missed if random fps/frame-based changes appear.


-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140218/7ee24adf/attachment.asc>