diff mbox

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

Message ID 20161011190639.79085-1-josh@itanimul.li
State Changes Requested
Headers show

Commit Message

Josh Dekker Oct. 11, 2016, 7:06 p.m. UTC 
Fixes ticket #5882. 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@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(-)

Comments

Michael Niedermayer Oct. 11, 2016, 11:27 p.m. UTC | #1 
On Tue, Oct 11, 2016 at 08:06:39PM +0100, Josh de Kock wrote:
> Fixes ticket #5882. 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@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);
>                  av_log(s, AV_LOG_WARNING,
>                         "WARNING codec timebase is very high. If duration is too long,\n"
>                         "file may not be playable by quicktime. Specify a shorter timebase\n"
> -                       "or choose different container.\n");
> +                       "or choose different container. Using -video_track_timescale %d\n"
> +                       "may fix this issue.\n", suggested);

maybe a -max_timescale parameter would be usefull
users could with that limit teh value without having to do something
per file

[...]
Nicolas George Oct. 12, 2016, 7:03 a.m. UTC | #2 
Le decadi 20 vendémiaire, an CCXXV, Josh de Kock a écrit :
> Fixes ticket #5882. 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@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);
>                  av_log(s, AV_LOG_WARNING,
>                         "WARNING codec timebase is very high. If duration is too long,\n"
>                         "file may not be playable by quicktime. Specify a shorter timebase\n"
> -                       "or choose different container.\n");

> +                       "or choose different container. Using -video_track_timescale %d\n"

Nit: the leading dash is specific to the command-line tools. At the library
level, the option is named just "video_track_timescale".

> +                       "may fix this issue.\n", suggested);
> +            }
>              if (track->mode == MODE_MOV &&
>                  track->par->codec_id == AV_CODEC_ID_RAWVIDEO &&
>                  track->tag == MKTAG('r','a','w',' ')) {

Regards,
Carl Eugen Hoyos Oct. 12, 2016, 10:04 a.m. UTC | #3 
2016-10-11 21:06 GMT+02:00 Josh de Kock <josh@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@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.

>                  av_log(s, AV_LOG_WARNING,
>                         "WARNING codec timebase is very high. If duration is too long,\n"
>                         "file may not be playable by quicktime. Specify a shorter timebase\n"
> -                       "or choose different container.\n");
> +                       "or choose different container. Using -video_track_timescale %d\n"
> +                       "may fix this issue.\n", suggested);

I believe this should also suggest to "force a sane framerate" as an
alternative.

Suggesting video_track_timescale is very useful, thank you!

Carl Eugen
Mark Thompson Oct. 12, 2016, 12:44 p.m. UTC | #4 
On 12/10/16 11:04, Carl Eugen Hoyos wrote:
> 2016-10-11 21:06 GMT+02:00 Josh de Kock <josh@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@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;
}
Michael Niedermayer Oct. 12, 2016, 1:35 p.m. UTC | #5 
On Wed, Oct 12, 2016 at 01:44:52PM +0100, Mark Thompson wrote:
> On 12/10/16 11:04, Carl Eugen Hoyos wrote:
> > 2016-10-11 21:06 GMT+02:00 Josh de Kock <josh@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@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)

why dont you use av_reduce() directly ?


[...]
Sven C. Dack Oct. 12, 2016, 1:38 p.m. UTC | #6 
On 12/10/16 13:44, Mark Thompson wrote:
> 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):

It may be too much. We only know of 3 video files, which don't play on the Apple 
player. We don't know how they got produced, what produced them, what caused 
them to have this deviation, if it was done intentionally or by accident, nor do 
we know why Apple chooses to deviate in their playback from other players, why 
exactly it fails and where else it fails apart from these 3 files. We really 
only know about 3 files, which could have already been fixed with the help of 
ffmpeg and without applying any patches to it. So I wouldn't take it this too 
far. Patches are supposed to be simple and one should try to make then even 
simpler, so say the rules of the project.
Mark Thompson Oct. 12, 2016, 1:50 p.m. UTC | #7 
On 12/10/16 14:35, Michael Niedermayer wrote:
> why dont you use av_reduce() directly ?

Because I didn't know it existed.  That would indeed be better (it appears to be
doing the same thing with continued fraction).  The sentiment behind a suggested
patch is equivalent.

- Mark
Sven C. Dack Oct. 12, 2016, 1:52 p.m. UTC | #8 
On 12/10/16 14:38, Sven C. Dack wrote:
>
> It may be too much. We only know of 3 video files, which don't play on the 
> Apple player. We don't know how they got produced, what produced them, what 
> caused them to have this deviation, if it was done intentionally or by 
> accident, nor do we know why Apple chooses to deviate in their playback from 
> other players, why exactly it fails and where else it fails apart from these 3 
> files. We really only know about 3 files, which could have already been fixed 
> with the help of ffmpeg and without applying any patches to it. So I wouldn't 
> take it this too far. Patches are supposed to be simple and one should try to 
> make then even simpler, so say the rules of the project. 
That said, I believe that Alex has already produced his 3 files and can now play 
them on his Apple player, all with the help of ffmpeg and the use of the right 
command option. So the fix isn't actually needed any longer.
diff mbox

Patch

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);
                 av_log(s, AV_LOG_WARNING,
                        "WARNING codec timebase is very high. If duration is too long,\n"
                        "file may not be playable by quicktime. Specify a shorter timebase\n"
-                       "or choose different container.\n");
+                       "or choose different container. Using -video_track_timescale %d\n"
+                       "may fix this issue.\n", suggested);
+            }
             if (track->mode == MODE_MOV &&
                 track->par->codec_id == AV_CODEC_ID_RAWVIDEO &&
                 track->tag == MKTAG('r','a','w',' ')) {