[FFmpeg-devel] [PATCH] matroskadec, matroskadec, srtenc: Read/Write duration for subtitles.

[Philip Langdale]

On Sun, 5 Aug 2012 03:37:42 +0200
Alexander Strasser <eclipse7 at gmx.net> wrote:

> 
>   I am in favor of your patch.
> 
>   Just two problems I noticed:
> 
> [...]
> > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> > index f5fdaae..abc6ddd 100644
> > --- a/libavformat/matroskaenc.c
> > +++ b/libavformat/matroskaenc.c
> > @@ -1136,7 +1136,9 @@ static int
> > mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt)
> > AVIOContext *pb = s->pb; AVCodecContext *codec =
> > s->streams[pkt->stream_index]->codec; int keyframe = !!(pkt->flags
> > & AV_PKT_FLAG_KEY);
> > -    int duration = pkt->duration;
> > +    /* For backward compatibility, prefer convergence_duration. */
> > +    int duration = pkt->convergence_duration > 0 ?
> > +                   pkt->convergence_duration : pkt->duration;
> 
>   I think this should stay unchanged and instead...
> 
> >      int ret;
> >      int64_t ts = mkv->tracks[pkt->stream_index].write_dts ?
> > pkt->dts : pkt->pts; 
> > @@ -1166,7 +1168,7 @@ static int
> > mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt)
> > duration = mkv_write_srt_blocks(s, pb, pkt); } else {
> >          ebml_master blockgroup = start_ebml_master(pb,
> > MATROSKA_ID_BLOCKGROUP, mkv_blockgroup_size(pkt->size));
> > -        duration = pkt->convergence_duration;
> > +        duration = pkt->duration;
> 
>   ...this should have the conditional as in the first hunk.

Agreed. I think I got confused when I was doing this part.

> >          mkv_write_block(s, pb, MATROSKA_ID_BLOCK, pkt, 
> >          put_ebml_uint(pb, MATROSKA_ID_BLOCKDURATION, duration);
> >          end_ebml_master(pb, blockgroup);

> > diff --git a/libavformat/srtenc.c b/libavformat/srtenc.c
> > index 171c45b..7c0119b 100644
> > --- a/libavformat/srtenc.c
> > +++ b/libavformat/srtenc.c
> > @@ -64,7 +64,9 @@ static int srt_write_packet(AVFormatContext *avf,
> > AVPacket *pkt) int len;
> >  
> >          if (d <= 0)
> > -            d = pkt->convergence_duration;
> > +            /* For backward compatibility, prefer
> > convergence_duration. */
> > +            d = pkt->convergence_duration > 0 ?
> > +                pkt->convergence_duration : pkt->duration;
> 
>   This should be wrong or at least redundant.
> 
>   AFAICT at that point d was already pkt->duration.

That's a good point. I'll leave the logic alone and just add a comment
here.

Thanks,

--phil