diff mbox series

[FFmpeg-devel] Increase MOV_TIMESCALE for higher precision

Message ID 1616537325-620485-1-git-send-email-andrey.rikunov@gmail.com
State New
Headers show
Series [FFmpeg-devel] Increase MOV_TIMESCALE for higher precision
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate fail Make fate failed
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate warning Make fate failed

Commit Message

Andrey Rikunov March 23, 2021, 10:08 p.m. UTC
---
 libavformat/movenc.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Gyan Doshi March 24, 2021, 5:30 a.m. UTC | #1
On 2021-03-24 03:38, Andrey Rikunov wrote:
> ---
>   libavformat/movenc.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavformat/movenc.h b/libavformat/movenc.h
> index cdbc407..8a152c0 100644
> --- a/libavformat/movenc.h
> +++ b/libavformat/movenc.h
> @@ -29,7 +29,7 @@
>   
>   #define MOV_FRAG_INFO_ALLOC_INCREMENT 64
>   #define MOV_INDEX_CLUSTER_SIZE 1024
> -#define MOV_TIMESCALE 1000
> +#define MOV_TIMESCALE 60000

Instead of hardcoding it, add an option to allow the user to set it.

Regards,
Gyan
Jan Ekström March 24, 2021, 10:15 a.m. UTC | #2
On Wed, Mar 24, 2021 at 7:30 AM Gyan Doshi <ffmpeg@gyani.pro> wrote:
>
>
>
> On 2021-03-24 03:38, Andrey Rikunov wrote:
> > ---
> >   libavformat/movenc.h | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavformat/movenc.h b/libavformat/movenc.h
> > index cdbc407..8a152c0 100644
> > --- a/libavformat/movenc.h
> > +++ b/libavformat/movenc.h
> > @@ -29,7 +29,7 @@
> >
> >   #define MOV_FRAG_INFO_ALLOC_INCREMENT 64
> >   #define MOV_INDEX_CLUSTER_SIZE 1024
> > -#define MOV_TIMESCALE 1000
> > +#define MOV_TIMESCALE 60000
>
> Instead of hardcoding it, add an option to allow the user to set it.

I think if the "Mov requires timescale 1000" assumption is no longer
correct, it should be removed and the generic time base based
timescale generation utilized instead, which is utilized for
MP4-likes. That way by setting time base one can affect it with the
standard option.

For the general logic (for MP4- likes), I am planning on getting rid
of the video time scale option in the following steps:
1. Figure out what the historical reasons for having a video time
scale of over 10k are (I think it was mostly to adjust A/V sync with
more granularity than frame rate - but we have edit lists for that
now?).
2. If those still hold, make a "auto_adjust_timescale" boolean, which
would first be on by default.
3. Make video_timescale disable the auto-adjustment of video
timescale, and set the time base earlier in the muxer logic (since
muxers are allowed to override the AVStreams' time base).
4. Deprecate video_timescale and point people towards
-auto_adjust_timescale false/0 -time_base:v "1:NUMBER"

I have seen people add various timescale related options to movenc in
their trees for audio/subtitles just because it's not clear that we
already have time_base :s . Thus this just needs to be cleaned up. And
if MOV still needs its timescale override of 1:1000 for some
compatibility reasons (?) then "auto_adjust_timescale" can also poke
at that.

Jan
Gyan Doshi March 24, 2021, 11:42 a.m. UTC | #3
On 2021-03-24 15:45, Jan Ekström wrote:
> On Wed, Mar 24, 2021 at 7:30 AM Gyan Doshi <ffmpeg@gyani.pro> wrote:
>>
>>
>> On 2021-03-24 03:38, Andrey Rikunov wrote:
>>> ---
>>>    libavformat/movenc.h | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavformat/movenc.h b/libavformat/movenc.h
>>> index cdbc407..8a152c0 100644
>>> --- a/libavformat/movenc.h
>>> +++ b/libavformat/movenc.h
>>> @@ -29,7 +29,7 @@
>>>
>>>    #define MOV_FRAG_INFO_ALLOC_INCREMENT 64
>>>    #define MOV_INDEX_CLUSTER_SIZE 1024
>>> -#define MOV_TIMESCALE 1000
>>> +#define MOV_TIMESCALE 60000
>> Instead of hardcoding it, add an option to allow the user to set it.
> if MOV still needs its timescale override of 1:1000 for some

To be clear, the MOV_TIMESCALE refers to the timescale written in the 
mvhd atom in all products of movenc.c. It is distinct from the 
stream/track timescales. The mvhd value is the basis for the duration 
field in edit list entries but not media time which is denominated in 
terms of track timescale ( what video_track_timescale targets but only 
for video streams). Users in the past have asked for a change (usually 
to 600) because some Apple products (FCP ?) only work well with 600 for 
mvhd timescale. There is no strict inter-relation or constraint across 
mvhd and the trak timescales, so auto_adjust_timescale shouldn't force 
it in any hardcoded manner.

Regards,
Gyan
Andrey Rikunov March 24, 2021, 1 p.m. UTC | #4
Gyan, thanks for clarification! It is all exactly as you described. I
thought to set MOV_TIMESCALE to either 60kHz or 90kHz as both are
divisible by most common fps values. And x10 increase is for further
precision in conversion to seconds. Say it's needed to
write segmentDuration=289.4892 sec in elst to start presentation time from.
Using timescale of 1000 we lose 0,0002 sec which turns into pts drift by 6
points for video track timescale of 30kHz.

IMHO there is no need to pull MOV_TIMESCALE up to cli arg because there is
no need to control it. The only requirement is best precision. What do you
think?

On Wed, Mar 24, 2021 at 2:42 PM Gyan Doshi <ffmpeg@gyani.pro> wrote:

>
>
> On 2021-03-24 15:45, Jan Ekström wrote:
> > On Wed, Mar 24, 2021 at 7:30 AM Gyan Doshi <ffmpeg@gyani.pro> wrote:
> >>
> >>
> >> On 2021-03-24 03:38, Andrey Rikunov wrote:
> >>> ---
> >>>    libavformat/movenc.h | 2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/libavformat/movenc.h b/libavformat/movenc.h
> >>> index cdbc407..8a152c0 100644
> >>> --- a/libavformat/movenc.h
> >>> +++ b/libavformat/movenc.h
> >>> @@ -29,7 +29,7 @@
> >>>
> >>>    #define MOV_FRAG_INFO_ALLOC_INCREMENT 64
> >>>    #define MOV_INDEX_CLUSTER_SIZE 1024
> >>> -#define MOV_TIMESCALE 1000
> >>> +#define MOV_TIMESCALE 60000
> >> Instead of hardcoding it, add an option to allow the user to set it.
> > if MOV still needs its timescale override of 1:1000 for some
>
> To be clear, the MOV_TIMESCALE refers to the timescale written in the
> mvhd atom in all products of movenc.c. It is distinct from the
> stream/track timescales. The mvhd value is the basis for the duration
> field in edit list entries but not media time which is denominated in
> terms of track timescale ( what video_track_timescale targets but only
> for video streams). Users in the past have asked for a change (usually
> to 600) because some Apple products (FCP ?) only work well with 600 for
> mvhd timescale. There is no strict inter-relation or constraint across
> mvhd and the trak timescales, so auto_adjust_timescale shouldn't force
> it in any hardcoded manner.
>
> Regards,
> Gyan
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavformat/movenc.h b/libavformat/movenc.h
index cdbc407..8a152c0 100644
--- a/libavformat/movenc.h
+++ b/libavformat/movenc.h
@@ -29,7 +29,7 @@ 
 
 #define MOV_FRAG_INFO_ALLOC_INCREMENT 64
 #define MOV_INDEX_CLUSTER_SIZE 1024
-#define MOV_TIMESCALE 1000
+#define MOV_TIMESCALE 60000
 
 #define RTP_MAX_PACKET_SIZE 1450