FFmpeg development discussions and patch reviews

Philip Langdale | 5 Aug 2012 00:08

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

Ok, let's get this sorted out once and for all.

I can't see any reason to manipulate the convergence duration instead.
These subtitle packets are the moral equivalent of keyframes, and
while the documentation says it should be used for 'some types of
subtitles', I don't see how Matroska or srt text subtitles fall into
that category.

The original claim was that convergence_duration was needed to avoid
overflow on long duration subtitles. This claim seems questionable.
If we consider the typical timebase of 1/1000, that still allows
for a duration of 8 years. For a 1/1000000 timebase, you still get
a 71 minute maximum duration, so I'm not sure where this claim
originated from. Only if you go down to a 1ns timebase do you end
up with a short max duration of ~4 seconds. Am I missing something?

This change is backward compatible by reading and writing
convergence_duration. I don't know if that's really necessary.

Signed-off-by: Philip Langdale <philipl <at> overt.org>
---
 libavformat/matroskadec.c |    5 +++--
 libavformat/matroskaenc.c |    6 ++++--
 libavformat/srtenc.c      |    4 +++-
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 2c954af..d5bdad4 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c

(Continue reading)

headers

Michael Niedermayer | 5 Aug 2012 00:38

Re: [PATCH] matroskadec, matroskadec, srtenc: Read/Write duration for subtitles.

On Sat, Aug 04, 2012 at 03:08:23PM -0700, Philip Langdale wrote:
> Ok, let's get this sorted out once and for all.
> 
> I can't see any reason to manipulate the convergence duration instead.
> These subtitle packets are the moral equivalent of keyframes, and
> while the documentation says it should be used for 'some types of
> subtitles', I don't see how Matroska or srt text subtitles fall into
> that category.
> 
> The original claim was that convergence_duration was needed to avoid
> overflow on long duration subtitles. This claim seems questionable.
> If we consider the typical timebase of 1/1000, that still allows
> for a duration of 8 years. For a 1/1000000 timebase, you still get
> a 71 minute maximum duration, so I'm not sure where this claim
> originated from. Only if you go down to a 1ns timebase do you end
> up with a short max duration of ~4 seconds. Am I missing something?
> 
> This change is backward compatible by reading and writing
> convergence_duration. I don't know if that's really necessary.
> 
> Signed-off-by: Philip Langdale <philipl <at> overt.org>
> ---
>  libavformat/matroskadec.c |    5 +++--
>  libavformat/matroskaenc.c |    6 ++++--
>  libavformat/srtenc.c      |    4 +++-
>  3 files changed, 10 insertions(+), 5 deletions(-)

the patch should be ok

[...]

(Continue reading)

headers

Alexander Strasser | 5 Aug 2012 03:37

Re: [PATCH] matroskadec, matroskadec, srtenc: Read/Write duration for subtitles.

Hi,

Philip Langdale wrote:
> Ok, let's get this sorted out once and for all.
> 
> I can't see any reason to manipulate the convergence duration instead.
> These subtitle packets are the moral equivalent of keyframes, and
> while the documentation says it should be used for 'some types of
> subtitles', I don't see how Matroska or srt text subtitles fall into
> that category.
> 
> The original claim was that convergence_duration was needed to avoid
> overflow on long duration subtitles. This claim seems questionable.
> If we consider the typical timebase of 1/1000, that still allows
> for a duration of 8 years. For a 1/1000000 timebase, you still get
> a 71 minute maximum duration, so I'm not sure where this claim
> originated from. Only if you go down to a 1ns timebase do you end
> up with a short max duration of ~4 seconds. Am I missing something?
> 
> This change is backward compatible by reading and writing
> convergence_duration. I don't know if that's really necessary.

  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

(Continue reading)

headers

Philip Langdale | 5 Aug 2012 06:21

Re: [PATCH] matroskadec, matroskadec, srtenc: Read/Write duration for subtitles.

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
> >  <at>  <at>  -1136,7 +1136,9  <at>  <at>  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; 
> >  <at>  <at>  -1166,7 +1168,7  <at>  <at>  static int
> > mkv_write_packet_internal(AVFormatContext *s, AVPacket *pkt)

(Continue reading)