| Message ID | 20171025082258.32530-2-george@nsup.org |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Oct 25, 2017 at 11:22 AM, Nicolas George <george@nsup.org> wrote: > Print a warning to let applicatios fix their use. > After a deprecation period, check with a low-level assert. > Also make the constraint explicit in the doxygen comment. > > Signed-off-by: Nicolas George <george@nsup.org> Thanks for the patch. > --- > libavformat/avio.h | 2 ++ > libavformat/aviobuf.c | 30 +++++++++++++++++++++--------- > libavformat/version.h | 3 +++ > 3 files changed, 26 insertions(+), 9 deletions(-) > > diff --git a/libavformat/avio.h b/libavformat/avio.h > index 19ecd96eb7..76ff7cd81e 100644 > --- a/libavformat/avio.h > +++ b/libavformat/avio.h > @@ -452,6 +452,8 @@ void avio_free_directory_entry(AVIODirEntry **entry); > * @param write_flag Set to 1 if the buffer should be writable, 0 otherwise. > * @param opaque An opaque pointer to user-specific data. > * @param read_packet A function for refilling the buffer, may be NULL. > + * For stream protocols, must never return 0 but rather > + * a proper AVERROR code. Otherwise OK, but since this is the first mention of "stream protocol" in the repo and el goog gives me just "Internet Stream Protocol" so I would like an explanation of what this means and how it connects to "custom AVIO implementations/wrappers"? > * @param write_packet A function for writing the buffer contents, may be NULL. > * The function may not change the input buffers content. > * @param seek A function for seeking to specified byte position, may be NULL. > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c > index 3e9d774a13..bb5bcf7a14 100644 > --- a/libavformat/aviobuf.c > +++ b/libavformat/aviobuf.c > @@ -524,6 +524,24 @@ void avio_write_marker(AVIOContext *s, int64_t time, enum AVIODataMarkerType typ > s->last_time = time; > } > > +static int read_packet_wrapper(AVIOContext *s, uint8_t *buf, int size) > +{ > + int ret; > + > + if (!s->read_packet) > + return AVERROR_EOF; > + ret = s->read_packet(s->opaque, buf, size); > +#if FF_API_OLD_AVIO_EOF_0 > + if (!ret && !s->max_packet_size) { Can you explain how you can utilize max_packet_size as a note regarding if zero is EOF or not? Is it just a variable that happens to be set to zero with custom AVIO contexts, or is there some other logic behind this? > + av_log(s, AV_LOG_WARNING, "Invalid return value 0 for stream protocol\n"); > + ret = AVERROR_EOF; > + } > +#else > + av_assert2(ret || s->max_packet_size); > +#endif > + return ret; > +} > + > /* Input stream */ > > static void fill_buffer(AVIOContext *s) > @@ -562,10 +580,7 @@ static void fill_buffer(AVIOContext *s) > len = s->orig_buffer_size; > } > > - if (s->read_packet) > - len = s->read_packet(s->opaque, dst, len); > - else > - len = AVERROR_EOF; > + len = read_packet_wrapper(s, dst, len); > if (len == AVERROR_EOF) { > /* do not modify buffer if EOF reached so that a seek back can > be done without rereading data */ > @@ -638,10 +653,7 @@ int avio_read(AVIOContext *s, unsigned char *buf, int size) > if (len == 0 || s->write_flag) { > if((s->direct || size > s->buffer_size) && !s->update_checksum) { > // bypass the buffer and read data directly into buf > - if(s->read_packet) > - len = s->read_packet(s->opaque, buf, size); > - else > - len = AVERROR_EOF; > + len = read_packet_wrapper(s, buf, size); > if (len == AVERROR_EOF) { > /* do not modify buffer if EOF reached so that a seek back can > be done without rereading data */ > @@ -708,7 +720,7 @@ int avio_read_partial(AVIOContext *s, unsigned char *buf, int size) > return -1; > > if (s->read_packet && s->write_flag) { > - len = s->read_packet(s->opaque, buf, size); > + len = read_packet_wrapper(s, buf, size); > if (len > 0) > s->pos += len; > return len; > diff --git a/libavformat/version.h b/libavformat/version.h > index 0feb788c36..358ab91ab2 100644 > --- a/libavformat/version.h > +++ b/libavformat/version.h > @@ -79,6 +79,9 @@ > #ifndef FF_API_OLD_ROTATE_API > #define FF_API_OLD_ROTATE_API (LIBAVFORMAT_VERSION_MAJOR < 59) > #endif > +#ifndef FF_API_OLD_AVIO_EOF_0 > +#define FF_API_OLD_AVIO_EOF_0 (LIBAVFORMAT_VERSION_MAJOR < 59) > +#endif > > > #ifndef FF_API_R_FRAME_RATE > -- > 2.14.2 Otherwise LGTM. Will test in a moment. Jan
On Wed, Oct 25, 2017 at 11:22 AM, Nicolas George <george@nsup.org> wrote: > +static int read_packet_wrapper(AVIOContext *s, uint8_t *buf, int size) > +{ > + int ret; > + > + if (!s->read_packet) > + return AVERROR_EOF; > + ret = s->read_packet(s->opaque, buf, size); > +#if FF_API_OLD_AVIO_EOF_0 > + if (!ret && !s->max_packet_size) { > + av_log(s, AV_LOG_WARNING, "Invalid return value 0 for stream protocol\n"); > + ret = AVERROR_EOF; > + } > +#else > + av_assert2(ret || s->max_packet_size); > +#endif > + return ret; > +} > + Built and tested locally and the effect seems to be the one wished, although it seems like it is complaining about the AVClass being nullptr? Is this something inherent to custom AVIO contexts and it's the client that's supposed to fix this? Log follows: > [ffmpeg] av_log callback called with bad parameters (NULL AVClass). > [ffmpeg] This is a bug in one of Libav/FFmpeg libraries used. > [ffmpeg] Invalid return value 0 for stream protocol Jan
Le sextidi 6 brumaire, an CCXXVI, Jan Ekstrom a écrit : > Built and tested locally and the effect seems to be the one wished, > although it seems like it is complaining about the AVClass being > nullptr? Is this something inherent to custom AVIO contexts and it's > the client that's supposed to fix this? Log follows: > > > [ffmpeg] av_log callback called with bad parameters (NULL AVClass). > > [ffmpeg] This is a bug in one of Libav/FFmpeg libraries used. > > [ffmpeg] Invalid return value 0 for stream protocol Thanks. It seems custom AVIOContext are not real AVIOContext in the full meaning, and there are glitches in fixing that. I have locally changed the log line: av_log(NULL, AV_LOG_WARNING, "Invalid ^^^^ Not completely satisfactory, but enough for a library-to-application warning. Shall I send the whole series again? Regards,
Le sextidi 6 brumaire, an CCXXVI, Jan Ekstrom a écrit : > Otherwise OK, but since this is the first mention of "stream protocol" > in the repo and el goog gives me just "Internet Stream Protocol" so I > would like an explanation of what this means and how it connects to > "custom AVIO implementations/wrappers"? It is standard parlance in networking: stream protocols produce a stream of octets, without any additional structure, while packet protocols produce packets, which are delimited at protocol level and visible by the application, and can be empty. FFmpeg distinguishes packet protocols with the max_packet_size: if it is 0, it is a stream protocol, if not a packet protocol. Regards,
On Fri, Oct 27, 2017 at 9:33 PM, Nicolas George <george@nsup.org> wrote: > It is standard parlance in networking: stream protocols produce a stream > of octets, without any additional structure, while packet protocols > produce packets, which are delimited at protocol level and visible by > the application, and can be empty. > Gotcha. Seems like it's called "/* default: stream file */" in the current code base, which is why I couldn't find references to stream protocols. > FFmpeg distinguishes packet protocols with the max_packet_size: if it is > 0, it is a stream protocol, if not a packet protocol. Gotcha^2. Please send the patch-set v2 with the related fixes (the av_log in 2/3 and the correct ret in 3/3) as with that I think this all looks good to go :) . Jan
diff --git a/libavformat/avio.h b/libavformat/avio.h index 19ecd96eb7..76ff7cd81e 100644 --- a/libavformat/avio.h +++ b/libavformat/avio.h @@ -452,6 +452,8 @@ void avio_free_directory_entry(AVIODirEntry **entry); * @param write_flag Set to 1 if the buffer should be writable, 0 otherwise. * @param opaque An opaque pointer to user-specific data. * @param read_packet A function for refilling the buffer, may be NULL. + * For stream protocols, must never return 0 but rather + * a proper AVERROR code. * @param write_packet A function for writing the buffer contents, may be NULL. * The function may not change the input buffers content. * @param seek A function for seeking to specified byte position, may be NULL. diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c index 3e9d774a13..bb5bcf7a14 100644 --- a/libavformat/aviobuf.c +++ b/libavformat/aviobuf.c @@ -524,6 +524,24 @@ void avio_write_marker(AVIOContext *s, int64_t time, enum AVIODataMarkerType typ s->last_time = time; } +static int read_packet_wrapper(AVIOContext *s, uint8_t *buf, int size) +{ + int ret; + + if (!s->read_packet) + return AVERROR_EOF; + ret = s->read_packet(s->opaque, buf, size); +#if FF_API_OLD_AVIO_EOF_0 + if (!ret && !s->max_packet_size) { + av_log(s, AV_LOG_WARNING, "Invalid return value 0 for stream protocol\n"); + ret = AVERROR_EOF; + } +#else + av_assert2(ret || s->max_packet_size); +#endif + return ret; +} + /* Input stream */ static void fill_buffer(AVIOContext *s) @@ -562,10 +580,7 @@ static void fill_buffer(AVIOContext *s) len = s->orig_buffer_size; } - if (s->read_packet) - len = s->read_packet(s->opaque, dst, len); - else - len = AVERROR_EOF; + len = read_packet_wrapper(s, dst, len); if (len == AVERROR_EOF) { /* do not modify buffer if EOF reached so that a seek back can be done without rereading data */ @@ -638,10 +653,7 @@ int avio_read(AVIOContext *s, unsigned char *buf, int size) if (len == 0 || s->write_flag) { if((s->direct || size > s->buffer_size) && !s->update_checksum) { // bypass the buffer and read data directly into buf - if(s->read_packet) - len = s->read_packet(s->opaque, buf, size); - else - len = AVERROR_EOF; + len = read_packet_wrapper(s, buf, size); if (len == AVERROR_EOF) { /* do not modify buffer if EOF reached so that a seek back can be done without rereading data */ @@ -708,7 +720,7 @@ int avio_read_partial(AVIOContext *s, unsigned char *buf, int size) return -1; if (s->read_packet && s->write_flag) { - len = s->read_packet(s->opaque, buf, size); + len = read_packet_wrapper(s, buf, size); if (len > 0) s->pos += len; return len; diff --git a/libavformat/version.h b/libavformat/version.h index 0feb788c36..358ab91ab2 100644 --- a/libavformat/version.h +++ b/libavformat/version.h @@ -79,6 +79,9 @@ #ifndef FF_API_OLD_ROTATE_API #define FF_API_OLD_ROTATE_API (LIBAVFORMAT_VERSION_MAJOR < 59) #endif +#ifndef FF_API_OLD_AVIO_EOF_0 +#define FF_API_OLD_AVIO_EOF_0 (LIBAVFORMAT_VERSION_MAJOR < 59) +#endif #ifndef FF_API_R_FRAME_RATE
Print a warning to let applicatios fix their use. After a deprecation period, check with a low-level assert. Also make the constraint explicit in the doxygen comment. Signed-off-by: Nicolas George <george@nsup.org> --- libavformat/avio.h | 2 ++ libavformat/aviobuf.c | 30 +++++++++++++++++++++--------- libavformat/version.h | 3 +++ 3 files changed, 26 insertions(+), 9 deletions(-)