* [FFmpeg-devel] [PATCH 01/21] avformat/avio: Don't use incompatible function pointer type for call
@ 2023-09-07 0:23 Andreas Rheinhardt
2023-09-07 0:32 ` [FFmpeg-devel] [PATCH 02/21] avformat/internal: Avoid casting const away Andreas Rheinhardt
` (3 more replies)
0 siblings, 4 replies; 19+ messages in thread
From: Andreas Rheinhardt @ 2023-09-07 0:23 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: 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(-)
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)
--
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".
^ permalink raw reply [flat|nested] 19+ messages in thread
* [FFmpeg-devel] [PATCH 02/21] avformat/internal: Avoid casting const away
2023-09-07 0:23 [FFmpeg-devel] [PATCH 01/21] avformat/avio: Don't use incompatible function pointer type for call Andreas Rheinhardt
@ 2023-09-07 0:32 ` Andreas Rheinhardt
2023-09-09 6:38 ` Tomas Härdin
2023-09-07 0:32 ` [FFmpeg-devel] [PATCH 03/21] avformat/aviobuf: Don't use incompatible function pointer type for call Andreas Rheinhardt
` (2 subsequent siblings)
3 siblings, 1 reply; 19+ messages in thread
From: Andreas Rheinhardt @ 2023-09-07 0:32 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Andreas Rheinhardt
Fixes many warnings when using -Wcast-qual.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
libavformat/internal.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/libavformat/internal.h b/libavformat/internal.h
index 051e8e2893..c7512898b5 100644
--- a/libavformat/internal.h
+++ b/libavformat/internal.h
@@ -418,7 +418,7 @@ static av_always_inline FFStream *ffstream(AVStream *st)
static av_always_inline const FFStream *cffstream(const AVStream *st)
{
- return (FFStream*)st;
+ return (const FFStream*)st;
}
#ifdef __GNUC__
--
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".
^ permalink raw reply [flat|nested] 19+ messages in thread
* [FFmpeg-devel] [PATCH 03/21] avformat/aviobuf: Don't use incompatible function pointer type for call
2023-09-07 0:23 [FFmpeg-devel] [PATCH 01/21] avformat/avio: Don't use incompatible function pointer type for call Andreas Rheinhardt
2023-09-07 0:32 ` [FFmpeg-devel] [PATCH 02/21] avformat/internal: Avoid casting const away Andreas Rheinhardt
@ 2023-09-07 0:32 ` Andreas Rheinhardt
2023-09-09 6:46 ` Tomas Härdin
2023-09-08 20:38 ` [FFmpeg-devel] [PATCH 01/21] avformat/avio: " Marton Balint
2023-09-09 6:36 ` [FFmpeg-devel] [PATCH 01/21] " Tomas Härdin
3 siblings, 1 reply; 19+ messages in thread
From: Andreas Rheinhardt @ 2023-09-07 0:32 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: Andreas Rheinhardt
It is undefined behaviour even in cases where it works
(it works because both are pointers). Instead change
the functions involved to use the type expected by the AVIO-API
and add inline wrappers for our internal callers.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
libavformat/avio.c | 15 +++++++++++----
libavformat/aviobuf.c | 6 ++----
libavformat/url.h | 20 ++++++++++++++++----
3 files changed, 29 insertions(+), 12 deletions(-)
diff --git a/libavformat/avio.c b/libavformat/avio.c
index d53da5cb0c..fd40f9ce43 100644
--- a/libavformat/avio.c
+++ b/libavformat/avio.c
@@ -404,8 +404,10 @@ static inline int retry_transfer_wrapper(URLContext *h, uint8_t *buf,
return len;
}
-int ffurl_read(URLContext *h, unsigned char *buf, int size)
+int ffurl_read2(void *urlcontext, uint8_t *buf, int size)
{
+ URLContext *h = urlcontext;
+
if (!(h->flags & AVIO_FLAG_READ))
return AVERROR(EIO);
return retry_transfer_wrapper(h, buf, NULL, size, 1,
@@ -420,8 +422,10 @@ int ffurl_read_complete(URLContext *h, unsigned char *buf, int size)
h->prot->url_read, NULL, 1);
}
-int ffurl_write(URLContext *h, const unsigned char *buf, int size)
+int ffurl_write2(void *urlcontext, uint8_t *buf, int size)
{
+ URLContext *h = urlcontext;
+
if (!(h->flags & AVIO_FLAG_WRITE))
return AVERROR(EIO);
/* avoid sending too big packets */
@@ -432,8 +436,9 @@ int ffurl_write(URLContext *h, const unsigned char *buf, int size)
NULL, h->prot->url_write, 0);
}
-int64_t ffurl_seek(URLContext *h, int64_t pos, int whence)
+int64_t ffurl_seek2(void *urlcontext, int64_t pos, int whence)
{
+ URLContext *h = urlcontext;
int64_t ret;
if (!h->prot->url_seek)
@@ -654,8 +659,10 @@ int ffurl_get_multi_file_handle(URLContext *h, int **handles, int *numhandles)
return h->prot->url_get_multi_file_handle(h, handles, numhandles);
}
-int ffurl_get_short_seek(URLContext *h)
+int ffurl_get_short_seek(void *urlcontext)
{
+ URLContext *h = urlcontext;
+
if (!h || !h->prot || !h->prot->url_get_short_seek)
return AVERROR(ENOSYS);
return h->prot->url_get_short_seek(h);
diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
index 029a9e966b..ad5827f216 100644
--- a/libavformat/aviobuf.c
+++ b/libavformat/aviobuf.c
@@ -976,9 +976,7 @@ int ffio_fdopen(AVIOContext **s, URLContext *h)
return AVERROR(ENOMEM);
*s = avio_alloc_context(buffer, buffer_size, h->flags & AVIO_FLAG_WRITE, h,
- (int (*)(void *, uint8_t *, int)) ffurl_read,
- (int (*)(void *, uint8_t *, int)) ffurl_write,
- (int64_t (*)(void *, int64_t, int))ffurl_seek);
+ ffurl_read2, ffurl_write2, ffurl_seek2);
if (!*s) {
av_freep(&buffer);
return AVERROR(ENOMEM);
@@ -1006,7 +1004,7 @@ int ffio_fdopen(AVIOContext **s, URLContext *h)
if (h->prot->url_read_seek)
(*s)->seekable |= AVIO_SEEKABLE_TIME;
}
- ((FFIOContext*)(*s))->short_seek_get = (int (*)(void *))ffurl_get_short_seek;
+ ((FFIOContext*)(*s))->short_seek_get = ffurl_get_short_seek;
(*s)->av_class = &ff_avio_class;
return 0;
}
diff --git a/libavformat/url.h b/libavformat/url.h
index 3cfe3ecc5c..bba1a9a1df 100644
--- a/libavformat/url.h
+++ b/libavformat/url.h
@@ -170,6 +170,7 @@ int ffurl_accept(URLContext *s, URLContext **c);
*/
int ffurl_handshake(URLContext *c);
+int ffurl_read2(void *urlcontext, unsigned char *buf, int size);
/**
* Read up to size bytes from the resource accessed by h, and store
* the read bytes in buf.
@@ -179,7 +180,10 @@ int ffurl_handshake(URLContext *c);
* indicates that it is not possible to read more from the accessed
* resource (except if the value of the size argument is also zero).
*/
-int ffurl_read(URLContext *h, unsigned char *buf, int size);
+static inline int ffurl_read(URLContext *h, uint8_t *buf, int size)
+{
+ return ffurl_read2(h, buf, size);
+}
/**
* Read as many bytes as possible (up to size), calling the
@@ -190,14 +194,19 @@ int ffurl_read(URLContext *h, unsigned char *buf, int size);
*/
int ffurl_read_complete(URLContext *h, unsigned char *buf, int size);
+int ffurl_write2(void *urlcontext, uint8_t *buf, int size);
/**
* Write size bytes from buf to the resource accessed by h.
*
* @return the number of bytes actually written, or a negative value
* corresponding to an AVERROR code in case of failure
*/
-int ffurl_write(URLContext *h, const unsigned char *buf, int size);
+static inline int ffurl_write(URLContext *h, const uint8_t *buf, int size)
+{
+ return ffurl_write2(h, (uint8_t*)buf, size);
+}
+int64_t ffurl_seek2(void *urlcontext, int64_t pos, int whence);
/**
* Change the position that will be used by the next read/write
* operation on the resource accessed by h.
@@ -212,7 +221,10 @@ int ffurl_write(URLContext *h, const unsigned char *buf, int size);
* the beginning of the file. You can use this feature together with
* SEEK_CUR to read the current file position.
*/
-int64_t ffurl_seek(URLContext *h, int64_t pos, int whence);
+static inline int64_t ffurl_seek(URLContext *h, int64_t pos, int whence)
+{
+ return ffurl_seek2(h, pos, whence);
+}
/**
* Close the resource accessed by the URLContext h, and free the
@@ -251,7 +263,7 @@ int ffurl_get_multi_file_handle(URLContext *h, int **handles, int *numhandles);
*
* @return threshold (>0) on success or <=0 on error.
*/
-int ffurl_get_short_seek(URLContext *h);
+int ffurl_get_short_seek(void *urlcontext);
/**
* Signal the URLContext that we are done reading or writing the stream.
--
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".
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [FFmpeg-devel] [PATCH 01/21] avformat/avio: Don't use incompatible function pointer type for call
2023-09-07 0:23 [FFmpeg-devel] [PATCH 01/21] avformat/avio: Don't use incompatible function pointer type for call Andreas Rheinhardt
2023-09-07 0:32 ` [FFmpeg-devel] [PATCH 02/21] avformat/internal: Avoid casting const away Andreas Rheinhardt
2023-09-07 0:32 ` [FFmpeg-devel] [PATCH 03/21] avformat/aviobuf: Don't use incompatible function pointer type for call Andreas Rheinhardt
@ 2023-09-08 20:38 ` Marton Balint
2023-09-09 6:37 ` Tomas Härdin
2023-09-10 10:07 ` [FFmpeg-devel] [PATCH v2] " Andreas Rheinhardt
2023-09-09 6:36 ` [FFmpeg-devel] [PATCH 01/21] " Tomas Härdin
3 siblings, 2 replies; 19+ messages in thread
From: Marton Balint @ 2023-09-08 20:38 UTC (permalink / raw)
To: FFmpeg development discussions and patches
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".
>
_______________________________________________
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".
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [FFmpeg-devel] [PATCH 01/21] avformat/avio: Don't use incompatible function pointer type for call
2023-09-07 0:23 [FFmpeg-devel] [PATCH 01/21] avformat/avio: Don't use incompatible function pointer type for call Andreas Rheinhardt
` (2 preceding siblings ...)
2023-09-08 20:38 ` [FFmpeg-devel] [PATCH 01/21] avformat/avio: " Marton Balint
@ 2023-09-09 6:36 ` Tomas Härdin
2023-09-10 10:18 ` Andreas Rheinhardt
3 siblings, 1 reply; 19+ messages in thread
From: Tomas Härdin @ 2023-09-09 6:36 UTC (permalink / raw)
To: FFmpeg development discussions and patches
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
_______________________________________________
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".
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [FFmpeg-devel] [PATCH 01/21] avformat/avio: Don't use incompatible function pointer type for call
2023-09-08 20:38 ` [FFmpeg-devel] [PATCH 01/21] avformat/avio: " Marton Balint
@ 2023-09-09 6:37 ` Tomas Härdin
2023-09-10 8:47 ` Marton Balint
2023-09-10 10:07 ` [FFmpeg-devel] [PATCH v2] " Andreas Rheinhardt
1 sibling, 1 reply; 19+ messages in thread
From: Tomas Härdin @ 2023-09-09 6:37 UTC (permalink / raw)
To: FFmpeg development discussions and patches
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
_______________________________________________
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".
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [FFmpeg-devel] [PATCH 02/21] avformat/internal: Avoid casting const away
2023-09-07 0:32 ` [FFmpeg-devel] [PATCH 02/21] avformat/internal: Avoid casting const away Andreas Rheinhardt
@ 2023-09-09 6:38 ` Tomas Härdin
0 siblings, 0 replies; 19+ messages in thread
From: Tomas Härdin @ 2023-09-09 6:38 UTC (permalink / raw)
To: FFmpeg development discussions and patches
tor 2023-09-07 klockan 02:32 +0200 skrev Andreas Rheinhardt:
> Fixes many warnings when using -Wcast-qual.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> libavformat/internal.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Looks fine
/Tomas
_______________________________________________
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".
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [FFmpeg-devel] [PATCH 03/21] avformat/aviobuf: Don't use incompatible function pointer type for call
2023-09-07 0:32 ` [FFmpeg-devel] [PATCH 03/21] avformat/aviobuf: Don't use incompatible function pointer type for call Andreas Rheinhardt
@ 2023-09-09 6:46 ` Tomas Härdin
2023-09-09 9:25 ` Andreas Rheinhardt
0 siblings, 1 reply; 19+ messages in thread
From: Tomas Härdin @ 2023-09-09 6:46 UTC (permalink / raw)
To: FFmpeg development discussions and patches
tor 2023-09-07 klockan 02:32 +0200 skrev Andreas Rheinhardt:
> It is undefined behaviour even in cases where it works
> (it works because both are pointers). Instead change
> the functions involved to use the type expected by the AVIO-API
> and add inline wrappers for our internal callers.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> libavformat/avio.c | 15 +++++++++++----
> libavformat/aviobuf.c | 6 ++----
> libavformat/url.h | 20 ++++++++++++++++----
> 3 files changed, 29 insertions(+), 12 deletions(-)
Should be OK. No version bump necessary since it's an internal API,
right? And only within lavf as well
/Tomas
_______________________________________________
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".
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [FFmpeg-devel] [PATCH 03/21] avformat/aviobuf: Don't use incompatible function pointer type for call
2023-09-09 6:46 ` Tomas Härdin
@ 2023-09-09 9:25 ` Andreas Rheinhardt
0 siblings, 0 replies; 19+ messages in thread
From: Andreas Rheinhardt @ 2023-09-09 9:25 UTC (permalink / raw)
To: ffmpeg-devel
Tomas Härdin:
> tor 2023-09-07 klockan 02:32 +0200 skrev Andreas Rheinhardt:
>> It is undefined behaviour even in cases where it works
>> (it works because both are pointers). Instead change
>> the functions involved to use the type expected by the AVIO-API
>> and add inline wrappers for our internal callers.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>> libavformat/avio.c | 15 +++++++++++----
>> libavformat/aviobuf.c | 6 ++----
>> libavformat/url.h | 20 ++++++++++++++++----
>> 3 files changed, 29 insertions(+), 12 deletions(-)
>
> Should be OK. No version bump necessary since it's an internal API,
> right? And only within lavf as well
>
That is correct; url.h is not even a public header.
- 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".
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [FFmpeg-devel] [PATCH 01/21] avformat/avio: Don't use incompatible function pointer type for call
2023-09-09 6:37 ` Tomas Härdin
@ 2023-09-10 8:47 ` Marton Balint
2023-09-10 9:02 ` Andreas Rheinhardt
0 siblings, 1 reply; 19+ messages in thread
From: Marton Balint @ 2023-09-10 8:47 UTC (permalink / raw)
To: FFmpeg development discussions and patches
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
_______________________________________________
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".
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [FFmpeg-devel] [PATCH 01/21] avformat/avio: Don't use incompatible function pointer type for call
2023-09-10 8:47 ` Marton Balint
@ 2023-09-10 9:02 ` Andreas Rheinhardt
2023-09-10 18:07 ` Marton Balint
0 siblings, 1 reply; 19+ messages in thread
From: Andreas Rheinhardt @ 2023-09-10 9:02 UTC (permalink / raw)
To: ffmpeg-devel
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
_______________________________________________
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".
^ permalink raw reply [flat|nested] 19+ messages in thread
* [FFmpeg-devel] [PATCH v2] avformat/avio: Don't use incompatible function pointer type for call
2023-09-08 20:38 ` [FFmpeg-devel] [PATCH 01/21] avformat/avio: " Marton Balint
2023-09-09 6:37 ` Tomas Härdin
@ 2023-09-10 10:07 ` Andreas Rheinhardt
2023-09-11 16:53 ` Tomas Härdin
1 sibling, 1 reply; 19+ messages in thread
From: Andreas Rheinhardt @ 2023-09-10 10:07 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: 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).
Instead use a macro to produce two functions with the required
types to be const-correct and type-safe.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
libavformat/avio.c | 98 +++++++++++++++++++++++-----------------------
1 file changed, 50 insertions(+), 48 deletions(-)
diff --git a/libavformat/avio.c b/libavformat/avio.c
index ab1c19a58d..9783cb1881 100644
--- a/libavformat/avio.c
+++ b/libavformat/avio.c
@@ -353,63 +353,67 @@ fail:
return ret;
}
-static inline int retry_transfer_wrapper(URLContext *h, uint8_t *buf,
- int size, int size_min,
- int (*transfer_func)(URLContext *h,
- uint8_t *buf,
- int size))
-{
- int ret, len;
- int fast_retries = 5;
- int64_t wait_since = 0;
-
- len = 0;
- while (len < size_min) {
- if (ff_check_interrupt(&h->interrupt_callback))
- return AVERROR_EXIT;
- ret = transfer_func(h, buf + len, size - len);
- if (ret == AVERROR(EINTR))
- continue;
- if (h->flags & AVIO_FLAG_NONBLOCK)
- return ret;
- if (ret == AVERROR(EAGAIN)) {
- ret = 0;
- if (fast_retries) {
- fast_retries--;
- } else {
- if (h->rw_timeout) {
- if (!wait_since)
- wait_since = av_gettime_relative();
- else if (av_gettime_relative() > wait_since + h->rw_timeout)
- return AVERROR(EIO);
- }
- av_usleep(1000);
- }
- } else if (ret == AVERROR_EOF)
- return (len > 0) ? len : AVERROR_EOF;
- else if (ret < 0)
- return ret;
- if (ret) {
- fast_retries = FFMAX(fast_retries, 2);
- wait_since = 0;
- }
- len += ret;
- }
- return len;
+#define RETRY_TRANSFER_WRAPPER(RW, CONST) \
+static inline int retry_ ## RW ## _wrapper(URLContext *h, CONST uint8_t *buf, \
+ int size, int size_min, \
+ int (*RW ## _func)(URLContext *h, \
+ CONST uint8_t *buf, \
+ int size)) \
+{ \
+ int ret, len; \
+ int fast_retries = 5; \
+ int64_t wait_since = 0; \
+ \
+ len = 0; \
+ while (len < size_min) { \
+ if (ff_check_interrupt(&h->interrupt_callback)) \
+ return AVERROR_EXIT; \
+ ret = RW ## _func(h, buf + len, size - len); \
+ if (ret == AVERROR(EINTR)) \
+ continue; \
+ if (h->flags & AVIO_FLAG_NONBLOCK) \
+ return ret; \
+ if (ret == AVERROR(EAGAIN)) { \
+ ret = 0; \
+ if (fast_retries) { \
+ fast_retries--; \
+ } else { \
+ if (h->rw_timeout) { \
+ if (!wait_since) \
+ wait_since = av_gettime_relative(); \
+ else if (av_gettime_relative() > wait_since + h->rw_timeout) \
+ return AVERROR(EIO); \
+ } \
+ av_usleep(1000); \
+ } \
+ } else if (ret == AVERROR_EOF) \
+ return (len > 0) ? len : AVERROR_EOF; \
+ else if (ret < 0) \
+ return ret; \
+ if (ret) { \
+ fast_retries = FFMAX(fast_retries, 2); \
+ wait_since = 0; \
+ } \
+ len += ret; \
+ } \
+ return len; \
}
+RETRY_TRANSFER_WRAPPER(read, )
+RETRY_TRANSFER_WRAPPER(write, const)
+
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_read_wrapper(h, buf, size, 1, h->prot->url_read);
}
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_read_wrapper(h, buf, size, size, h->prot->url_read);
}
int ffurl_write(URLContext *h, const unsigned char *buf, int size)
@@ -420,9 +424,7 @@ 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_write_wrapper(h, buf, size, size, h->prot->url_write);
}
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".
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [FFmpeg-devel] [PATCH 01/21] avformat/avio: Don't use incompatible function pointer type for call
2023-09-09 6:36 ` [FFmpeg-devel] [PATCH 01/21] " Tomas Härdin
@ 2023-09-10 10:18 ` Andreas Rheinhardt
0 siblings, 0 replies; 19+ messages in thread
From: Andreas Rheinhardt @ 2023-09-10 10:18 UTC (permalink / raw)
To: ffmpeg-devel
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
_______________________________________________
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".
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [FFmpeg-devel] [PATCH 01/21] avformat/avio: Don't use incompatible function pointer type for call
2023-09-10 9:02 ` Andreas Rheinhardt
@ 2023-09-10 18:07 ` Marton Balint
2023-09-10 18:23 ` Andreas Rheinhardt
2023-09-11 17:27 ` [FFmpeg-devel] [PATCH v3] " Andreas Rheinhardt
0 siblings, 2 replies; 19+ messages in thread
From: Marton Balint @ 2023-09-10 18:07 UTC (permalink / raw)
To: FFmpeg development discussions and patches
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
_______________________________________________
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".
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [FFmpeg-devel] [PATCH 01/21] avformat/avio: Don't use incompatible function pointer type for call
2023-09-10 18:07 ` Marton Balint
@ 2023-09-10 18:23 ` Andreas Rheinhardt
2023-09-11 17:27 ` [FFmpeg-devel] [PATCH v3] " Andreas Rheinhardt
1 sibling, 0 replies; 19+ messages in thread
From: Andreas Rheinhardt @ 2023-09-10 18:23 UTC (permalink / raw)
To: ffmpeg-devel
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".
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [FFmpeg-devel] [PATCH v2] avformat/avio: Don't use incompatible function pointer type for call
2023-09-10 10:07 ` [FFmpeg-devel] [PATCH v2] " Andreas Rheinhardt
@ 2023-09-11 16:53 ` Tomas Härdin
0 siblings, 0 replies; 19+ messages in thread
From: Tomas Härdin @ 2023-09-11 16:53 UTC (permalink / raw)
To: FFmpeg development discussions and patches
sön 2023-09-10 klockan 12:07 +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).
> Instead use a macro to produce two functions with the required
> types to be const-correct and type-safe.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> libavformat/avio.c | 98 +++++++++++++++++++++++---------------------
> --
> 1 file changed, 50 insertions(+), 48 deletions(-)
>
> diff --git a/libavformat/avio.c b/libavformat/avio.c
> index ab1c19a58d..9783cb1881 100644
> --- a/libavformat/avio.c
> +++ b/libavformat/avio.c
> @@ -353,63 +353,67 @@ fail:
> return ret;
> }
>
> -static inline int retry_transfer_wrapper(URLContext *h, uint8_t
> *buf,
> - int size, int size_min,
> - int
> (*transfer_func)(URLContext *h,
> -
> uint8_t *buf,
> - int
> size))
> -{
> - int ret, len;
> - int fast_retries = 5;
> - int64_t wait_since = 0;
> -
> - len = 0;
> - while (len < size_min) {
> - if (ff_check_interrupt(&h->interrupt_callback))
> - return AVERROR_EXIT;
> - ret = transfer_func(h, buf + len, size - len);
> - if (ret == AVERROR(EINTR))
> - continue;
> - if (h->flags & AVIO_FLAG_NONBLOCK)
> - return ret;
> - if (ret == AVERROR(EAGAIN)) {
> - ret = 0;
> - if (fast_retries) {
> - fast_retries--;
> - } else {
> - if (h->rw_timeout) {
> - if (!wait_since)
> - wait_since = av_gettime_relative();
> - else if (av_gettime_relative() > wait_since + h-
> >rw_timeout)
> - return AVERROR(EIO);
> - }
> - av_usleep(1000);
> - }
> - } else if (ret == AVERROR_EOF)
> - return (len > 0) ? len : AVERROR_EOF;
> - else if (ret < 0)
> - return ret;
> - if (ret) {
> - fast_retries = FFMAX(fast_retries, 2);
> - wait_since = 0;
> - }
> - len += ret;
> - }
> - return len;
> +#define RETRY_TRANSFER_WRAPPER(RW,
> CONST) \
> +static inline int retry_ ## RW ## _wrapper(URLContext *h, CONST
> uint8_t *buf, \
> + int size, int
> size_min, \
> + int (*RW ##
> _func)(URLContext *h, \
> + CONST
> uint8_t *buf, \
> + int
> size)) \
> +{
> \
> + int ret,
> len; \
> + int fast_retries =
> 5; \
> + int64_t wait_since =
> 0; \
> +
> \
> + len =
> 0; \
> + while (len < size_min)
> { \
> + if (ff_check_interrupt(&h-
> >interrupt_callback)) \
> + return
> AVERROR_EXIT; \
> + ret = RW ## _func(h, buf + len, size -
> len); \
> + if (ret ==
> AVERROR(EINTR)) \
> +
> continue; \
> + if (h->flags &
> AVIO_FLAG_NONBLOCK) \
> + return
> ret; \
> + if (ret == AVERROR(EAGAIN))
> { \
> + ret =
> 0; \
> + if (fast_retries)
> { \
> + fast_retries--
> ; \
> + } else
> { \
> + if (h->rw_timeout)
> { \
> + if
> (!wait_since) \
> + wait_since =
> av_gettime_relative(); \
> + else if (av_gettime_relative() > wait_since + h-
> >rw_timeout) \
> + return
> AVERROR(EIO); \
> +
> } \
> +
> av_usleep(1000); \
> +
> } \
> + } else if (ret ==
> AVERROR_EOF) \
> + return (len > 0) ? len :
> AVERROR_EOF; \
> + else if (ret <
> 0) \
> + return
> ret; \
> + if (ret)
> { \
> + fast_retries = FFMAX(fast_retries,
> 2); \
> + wait_since =
> 0; \
> +
> }
> \
> + len +=
> ret; \
> +
> }
> \
> + return
> len; \
> }
>
> +RETRY_TRANSFER_WRAPPER(read, )
> +RETRY_TRANSFER_WRAPPER(write, const)
> +
> 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_read_wrapper(h, buf, size, 1, h->prot->url_read);
> }
>
> 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_read_wrapper(h, buf, size, size, h->prot-
> >url_read);
> }
>
> int ffurl_write(URLContext *h, const unsigned char *buf, int size)
> @@ -420,9 +424,7 @@ 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_write_wrapper(h, buf, size, size, h->prot-
> >url_write);
> }
>
> int64_t ffurl_seek(URLContext *h, int64_t pos, int whence)
Fair enough.
/Tomas
_______________________________________________
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".
^ permalink raw reply [flat|nested] 19+ messages in thread
* [FFmpeg-devel] [PATCH v3] avformat/avio: Don't use incompatible function pointer type for call
2023-09-10 18:07 ` Marton Balint
2023-09-10 18:23 ` Andreas Rheinhardt
@ 2023-09-11 17:27 ` Andreas Rheinhardt
2023-09-11 18:43 ` Marton Balint
2023-09-12 14:59 ` Andreas Rheinhardt
1 sibling, 2 replies; 19+ messages in thread
From: Andreas Rheinhardt @ 2023-09-11 17:27 UTC (permalink / raw)
To: ffmpeg-devel; +Cc: 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).
Instead add a cbuf parameter to pass a const buffer (for writing)
as well as a parameter indicating whether we are reading or writing;
retry_transfer_wrapper() itself then uses the correct function
based upon this information.
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
Another approach; IMO the best of the three. What do you think?
libavformat/avio.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/libavformat/avio.c b/libavformat/avio.c
index b6a192892a..b793a7546c 100644
--- a/libavformat/avio.c
+++ b/libavformat/avio.c
@@ -348,10 +348,9 @@ 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)
{
int ret, len;
int fast_retries = 5;
@@ -361,7 +360,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 ? h->prot->url_read (h, buf + len, size - len):
+ h->prot->url_write(h, cbuf + len, size - len);
if (ret == AVERROR(EINTR))
continue;
if (h->flags & AVIO_FLAG_NONBLOCK)
@@ -398,14 +398,14 @@ int ffurl_read2(void *urlcontext, uint8_t *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, 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, 1);
}
#if FF_API_AVIO_WRITE_NONCONST
@@ -422,9 +422,7 @@ int ffurl_write2(void *urlcontext, const uint8_t *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, 0);
}
int64_t ffurl_seek2(void *urlcontext, 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".
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [FFmpeg-devel] [PATCH v3] avformat/avio: Don't use incompatible function pointer type for call
2023-09-11 17:27 ` [FFmpeg-devel] [PATCH v3] " Andreas Rheinhardt
@ 2023-09-11 18:43 ` Marton Balint
2023-09-12 14:59 ` Andreas Rheinhardt
1 sibling, 0 replies; 19+ messages in thread
From: Marton Balint @ 2023-09-11 18:43 UTC (permalink / raw)
To: FFmpeg development discussions and patches
On Mon, 11 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).
>
> Instead add a cbuf parameter to pass a const buffer (for writing)
> as well as a parameter indicating whether we are reading or writing;
> retry_transfer_wrapper() itself then uses the correct function
> based upon this information.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> Another approach; IMO the best of the three. What do you think?
OK for me, thanks.
Marton
>
> libavformat/avio.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/libavformat/avio.c b/libavformat/avio.c
> index b6a192892a..b793a7546c 100644
> --- a/libavformat/avio.c
> +++ b/libavformat/avio.c
> @@ -348,10 +348,9 @@ 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)
> {
> int ret, len;
> int fast_retries = 5;
> @@ -361,7 +360,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 ? h->prot->url_read (h, buf + len, size - len):
> + h->prot->url_write(h, cbuf + len, size - len);
> if (ret == AVERROR(EINTR))
> continue;
> if (h->flags & AVIO_FLAG_NONBLOCK)
> @@ -398,14 +398,14 @@ int ffurl_read2(void *urlcontext, uint8_t *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, 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, 1);
> }
>
> #if FF_API_AVIO_WRITE_NONCONST
> @@ -422,9 +422,7 @@ int ffurl_write2(void *urlcontext, const uint8_t *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, 0);
> }
>
> int64_t ffurl_seek2(void *urlcontext, 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".
>
_______________________________________________
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".
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [FFmpeg-devel] [PATCH v3] avformat/avio: Don't use incompatible function pointer type for call
2023-09-11 17:27 ` [FFmpeg-devel] [PATCH v3] " Andreas Rheinhardt
2023-09-11 18:43 ` Marton Balint
@ 2023-09-12 14:59 ` Andreas Rheinhardt
1 sibling, 0 replies; 19+ messages in thread
From: Andreas Rheinhardt @ 2023-09-12 14:59 UTC (permalink / raw)
To: ffmpeg-devel
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).
>
> Instead add a cbuf parameter to pass a const buffer (for writing)
> as well as a parameter indicating whether we are reading or writing;
> retry_transfer_wrapper() itself then uses the correct function
> based upon this information.
>
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> Another approach; IMO the best of the three. What do you think?
>
> libavformat/avio.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/libavformat/avio.c b/libavformat/avio.c
> index b6a192892a..b793a7546c 100644
> --- a/libavformat/avio.c
> +++ b/libavformat/avio.c
> @@ -348,10 +348,9 @@ 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)
> {
> int ret, len;
> int fast_retries = 5;
> @@ -361,7 +360,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 ? h->prot->url_read (h, buf + len, size - len):
> + h->prot->url_write(h, cbuf + len, size - len);
> if (ret == AVERROR(EINTR))
> continue;
> if (h->flags & AVIO_FLAG_NONBLOCK)
> @@ -398,14 +398,14 @@ int ffurl_read2(void *urlcontext, uint8_t *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, 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, 1);
> }
>
> #if FF_API_AVIO_WRITE_NONCONST
> @@ -422,9 +422,7 @@ int ffurl_write2(void *urlcontext, const uint8_t *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, 0);
> }
>
> int64_t ffurl_seek2(void *urlcontext, int64_t pos, int whence)
Will apply this tonight unless there are objections.
- 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".
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2023-09-12 14:58 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-07 0:23 [FFmpeg-devel] [PATCH 01/21] avformat/avio: Don't use incompatible function pointer type for call Andreas Rheinhardt
2023-09-07 0:32 ` [FFmpeg-devel] [PATCH 02/21] avformat/internal: Avoid casting const away Andreas Rheinhardt
2023-09-09 6:38 ` Tomas Härdin
2023-09-07 0:32 ` [FFmpeg-devel] [PATCH 03/21] avformat/aviobuf: Don't use incompatible function pointer type for call Andreas Rheinhardt
2023-09-09 6:46 ` Tomas Härdin
2023-09-09 9:25 ` Andreas Rheinhardt
2023-09-08 20:38 ` [FFmpeg-devel] [PATCH 01/21] avformat/avio: " Marton Balint
2023-09-09 6:37 ` Tomas Härdin
2023-09-10 8:47 ` Marton Balint
2023-09-10 9:02 ` Andreas Rheinhardt
2023-09-10 18:07 ` Marton Balint
2023-09-10 18:23 ` Andreas Rheinhardt
2023-09-11 17:27 ` [FFmpeg-devel] [PATCH v3] " Andreas Rheinhardt
2023-09-11 18:43 ` Marton Balint
2023-09-12 14:59 ` Andreas Rheinhardt
2023-09-10 10:07 ` [FFmpeg-devel] [PATCH v2] " Andreas Rheinhardt
2023-09-11 16:53 ` Tomas Härdin
2023-09-09 6:36 ` [FFmpeg-devel] [PATCH 01/21] " Tomas Härdin
2023-09-10 10:18 ` Andreas Rheinhardt
Git Inbox Mirror of the ffmpeg-devel mailing list - see https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
This inbox may be cloned and mirrored by anyone:
git clone --mirror https://master.gitmailbox.com/ffmpegdev/0 ffmpegdev/git/0.git
# If you have public-inbox 1.1+ installed, you may
# initialize and index your mirror using the following commands:
public-inbox-init -V2 ffmpegdev ffmpegdev/ https://master.gitmailbox.com/ffmpegdev \
ffmpegdev@gitmailbox.com
public-inbox-index ffmpegdev
Example config snippet for mirrors.
AGPL code for this site: git clone https://public-inbox.org/public-inbox.git