diff mbox

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

Message ID 20161011113030.76374-1-josh@itanimul.li
State Superseded
Headers show

Commit Message

Josh Dekker Oct. 11, 2016, 11:30 a.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>
---
 libavformat/movenc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Moritz Barsnick Oct. 11, 2016, 11:49 a.m. UTC | #1
On Tue, Oct 11, 2016 at 12:30:30 +0100, Josh de Kock wrote:
>                         "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. You may find the -video_track_timescale\n"
> +                       "flag useful to forcibly set a specific timescale. Ex. 30000/1001 FPS\n"
> +                       "would be -video_track_timescale 30000.\n");

Would it be useful to print the max duration (do we know what it would
be?), and possibly even the present timebase?

Moritz
Alexey Eromenko Oct. 11, 2016, 12:32 p.m. UTC | #2
Also the patch needs to cover both MP4 and MOV containers.

On Tue, Oct 11, 2016 at 1:49 PM, Moritz Barsnick <barsnick@gmx.net> wrote:
> On Tue, Oct 11, 2016 at 12:30:30 +0100, Josh de Kock wrote:
>>                         "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. You may find the -video_track_timescale\n"
>> +                       "flag useful to forcibly set a specific timescale. Ex. 30000/1001 FPS\n"
>> +                       "would be -video_track_timescale 30000.\n");
>
> Would it be useful to print the max duration (do we know what it would
> be?), and possibly even the present timebase?
>
> Moritz
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Alexey Eromenko Oct. 11, 2016, 12:40 p.m. UTC | #3
I have written a different patch, which should auto-fix the problem,
but I can't compile and test it now (due to build system issues):
movenc.c affects both MP4 and MOV containers, right ?

====

--- ./movenc.c.orig     2016-10-11 06:24:44.328730759 -0400
+++ ./ffmpeg-3.1.3/ffmpeg-3.1.3/libavformat/movenc.c    2016-10-11
08:12:50.626150035 -0400
@@ -5533,22 +5533,26 @@
                 track->timescale = mov->video_track_timescale;
             } else {
                 track->timescale = st->time_base.den;
                 while(track->timescale < 10000)
                     track->timescale *= 2;
             }
+            if (track->timescale > 100000) {
+                int timescale_new = ((st->time_base.den) * (0x10000)) /
+                (st->time_base.num)*1000;
+                av_log(s, AV_LOG_WARNING,
+                       "WARNING codec timebase is very high. If
duration is too long,\n"
+                       "file may not be playable by Apple Quicktime.
Auto-setting\n"
+                       "a shorter timebase %d instead of %d.\n",
timescale_new, track->timescale);
+                track->timescale = timescale_new;
+            }
             if (st->codecpar->width > 65535 || st->codecpar->height > 65535) {
                 av_log(s, AV_LOG_ERROR, "Resolution %dx%d too large
for mov/mp4\n", st->codecpar->width, st->codecpar->height);
                 ret = AVERROR(EINVAL);
                 goto error;
             }
-            if (track->mode == MODE_MOV && track->timescale > 100000)
-                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");
             if (track->mode == MODE_MOV &&
                 track->par->codec_id == AV_CODEC_ID_RAWVIDEO &&
                 track->tag == MKTAG('r','a','w',' ')) {
                 enum AVPixelFormat pix_fmt = track->par->format;
                 if (pix_fmt == AV_PIX_FMT_NONE &&
track->par->bits_per_coded_sample == 1)
                     pix_fmt = AV_PIX_FMT_MONOWHITE;
Alexey Eromenko Oct. 11, 2016, 12:44 p.m. UTC | #4
Josh, thanks for your efforts.
Michael Niedermayer Oct. 11, 2016, 1:49 p.m. UTC | #5
On Tue, Oct 11, 2016 at 02:40:57PM +0200, Alexey Eromenko wrote:
> I have written a different patch, which should auto-fix the problem,
> but I can't compile and test it now (due to build system issues):
> movenc.c affects both MP4 and MOV containers, right ?
> 
> ====
> 
> --- ./movenc.c.orig     2016-10-11 06:24:44.328730759 -0400
> +++ ./ffmpeg-3.1.3/ffmpeg-3.1.3/libavformat/movenc.c    2016-10-11
> 08:12:50.626150035 -0400
> @@ -5533,22 +5533,26 @@
>                  track->timescale = mov->video_track_timescale;
>              } else {
>                  track->timescale = st->time_base.den;
>                  while(track->timescale < 10000)
>                      track->timescale *= 2;
>              }
> +            if (track->timescale > 100000) {
> +                int timescale_new = ((st->time_base.den) * (0x10000)) /
> +                (st->time_base.num)*1000;
> +                av_log(s, AV_LOG_WARNING,
> +                       "WARNING codec timebase is very high. If
> duration is too long,\n"

this patch is corrupted by newlines, check your word wrap settings
also patches must be git patches against git master, this is against
3.1 and not a git format-patch (lacking commit message and author)

about the code itself, theres a integer overflow in your code

[...]
Alexey Eromenko Oct. 11, 2016, 3:16 p.m. UTC | #6
GMail disallows me to paste unwrapped text, so I will use good-old
pastebin plain text.

http://pastebin.com/SPUF4NTS
Clément Bœsch Oct. 11, 2016, 3:25 p.m. UTC | #7
On Tue, Oct 11, 2016 at 05:16:30PM +0200, Alexey Eromenko wrote:
> GMail disallows me to paste unwrapped text, so I will use good-old
> pastebin plain text.
> 
> http://pastebin.com/SPUF4NTS

No, please fix or learn how to use your mailer.

BTW, what if I actually want/need a large timebase because I want a valid
file and not a crumbled one to play on a broken player?
Alexey Eromenko Oct. 11, 2016, 3:27 p.m. UTC | #8
In which cases a large timebase would help ?
diff mbox

Patch

diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index d7c7158..1f4f2b0 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -5681,7 +5681,9 @@  static int mov_write_header(AVFormatContext *s)
                 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. You may find the -video_track_timescale\n"
+                       "flag useful to forcibly set a specific timescale. Ex. 30000/1001 FPS\n"
+                       "would be -video_track_timescale 30000.\n");
             if (track->mode == MODE_MOV &&
                 track->par->codec_id == AV_CODEC_ID_RAWVIDEO &&
                 track->tag == MKTAG('r','a','w',' ')) {