[FFmpeg-devel] Apple Quicktime fix

---
 libavformat/movenc.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

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.

> +                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.

> +                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.

>              if (track->mode == MODE_MOV &&
>                  track->par->codec_id == AV_CODEC_ID_RAWVIDEO &&
>                  track->tag == MKTAG('r','a','w',' ')) {
>

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

>>> +                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.
>

Do you suggest moving the multiplication after the division ?

-Alexey

Hi!

> Am 11.10.2016 um 19:24 schrieb Alexey Eromenko <al4321@gmail.com>:


> -            if (track->mode == MODE_MOV

This change is not acceptable:
The problem only affects mov, not isom.
Mentioning the option name in the existing warning message is of course a good idea.

Carl Eugen

On Tuesday, 11 October 2016, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:

> Hi!
>
> > Am 11.10.2016 um 19:24 schrieb Alexey Eromenko <al4321@gmail.com
> <javascript:;>>:
>
>
> > -            if (track->mode == MODE_MOV
>
> This change is not acceptable:
> The problem only affects mov, not isom.
> Mentioning the option name in the existing warning message is of course a
> good idea.


The problem affects both MOV and MP4 on Apple decoders. My goal is to
create a video file that is cross platform, works everywhere. The only way
to achieve that goal is to use a subset of MP4 specification. As long as my
patch doesn't break video for other people, and fixes this use case, I
recommend to apply it.

Hi,

On Oct 11, 2016 2:40 PM, "Alexey Eromenko" <al4321@gmail.com> wrote:
>
> On Tue, Oct 11, 2016 at 8:29 PM, Josh de Kock <josh@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.

We use int64_t all over the place, it's OK to rely on it since it
guarantees the same result across archs, which floats don't.

Ronald