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

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

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@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();
> 

I don't get how this would help (apart from the fact that you switched
write and read): The compiler knows that cbuf is NULL for reading, which
means it can optimize away the second if, but it doesn't know whether
buf is NULL and therefore will add an explicit (and unnecessary) check
in the first if. So this is worse code.
And apart from that, such a version would also rely on passing buf and
cbuf and (potentially) the callbacks; I consider this worse and more
obscure than a simple read parameter.

> But v2 is also fine, use whichever you prefer.
> 

Will do as soon as Tomas ok's this version.

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".