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

[Marton Balint]

On Sun, 10 Sep 2023, Andreas Rheinhardt wrote:

> 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.)

Maybe this could work to make the compiler optimize away the undeeded one:

if (buf && !cbuf)
   write();
if (!buf && cbuf)
   read();

But v2 is also fine, use whichever you prefer.

Thanks,
Marton