[FFmpeg-devel] [PATCH] Apple Quicktime fix

[Alexey Eromenko]

On Tue, Oct 11, 2016 at 8:29 PM, Josh de Kock <josh at 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"