On Tue, Oct 11, 2016 at 8:29 PM, Josh de Kock <j...@itanimul.li> wrote: > On 11/10/2016 18:24, Alexey Eromenko wrote: >> >> --- >> libavformat/movenc.c | 14 +++++++++----- >> 1 file changed, 9 insertions(+), 5 deletions(-) >> >> diff --git a/libavformat/movenc.c b/libavformat/movenc.c >> index 8b4aa5f..0e2fc55 100644 >> --- a/libavformat/movenc.c >> +++ b/libavformat/movenc.c >> @@ -5666,16 +5666,20 @@ static int mov_write_header(AVFormatContext *s) >> while(track->timescale < 10000) >> track->timescale *= 2; >> } >> + if (track->timescale > 100000 && >> (!mov->video_track_timescale)) { > > As I said before, having this as 'default' behaviour would interfere with > large, but correct timescales. This should only be a suggestion to the user. >
This happens only for very high timescale from source video, and it seems to work on Apple QuickTime/iTunes, WMP and VLC. For the vast majority videos (99%+), I _do not_ touch the timebase. And when I do touch, I give a WARNING to the user. Plus I offer to override this decision via a parameter. It should be stable and regression-free for most of our users. Is there any downside for this approach ? >> + unsigned int timescale_new = (unsigned >> int)((double)(st->time_base.den) >> + * 1000 / (double)(st->time_base.num)); > > You surely don't need all these casts. Unless I'm guaranteed to get int64 on ALL platforms, I don't see how-to write this code without casts to double-float. Int32 will over-flow, even unsigned. I can do the multiply after the division, but afraid of losing precision. And since this is not a real-time code anyway, I consider this an "okay" trade-off. Feel free to replace it with a better alternative. >> + 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 %u 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"); > > Keep the logic in the same place please. Logically timescale part 1 and timescale part 2 belong together, so I grouped them together. if (mov->video_track_timescale) { track->timescale = mov->video_track_timescale; } else { and if (track->timescale > 100000) { unsigned int timescale_new = (unsigned int)((double)(st->time_base.den) Feel free to modify my code and commit, after we reach some rough consensus on the matters. -- -Alexey Eromenko "Technologov" _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel