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

[Andreas Rheinhardt]

James Almer:
> On 8/4/2021 1:48 PM, Andreas Rheinhardt wrote:
>> 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.
> 
> Something similar to what you did in patch 1/1 for AVIOContext +
> DynBuffer in a single allocation? It should be ok if not too ugly. I'd
> rather not have non public fields in public headers if possible.
> 

Ok, then I'll work on this.

- Andreas