| Message ID | AS8P250MB07440B2119F95B5951BA54298FEEA@AS8P250MB0744.EURP250.PROD.OUTLOOK.COM |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,01/21] avformat/avio: Don't use incompatible function pointer type for call | expand |
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. Regards, Marton > { > int ret, len; > int fast_retries = 5; > @@ -367,7 +372,8 @@ static inline int retry_transfer_wrapper(URLContext *h, uint8_t *buf, > while (len < size_min) { > if (ff_check_interrupt(&h->interrupt_callback)) > return AVERROR_EXIT; > - ret = transfer_func(h, buf + len, size - len); > + ret = read ? read_func (h, buf + len, size - len) > + : write_func(h, cbuf + len, size - len); > if (ret == AVERROR(EINTR)) > continue; > if (h->flags & AVIO_FLAG_NONBLOCK) > @@ -402,14 +408,16 @@ int ffurl_read(URLContext *h, unsigned char *buf, int size) > { > if (!(h->flags & AVIO_FLAG_READ)) > return AVERROR(EIO); > - return retry_transfer_wrapper(h, buf, size, 1, h->prot->url_read); > + return retry_transfer_wrapper(h, buf, NULL, size, 1, > + h->prot->url_read, NULL, 1); > } > > int ffurl_read_complete(URLContext *h, unsigned char *buf, int size) > { > if (!(h->flags & AVIO_FLAG_READ)) > return AVERROR(EIO); > - return retry_transfer_wrapper(h, buf, size, size, h->prot->url_read); > + return retry_transfer_wrapper(h, buf, NULL, size, size, > + h->prot->url_read, NULL, 1); > } > > int ffurl_write(URLContext *h, const unsigned char *buf, int size) > @@ -420,9 +428,8 @@ int ffurl_write(URLContext *h, const unsigned char *buf, int size) > if (h->max_packet_size && size > h->max_packet_size) > return AVERROR(EIO); > > - return retry_transfer_wrapper(h, (unsigned char *)buf, size, size, > - (int (*)(struct URLContext *, uint8_t *, int)) > - h->prot->url_write); > + return retry_transfer_wrapper(h, NULL, buf, size, size, > + NULL, h->prot->url_write, 0); > } > > int64_t ffurl_seek(URLContext *h, int64_t pos, int whence) > -- > 2.34.1 > > _______________________________________________ > 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". >
tor 2023-09-07 klockan 02:23 +0200 skrev Andreas Rheinhardt: > 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(-) Looks OK. It's probably possible to get around the need for cbuf by casting to/from uintptr_t, but using cbuf is more type safe /Tomas
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 /Tomas
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). Regards, Marton
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.) - Andreas
Tomas Härdin: > tor 2023-09-07 klockan 02:23 +0200 skrev Andreas Rheinhardt: >> 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(-) > > Looks OK. It's probably possible to get around the need for cbuf by > casting to/from uintptr_t, but using cbuf is more type safe > I just sent a new version to accomodate Marton's objections; are you ok with it? - Andreas
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(); But v2 is also fine, use whichever you prefer. Thanks, Marton
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
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) { int ret, len; int fast_retries = 5; @@ -367,7 +372,8 @@ static inline int retry_transfer_wrapper(URLContext *h, uint8_t *buf, while (len < size_min) { if (ff_check_interrupt(&h->interrupt_callback)) return AVERROR_EXIT; - ret = transfer_func(h, buf + len, size - len); + ret = read ? read_func (h, buf + len, size - len) + : write_func(h, cbuf + len, size - len); if (ret == AVERROR(EINTR)) continue; if (h->flags & AVIO_FLAG_NONBLOCK) @@ -402,14 +408,16 @@ int ffurl_read(URLContext *h, unsigned char *buf, int size) { if (!(h->flags & AVIO_FLAG_READ)) return AVERROR(EIO); - return retry_transfer_wrapper(h, buf, size, 1, h->prot->url_read); + return retry_transfer_wrapper(h, buf, NULL, size, 1, + h->prot->url_read, NULL, 1); } int ffurl_read_complete(URLContext *h, unsigned char *buf, int size) { if (!(h->flags & AVIO_FLAG_READ)) return AVERROR(EIO); - return retry_transfer_wrapper(h, buf, size, size, h->prot->url_read); + return retry_transfer_wrapper(h, buf, NULL, size, size, + h->prot->url_read, NULL, 1); } int ffurl_write(URLContext *h, const unsigned char *buf, int size) @@ -420,9 +428,8 @@ int ffurl_write(URLContext *h, const unsigned char *buf, int size) if (h->max_packet_size && size > h->max_packet_size) return AVERROR(EIO); - return retry_transfer_wrapper(h, (unsigned char *)buf, size, size, - (int (*)(struct URLContext *, uint8_t *, int)) - h->prot->url_write); + return retry_transfer_wrapper(h, NULL, buf, size, size, + NULL, h->prot->url_write, 0); } int64_t ffurl_seek(URLContext *h, int64_t pos, int whence)
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(-)