[FFmpeg-devel] [PATCH 4/4] avformat/avio: Move internal AVIOContext field to the end

[Andreas Rheinhardt]

James Almer:
> On 8/4/2021 1:24 PM, Andreas Rheinhardt wrote:
>> This gets them off the ABI altogether at the cost of breaking the ABI
>> once more now.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
>> ---
>> Do this and the preceding patch need a version bump?
> 
> You're not removing anything that was "public", so IMO no.
> 
> Changes in offsets are ok since the latest AVPacket addition was a few
> days ago, so the ABI is not frozen yet (And at this rate wont be until
> we branch out a 5.0 release).
> 

Good. Because I intend to send another patchset that breaks ABI (but
only the avpriv-ABI, not the public ABI).

>>
>>   libavformat/avio.h | 98 ++++++++++++++++++++++------------------------
>>   1 file changed, 47 insertions(+), 51 deletions(-)
>>
>> diff --git a/libavformat/avio.h b/libavformat/avio.h
>> index 0b35409787..ebf43bfe15 100644
>> --- a/libavformat/avio.h
>> +++ b/libavformat/avio.h
>> @@ -148,9 +148,9 @@ enum AVIODataMarkerType {
>>     /**
>>    * Bytestream IO Context.
>> - * New fields can be added to the end with minor version bumps.
>> - * Removal, reordering and changes to existing fields require a major
>> - * version bump.
>> + * New public fields can be added with minor version bumps.
>> + * Removal, reordering and changes to existing public fields require
>> + * a major version bump.
>>    * sizeof(AVIOContext) must not be used outside libav*.
>>    *
>>    * @note None of the function pointers in AVIOContext should be called
>> @@ -237,12 +237,14 @@ typedef struct AVIOContext {
>>       int64_t (*seek)(void *opaque, int64_t offset, int whence);
>>       int64_t pos;            /**< position in the file of the current
>> buffer */
>>       int eof_reached;        /**< true if was unable to read due to
>> error or eof */
>> +    int error;              /**< contains the error code or 0 if no
>> error happened */
>>       int write_flag;         /**< true if open for writing */
>>       int max_packet_size;
>> +    int min_packet_size;    /**< Try to buffer at least this amount
>> of data
>> +                                 before flushing it. */
>>       unsigned long checksum;
>>       unsigned char *checksum_ptr;
>>       unsigned long (*update_checksum)(unsigned long checksum, const
>> uint8_t *buf, unsigned int size);
>> -    int error;              /**< contains the error code or 0 if no
>> error happened */
>>       /**
>>        * Pause or resume playback for network streaming protocols -
>> e.g. MMS.
>>        */
>> @@ -259,12 +261,6 @@ typedef struct AVIOContext {
>>        */
>>       int seekable;
>>   -    /**
>> -     * max filesize, used to limit allocations
>> -     * This field is internal to libavformat and access from outside
>> is not allowed.
>> -     */
>> -    int64_t maxsize;
>> -
>>       /**
>>        * avio_read and avio_write should if possible be satisfied
>> directly
>>        * instead of going through a buffer, and avio_seek will always
>> @@ -272,37 +268,6 @@ typedef struct AVIOContext {
>>        */
>>       int direct;
>>   -    /**
>> -     * Bytes read statistic
>> -     * This field is internal to libavformat and access from outside
>> is not allowed.
>> -     */
>> -    int64_t bytes_read;
>> -
>> -    /**
>> -     * seek statistic
>> -     * This field is internal to libavformat and access from outside
>> is not allowed.
>> -     */
>> -    int seek_count;
>> -
>> -    /**
>> -     * writeout statistic
>> -     * This field is internal to libavformat and access from outside
>> is not allowed.
>> -     */
>> -    int writeout_count;
>> -
>> -    /**
>> -     * Original buffer size
>> -     * used internally after probing and ensure seekback to reset the
>> buffer size
>> -     * This field is internal to libavformat and access from outside
>> is not allowed.
>> -     */
>> -    int orig_buffer_size;
>> -
>> -    /**
>> -     * Threshold to favor readahead over seek.
>> -     * This is current internal only, do not use from outside.
>> -     */
>> -    int short_seek_threshold;
>> -
>>       /**
>>        * ',' separated list of allowed protocols.
>>        */
>> @@ -325,30 +290,61 @@ typedef struct AVIOContext {
>>        */
>>       int ignore_boundary_point;
>>   +    int64_t written;
>> +
>> +    /**
>> +     * Maximum reached position before a backward seek in the write
>> buffer,
>> +     * used keeping track of already written data for a later flush.
>> +     */
>> +    unsigned char *buf_ptr_max;
>> +
>> +    /*****************************************************************
>> +     * No fields below this line are part of the public API. They
>> +     * may not be used outside of libavformat and can be changed and
>> +     * removed at will.
>> +     * New public fields should be added right above.
>> +     *****************************************************************
> 
> Since we got rid of this kind of notice from AVStream not too long ago,
> wouldn't it be better if we prevent it from happening here too?
> You could introduce a new AVIOInternal opaque struct and field here,
> much like we did with AVStreamInternal and AVFormatInternal to solve the
> same problem.
> 

This would add another allocation, so no. Unless you would be ok with
allocating said internal together with AVIOContext.

(Contrary to AVStream the internals of AVIOContext are not used that
widely, so it would be way less code to adapt.)

>> +     */
>> +    /**
>> +     * A callback that is used instead of short_seek_threshold.
>> +     * This is currently internal only, do not use from outside.
>> +     */
>> +    int (*short_seek_get)(void *opaque);
>> +
>>       /**
>> -     * Internal, not meant to be used from outside of AVIOContext.
>> +     * Threshold to favor readahead over seek.
>> +     * This is currently internal only, do not use from outside.
>>        */
>> +    int short_seek_threshold;
>> +
>>       enum AVIODataMarkerType current_type;
>>       int64_t last_time;
>>         /**
>> -     * A callback that is used instead of short_seek_threshold.
>> -     * This is current internal only, do not use from outside.
>> +     * max filesize, used to limit allocations
>>        */
>> -    int (*short_seek_get)(void *opaque);
>> +    int64_t maxsize;
>>   -    int64_t written;
>> +    /**
>> +     * Bytes read statistic
>> +     */
>> +    int64_t bytes_read;
>>         /**
>> -     * Maximum reached position before a backward seek in the write
>> buffer,
>> -     * used keeping track of already written data for a later flush.
>> +     * seek statistic
>>        */
>> -    unsigned char *buf_ptr_max;
>> +    int seek_count;
>> +
>> +    /**
>> +     * writeout statistic
>> +     */
>> +    int writeout_count;
>>         /**
>> -     * Try to buffer at least this amount of data before flushing it
>> +     * Original buffer size
>> +     * used after probing to ensure seekback and to reset the buffer
>> size
>>        */
>> -    int min_packet_size;
>> +    int orig_buffer_size;
>>   } AVIOContext;
>>     /**
>>
>