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

James Almer jamrial at gmail.com
Wed Aug 4 20:01:52 EEST 2021


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.

> 
> (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;
>>>      /**
>>>
>>
> 
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
> 



More information about the ffmpeg-devel mailing list