diff mbox series

[FFmpeg-devel,18/18] lavf: document some AVStream fields as private

Message ID 20201009130430.602-18-anton@khirnov.net
State New
Headers show
Series [FFmpeg-devel,01/18] lavf: move AVStream.info to AVStreamInternal
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Anton Khirnov Oct. 9, 2020, 1:04 p.m. UTC
Specifically: pts_wrap_bits, first_dts, cur_dts.
They are supposed to be private and are located in the private section
of AVStream, but ffmpeg.c currently accesses them regardless. They
should be moved to AVStreamInternal once that bug is fixed.

Remove the marker for the private AVStream section, as there are no
other accessible fields left there. It has proven highly confusing and
people kept adding supposedly-public fields into the private section.
New private per-stream fields should be added to AVStreamInternal.
---
 libavformat/avformat.h | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

Comments

Michael Niedermayer Oct. 9, 2020, 9:55 p.m. UTC | #1
On Fri, Oct 09, 2020 at 03:04:30PM +0200, Anton Khirnov wrote:
> Specifically: pts_wrap_bits, first_dts, cur_dts.
> They are supposed to be private and are located in the private section
> of AVStream, but ffmpeg.c currently accesses them regardless. They
> should be moved to AVStreamInternal once that bug is fixed.
> 
> Remove the marker for the private AVStream section, as there are no
> other accessible fields left there. It has proven highly confusing and
> people kept adding supposedly-public fields into the private section.
> New private per-stream fields should be added to AVStreamInternal.
> ---
>  libavformat/avformat.h | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
> index a01912d654..612791a2eb 100644
> --- a/libavformat/avformat.h
> +++ b/libavformat/avformat.h
> @@ -1013,22 +1013,16 @@ typedef struct AVStream {
>       */
>      AVCodecParameters *codecpar;
>  
> -    /*****************************************************************
> -     * All fields below this line are not part of the public API. They
> -     * may not be used outside of libavformat and can be changed and
> -     * removed at will.
> -     * Internal note: be aware that physically removing these fields
> -     * will break ABI. Replace removed fields with dummy fields, and
> -     * add new fields to AVStreamInternal.
> -     *****************************************************************
> -     */
> -
>  #if LIBAVFORMAT_VERSION_MAJOR < 59
>      // kept for ABI compatibility only, do not access in any way
>      void *unused;
>  #endif
>  

> -    int pts_wrap_bits; /**< number of bits in pts (used for wrapping control) */
> +    /**
> +     * number of bits in pts (used for wrapping control)
> +     * private, do not access from outside libavformat.
> +     */
> +    int pts_wrap_bits;

This is maybe a really bad way to export "pts_wrap_bits" but i think
User applications could quite reasonably need to know at which point pts wrap.
Or whats the max duration for a timebase where pts are still unique or valid.

Based on this a user app might warn the user at the begin of transcoding that
timestamps will wrap and that with non streaming output, wrap might equal fail.

so maybe this should not be declared private without replacement.

thx

[...]
James Almer Oct. 9, 2020, 9:57 p.m. UTC | #2
On 10/9/2020 6:55 PM, Michael Niedermayer wrote:
> On Fri, Oct 09, 2020 at 03:04:30PM +0200, Anton Khirnov wrote:
>> Specifically: pts_wrap_bits, first_dts, cur_dts.
>> They are supposed to be private and are located in the private section
>> of AVStream, but ffmpeg.c currently accesses them regardless. They
>> should be moved to AVStreamInternal once that bug is fixed.
>>
>> Remove the marker for the private AVStream section, as there are no
>> other accessible fields left there. It has proven highly confusing and
>> people kept adding supposedly-public fields into the private section.
>> New private per-stream fields should be added to AVStreamInternal.
>> ---
>>  libavformat/avformat.h | 20 +++++++++-----------
>>  1 file changed, 9 insertions(+), 11 deletions(-)
>>
>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>> index a01912d654..612791a2eb 100644
>> --- a/libavformat/avformat.h
>> +++ b/libavformat/avformat.h
>> @@ -1013,22 +1013,16 @@ typedef struct AVStream {
>>       */
>>      AVCodecParameters *codecpar;
>>  
>> -    /*****************************************************************
>> -     * All fields below this line are not part of the public API. They
>> -     * may not be used outside of libavformat and can be changed and
>> -     * removed at will.
>> -     * Internal note: be aware that physically removing these fields
>> -     * will break ABI. Replace removed fields with dummy fields, and
>> -     * add new fields to AVStreamInternal.
>> -     *****************************************************************
>> -     */
>> -
>>  #if LIBAVFORMAT_VERSION_MAJOR < 59
>>      // kept for ABI compatibility only, do not access in any way
>>      void *unused;
>>  #endif
>>  
> 
>> -    int pts_wrap_bits; /**< number of bits in pts (used for wrapping control) */
>> +    /**
>> +     * number of bits in pts (used for wrapping control)
>> +     * private, do not access from outside libavformat.
>> +     */
>> +    int pts_wrap_bits;
> 
> This is maybe a really bad way to export "pts_wrap_bits" but i think
> User applications could quite reasonably need to know at which point pts wrap.
> Or whats the max duration for a timebase where pts are still unique or valid.
> 
> Based on this a user app might warn the user at the begin of transcoding that
> timestamps will wrap and that with non streaming output, wrap might equal fail.
> 
> so maybe this should not be declared private without replacement.

It already is private. This commit doesn't change that.

> 
> thx
> 
> [...]
> 
> 
> _______________________________________________
> 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".
>
James Almer Oct. 9, 2020, 10:11 p.m. UTC | #3
On 10/9/2020 6:57 PM, James Almer wrote:
> On 10/9/2020 6:55 PM, Michael Niedermayer wrote:
>> On Fri, Oct 09, 2020 at 03:04:30PM +0200, Anton Khirnov wrote:
>>> Specifically: pts_wrap_bits, first_dts, cur_dts.
>>> They are supposed to be private and are located in the private section
>>> of AVStream, but ffmpeg.c currently accesses them regardless. They
>>> should be moved to AVStreamInternal once that bug is fixed.
>>>
>>> Remove the marker for the private AVStream section, as there are no
>>> other accessible fields left there. It has proven highly confusing and
>>> people kept adding supposedly-public fields into the private section.
>>> New private per-stream fields should be added to AVStreamInternal.
>>> ---
>>>  libavformat/avformat.h | 20 +++++++++-----------
>>>  1 file changed, 9 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h
>>> index a01912d654..612791a2eb 100644
>>> --- a/libavformat/avformat.h
>>> +++ b/libavformat/avformat.h
>>> @@ -1013,22 +1013,16 @@ typedef struct AVStream {
>>>       */
>>>      AVCodecParameters *codecpar;
>>>  
>>> -    /*****************************************************************
>>> -     * All fields below this line are not part of the public API. They
>>> -     * may not be used outside of libavformat and can be changed and
>>> -     * removed at will.
>>> -     * Internal note: be aware that physically removing these fields
>>> -     * will break ABI. Replace removed fields with dummy fields, and
>>> -     * add new fields to AVStreamInternal.
>>> -     *****************************************************************
>>> -     */
>>> -
>>>  #if LIBAVFORMAT_VERSION_MAJOR < 59
>>>      // kept for ABI compatibility only, do not access in any way
>>>      void *unused;
>>>  #endif
>>>  
>>
>>> -    int pts_wrap_bits; /**< number of bits in pts (used for wrapping control) */
>>> +    /**
>>> +     * number of bits in pts (used for wrapping control)
>>> +     * private, do not access from outside libavformat.
>>> +     */
>>> +    int pts_wrap_bits;
>>
>> This is maybe a really bad way to export "pts_wrap_bits" but i think
>> User applications could quite reasonably need to know at which point pts wrap.
>> Or whats the max duration for a timebase where pts are still unique or valid.
>>
>> Based on this a user app might warn the user at the begin of transcoding that
>> timestamps will wrap and that with non streaming output, wrap might equal fail.
>>
>> so maybe this should not be declared private without replacement.
> 
> It already is private. This commit doesn't change that.

I'm not saying that introducing a proper way to export this same
information is a bad idea (It would in fact simply "fixing" the relevant
code in ffmpeg.c), just that this commit is merely moving a notification
from one part of the struct to another. The field itself remains
unaffected after this change.

> 
>>
>> thx
>>
>> [...]
>>
>>
>> _______________________________________________
>> 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/avformat.h b/libavformat/avformat.h
index a01912d654..612791a2eb 100644
--- a/libavformat/avformat.h
+++ b/libavformat/avformat.h
@@ -1013,22 +1013,16 @@  typedef struct AVStream {
      */
     AVCodecParameters *codecpar;
 
-    /*****************************************************************
-     * All fields below this line are not part of the public API. They
-     * may not be used outside of libavformat and can be changed and
-     * removed at will.
-     * Internal note: be aware that physically removing these fields
-     * will break ABI. Replace removed fields with dummy fields, and
-     * add new fields to AVStreamInternal.
-     *****************************************************************
-     */
-
 #if LIBAVFORMAT_VERSION_MAJOR < 59
     // kept for ABI compatibility only, do not access in any way
     void *unused;
 #endif
 
-    int pts_wrap_bits; /**< number of bits in pts (used for wrapping control) */
+    /**
+     * number of bits in pts (used for wrapping control)
+     * private, do not access from outside libavformat.
+     */
+    int pts_wrap_bits;
 
     // Timestamp generation support:
     /**
@@ -1037,8 +1031,12 @@  typedef struct AVStream {
      * Initialized when AVCodecParserContext.dts_sync_point >= 0 and
      * a DTS is received from the underlying container. Otherwise set to
      * AV_NOPTS_VALUE by default.
+     * private, do not access from outside libavformat.
      */
     int64_t first_dts;
+    /**
+     * private, do not access from outside libavformat.
+     */
     int64_t cur_dts;
 
 #if LIBAVFORMAT_VERSION_MAJOR < 59