[FFmpeg-devel] [PATCH 01/21] avformat/avio: Don't use incompatible function pointer type for call

[Andreas Rheinhardt]

Marton Balint:
> 
> 
> On Sat, 9 Sep 2023, Tomas Härdin wrote:
> 
>> fre 2023-09-08 klockan 22:38 +0200 skrev Marton Balint:
>>>
>>>
>>> On Thu, 7 Sep 2023, Andreas Rheinhardt wrote:
>>>
>>> > It is undefined behaviour even in cases where it works
>>> > (it works because it is only a const uint8_t* vs. uint8_t*
>>> > difference).
>>> > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at outlook.com>
>>> > ---
>>> > libavformat/avio.c | 25 ++++++++++++++++---------
>>> > 1 file changed, 16 insertions(+), 9 deletions(-)
>>> > > diff --git a/libavformat/avio.c b/libavformat/avio.c
>>> > index ab1c19a58d..d53da5cb0c 100644
>>> > --- a/libavformat/avio.c
>>> > +++ b/libavformat/avio.c
>>> > @@ -354,10 +354,15 @@ fail:
>>> > }
>>> > > static inline int retry_transfer_wrapper(URLContext *h, uint8_t
>>> > *buf,
>>> > +                                         const uint8_t *cbuf,
>>> >                                          int size, int size_min,
>>> > -                                         int
>>> > (*transfer_func)(URLContext *h,
>>> > -                                                             
>>> > uint8_t *buf,
>>> > -                                                              int
>>> > size))
>>> > +                                         int
>>> > (*read_func)(URLContext *h,
>>> > +                                                          uint8_t
>>> > *buf,
>>> > +                                                          int
>>> > size),
>>> > +                                         int
>>> > (*write_func)(URLContext *h,
>>> > +                                                           const
>>> > uint8_t *buf,
>>> > +                                                           int
>>> > size),
>>> > +                                         int read)
>>>
>>> These extra parameters are very ugly, can't we think of another way
>>> to properly support this?
>>>
>>> One idea is putting retry_transfer_wrapper in a template file and
>>> include it twice with proper defines-s for the read and write flavours.
>>
>> Seems like a lot of work for a function that's internal to avio.c
> 
> If future extensibility is not important here then function pointers
> should not be passed to retry_tranfer_wrapper because
> h->prot->url_read/write can be used directly. And usage of buf/cbuf is
> readundant with the read paramter, because by checking if buf or cbuf is
> null you can decide the operation (read of write).
> 

The compiler does not know whether buf given to
ffurl_(read|write|read_complete) is NULL or not in the first place (it
also does not know whether the url_read and url_write function pointers
are NULL or not); therefore if one use e.g. cbuf != NULL as meaning read
== 0, then the write function would actually check for whether cbuf is
NULL which is worse than it is now.
(My initial version (not sent to this list) checked for whether the read
function was NULL in order to determine whether we are reading or
writing; the assumption was that the compiler would optimize the check
away when reading, because if the read function were NULL, then a NULL
function pointer would be used for a call, which is undefined behaviour.
But it didn't. Instead it added ffurl_read.cold and
ffurl_read_complete.cold functions (which presumably abort or so) for
this case.)

- Andreas