| Message ID | 20240324022504.1419451-2-marth64@proxyid.net |
|---|---|
| State | New |
| Headers | show |
| Series | RCWT Closed Captions demuxer (v9) | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
On date Saturday 2024-03-23 21:25:00 -0500, Marth64 wrote: > If ff_subtitles_queue_insert() were given a NULL buffer > with 0 length, it would still attempt to grow the packet > or memcpy depending on if merge option is enabled. > > In this commit, allow passing a NULL buffer with 0 length > without attempting to do such operations. This way, if a > subtitle demuxer happens to pass an empty cue or wants to > use av_get_packet() to read bytes, there are no unnecessary > operations on the packet after it is allocated. > > Signed-off-by: Marth64 <marth64@proxyid.net> > --- > libavformat/subtitles.c | 24 ++++++++++++++++-------- > libavformat/subtitles.h | 2 +- > 2 files changed, 17 insertions(+), 9 deletions(-) > > diff --git a/libavformat/subtitles.c b/libavformat/subtitles.c > index 3413763c7b..4742ca0667 100644 > --- a/libavformat/subtitles.c > +++ b/libavformat/subtitles.c > @@ -21,6 +21,7 @@ > #include "avformat.h" > #include "subtitles.h" > #include "avio_internal.h" > +#include "libavutil/avassert.h" > #include "libavutil/avstring.h" > > void ff_text_init_avio(void *s, FFTextReader *r, AVIOContext *pb) > @@ -111,15 +112,20 @@ AVPacket *ff_subtitles_queue_insert(FFDemuxSubtitlesQueue *q, > { > AVPacket **subs, *sub; > > + if (!event) > + av_assert1(len == 0); > + > if (merge && q->nb_subs > 0) { > /* merge with previous event */ > > int old_len; > sub = q->subs[q->nb_subs - 1]; > old_len = sub->size; > - if (av_grow_packet(sub, len) < 0) > - return NULL; > - memcpy(sub->data + old_len, event, len); > + if (event) { > + if (av_grow_packet(sub, len) < 0) > + return NULL; > + memcpy(sub->data + old_len, event, len); > + } > } else { > /* new event */ > > @@ -133,14 +139,16 @@ AVPacket *ff_subtitles_queue_insert(FFDemuxSubtitlesQueue *q, > sub = av_packet_alloc(); > if (!sub) > return NULL; > - if (av_new_packet(sub, len) < 0) { > - av_packet_free(&sub); > - return NULL; > + if (event) { > + if (av_new_packet(sub, len) < 0) { > + av_packet_free(&sub); > + return NULL; > + } > + memcpy(sub->data, event, len); > } > - subs[q->nb_subs++] = sub; > sub->flags |= AV_PKT_FLAG_KEY; > sub->pts = sub->dts = 0; > - memcpy(sub->data, event, len); > + subs[q->nb_subs++] = sub; is the move needed? (but not blocking) LGTM otherwise.
> is the move needed? (but not blocking)
It is not needed, but the code flows better I think in terms of
readability. I think it feels cleaner to say, "set up `sub` then add it to
the array" vs. "set up sub's data, add it to the array, then set up its
other fields". But, I am good either way and on stand by if you request me
to change it.
Thank you for the review.
Marth64: > If ff_subtitles_queue_insert() were given a NULL buffer > with 0 length, it would still attempt to grow the packet > or memcpy depending on if merge option is enabled. > > In this commit, allow passing a NULL buffer with 0 length > without attempting to do such operations. This way, if a > subtitle demuxer happens to pass an empty cue or wants to > use av_get_packet() to read bytes, there are no unnecessary > operations on the packet after it is allocated. > > Signed-off-by: Marth64 <marth64@proxyid.net> > --- > libavformat/subtitles.c | 24 ++++++++++++++++-------- > libavformat/subtitles.h | 2 +- > 2 files changed, 17 insertions(+), 9 deletions(-) > > diff --git a/libavformat/subtitles.c b/libavformat/subtitles.c > index 3413763c7b..4742ca0667 100644 > --- a/libavformat/subtitles.c > +++ b/libavformat/subtitles.c > @@ -21,6 +21,7 @@ > #include "avformat.h" > #include "subtitles.h" > #include "avio_internal.h" > +#include "libavutil/avassert.h" > #include "libavutil/avstring.h" > > void ff_text_init_avio(void *s, FFTextReader *r, AVIOContext *pb) > @@ -111,15 +112,20 @@ AVPacket *ff_subtitles_queue_insert(FFDemuxSubtitlesQueue *q, > { > AVPacket **subs, *sub; > > + if (!event) > + av_assert1(len == 0); av_assert1(event || len == 0) will make the assert message more meaningful and saves a line. > + > if (merge && q->nb_subs > 0) { > /* merge with previous event */ > > int old_len; > sub = q->subs[q->nb_subs - 1]; > old_len = sub->size; > - if (av_grow_packet(sub, len) < 0) > - return NULL; > - memcpy(sub->data + old_len, event, len); > + if (event) { > + if (av_grow_packet(sub, len) < 0) > + return NULL; > + memcpy(sub->data + old_len, event, len); > + } > } else { > /* new event */ > > @@ -133,14 +139,16 @@ AVPacket *ff_subtitles_queue_insert(FFDemuxSubtitlesQueue *q, > sub = av_packet_alloc(); > if (!sub) > return NULL; > - if (av_new_packet(sub, len) < 0) { > - av_packet_free(&sub); > - return NULL; > + if (event) { > + if (av_new_packet(sub, len) < 0) { > + av_packet_free(&sub); > + return NULL; > + } > + memcpy(sub->data, event, len); > } > - subs[q->nb_subs++] = sub; > sub->flags |= AV_PKT_FLAG_KEY; > sub->pts = sub->dts = 0; > - memcpy(sub->data, event, len); > + subs[q->nb_subs++] = sub; > } > return sub; > } > diff --git a/libavformat/subtitles.h b/libavformat/subtitles.h > index 88665663c5..ad6b96ca6a 100644 > --- a/libavformat/subtitles.h > +++ b/libavformat/subtitles.h > @@ -112,7 +112,7 @@ typedef struct { > /** > * Insert a new subtitle event. > * > - * @param event the subtitle line, may not be zero terminated > + * @param event the subtitle line (not zero terminated) or NULL on not yet available event > * @param len the length of the event (in strlen() sense, so without '\0') > * @param merge set to 1 if the current event should be concatenated with the > * previous one instead of adding a new entry, 0 otherwise
diff --git a/libavformat/subtitles.c b/libavformat/subtitles.c index 3413763c7b..4742ca0667 100644 --- a/libavformat/subtitles.c +++ b/libavformat/subtitles.c @@ -21,6 +21,7 @@ #include "avformat.h" #include "subtitles.h" #include "avio_internal.h" +#include "libavutil/avassert.h" #include "libavutil/avstring.h" void ff_text_init_avio(void *s, FFTextReader *r, AVIOContext *pb) @@ -111,15 +112,20 @@ AVPacket *ff_subtitles_queue_insert(FFDemuxSubtitlesQueue *q, { AVPacket **subs, *sub; + if (!event) + av_assert1(len == 0); + if (merge && q->nb_subs > 0) { /* merge with previous event */ int old_len; sub = q->subs[q->nb_subs - 1]; old_len = sub->size; - if (av_grow_packet(sub, len) < 0) - return NULL; - memcpy(sub->data + old_len, event, len); + if (event) { + if (av_grow_packet(sub, len) < 0) + return NULL; + memcpy(sub->data + old_len, event, len); + } } else { /* new event */ @@ -133,14 +139,16 @@ AVPacket *ff_subtitles_queue_insert(FFDemuxSubtitlesQueue *q, sub = av_packet_alloc(); if (!sub) return NULL; - if (av_new_packet(sub, len) < 0) { - av_packet_free(&sub); - return NULL; + if (event) { + if (av_new_packet(sub, len) < 0) { + av_packet_free(&sub); + return NULL; + } + memcpy(sub->data, event, len); } - subs[q->nb_subs++] = sub; sub->flags |= AV_PKT_FLAG_KEY; sub->pts = sub->dts = 0; - memcpy(sub->data, event, len); + subs[q->nb_subs++] = sub; } return sub; } diff --git a/libavformat/subtitles.h b/libavformat/subtitles.h index 88665663c5..ad6b96ca6a 100644 --- a/libavformat/subtitles.h +++ b/libavformat/subtitles.h @@ -112,7 +112,7 @@ typedef struct { /** * Insert a new subtitle event. * - * @param event the subtitle line, may not be zero terminated + * @param event the subtitle line (not zero terminated) or NULL on not yet available event * @param len the length of the event (in strlen() sense, so without '\0') * @param merge set to 1 if the current event should be concatenated with the * previous one instead of adding a new entry, 0 otherwise
If ff_subtitles_queue_insert() were given a NULL buffer with 0 length, it would still attempt to grow the packet or memcpy depending on if merge option is enabled. In this commit, allow passing a NULL buffer with 0 length without attempting to do such operations. This way, if a subtitle demuxer happens to pass an empty cue or wants to use av_get_packet() to read bytes, there are no unnecessary operations on the packet after it is allocated. Signed-off-by: Marth64 <marth64@proxyid.net> --- libavformat/subtitles.c | 24 ++++++++++++++++-------- libavformat/subtitles.h | 2 +- 2 files changed, 17 insertions(+), 9 deletions(-)