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.rheinha...@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 _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org https://ffmpeg.org/mailman/listinfo/ffmpeg-devel To unsubscribe, visit link above, or email ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
