[FFmpeg-devel] [PATCH v2] lavf/movenc: suggest video_track_timescale for invalid timescale

[Mark Thompson]

On 12/10/16 11:04, Carl Eugen Hoyos wrote:
> 2016-10-11 21:06 GMT+02:00 Josh de Kock <josh at itanimul.li>:
>> Fixes ticket #5882.
> 
> I don't object but I don't think it is correct.
> 
>> While it doesn't automatically set the timescale
>> for the user as that would destroy data without prompt, it will tell
>> the user how they could set the timescale (as this is mostly likely
>> what they want).
>>
>> Signed-off-by: Josh de Kock <josh at itanimul.li>
>> ---
>>
>>> Would it be useful to print the max duration?
>>  I'm not entirely sure, open to suggestions though.
>>
>>  libavformat/movenc.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavformat/movenc.c b/libavformat/movenc.c
>> index d7c7158..6bada25 100644
>> --- a/libavformat/movenc.c
>> +++ b/libavformat/movenc.c
>> @@ -5677,11 +5677,16 @@ static int mov_write_header(AVFormatContext *s)
>>                  ret = AVERROR(EINVAL);
>>                  goto error;
>>              }
>> -            if (track->mode == MODE_MOV && track->timescale > 100000)
>> +            if (track->mode == MODE_MOV && track->timescale > 100000) {
>> +                /* NOTE: forcing setting the suggested timescale manually means ffmpeg won't destroy
>> +                 * timestamps without explicit instruction. */
> 
>> +                unsigned int suggested = (unsigned int)av_rescale(st->time_base.den, 1000, st->time_base.num);
> 
> If we go this way (I am not completely convinced about it), we definitely need
> a heuristic that suggests 24000 for 10000000/417083 fps and 30000 if applicable.

How about the approach in the test program below?  This makes the whole timebase
fraction; the suggested timescale would then be the denominator of that.  (It is
currently just a program linked with lavu, I could make it into a patch if
someone else thinks this might actually be a good idea.  It would need better
error handling and some thoughts about overflow, too.)

It shows that we can do significantly better in the 417083/10000000 case than
any of the currently-suggested approaches (divide by 1000, guess 1/24 or
1001/24000):

$ ./a.out 417083 10000000 100000
3715/89071
$ bc -l
417083/10000000
.04170830000000000000
3715/89071
.04170830012012888594
417/10000
.04170000000000000000
1/24
.04166666666666666666
1001/24000
.04170833333333333333

(The suggested timescale value would be 89071.)

---

#include <stdio.h>

#include "libavutil/rational.h"

AVRational av_rational_reduce(AVRational qi, int max_den)
{
  AVRational qc = qi;
  AVRational q0 = { 0, 1 };
  AVRational q1 = { 1, 0 };
  AVRational q2;
  int a, t, k;

  if (qc.den <= max_den)
    return qc;

  while (1) {
    a = qc.num / qc.den;

    q2.num = q0.num + a * q1.num;
    q2.den = q0.den + a * q1.den;
    if (q2.den > max_den)
      break;

    q0 = q1;
    q1 = q2;

    t = qc.num;
    qc.num = qc.den;
    qc.den = t - a * qc.den;
  }

  k = (max_den - q0.den) / q1.den;
  q2.num = q0.num + k * q1.num;
  q2.den = q0.den + k * q1.den;

  if (av_nearer_q(qi, q1, q2) >= 0)
    return q1;
  else
    return q2;
}

int main(int argc, char **argv)
{
  AVRational ri, ro;
  int max;
  sscanf(argv[1], "%d", &ri.num);
  sscanf(argv[2], "%d", &ri.den);
  sscanf(argv[3], "%d", &max);

  ro = av_rational_reduce(ri, max);

  printf("%d/%d\n", ro.num, ro.den);

  return 0;
}