| Message ID | 20240321035207.3849560-2-marth64@proxyid.net |
|---|---|
| State | New |
| Headers | show |
| Series | RCWT Closed Captions demuxer (meta) | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
This is in response to the last paragraph of feedback in https://ffmpeg.org//pipermail/ffmpeg-devel/2024-March/323858.html
Marth64: > If ff_subtitles_queue_insert() were to 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, consider a NULL buffer with 0 length as > an empty event and do not attempt to modify the packet. > 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 | 4 ++++ > libavformat/subtitles.h | 2 +- > 2 files changed, 5 insertions(+), 1 deletion(-) > > diff --git a/libavformat/subtitles.c b/libavformat/subtitles.c > index 3413763c7b..38d2ffb8a9 100644 > --- a/libavformat/subtitles.c > +++ b/libavformat/subtitles.c > @@ -117,6 +117,8 @@ AVPacket *ff_subtitles_queue_insert(FFDemuxSubtitlesQueue *q, > int old_len; > sub = q->subs[q->nb_subs - 1]; > old_len = sub->size; > + if (event == NULL && len == 0) > + return sub; Checks for NULL are typically written as !event. I'd prefer if (!event) { av_assert1(len == 0); return sub; } as this also makes clear that !event with len > 0 must not happen and also avoids one runtime check for ordinary users (with assert-level 0). > if (av_grow_packet(sub, len) < 0) > return NULL; > memcpy(sub->data + old_len, event, len); > @@ -140,6 +142,8 @@ AVPacket *ff_subtitles_queue_insert(FFDemuxSubtitlesQueue *q, > subs[q->nb_subs++] = sub; > sub->flags |= AV_PKT_FLAG_KEY; > sub->pts = sub->dts = 0; > + if (event == NULL && len == 0) > + return sub; > memcpy(sub->data, event, len); This will allocate sub->data to a buffer of size AV_INPUT_BUFFER_PADDING_SIZE and usable size of zero. This is not what is intended: You should not allocate anything at all, i.e. skip av_new_packet(). This small buffer (together with the AVBuffer and AVBufferRef allocated for it) will leak with your current patch 2. > } > return sub; > diff --git a/libavformat/subtitles.h b/libavformat/subtitles.h > index 88665663c5..ba162fa503 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 empty event 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
Thank you Andreas, for walking me through this in detail. I will update accordingly and test.
diff --git a/libavformat/subtitles.c b/libavformat/subtitles.c index 3413763c7b..38d2ffb8a9 100644 --- a/libavformat/subtitles.c +++ b/libavformat/subtitles.c @@ -117,6 +117,8 @@ AVPacket *ff_subtitles_queue_insert(FFDemuxSubtitlesQueue *q, int old_len; sub = q->subs[q->nb_subs - 1]; old_len = sub->size; + if (event == NULL && len == 0) + return sub; if (av_grow_packet(sub, len) < 0) return NULL; memcpy(sub->data + old_len, event, len); @@ -140,6 +142,8 @@ AVPacket *ff_subtitles_queue_insert(FFDemuxSubtitlesQueue *q, subs[q->nb_subs++] = sub; sub->flags |= AV_PKT_FLAG_KEY; sub->pts = sub->dts = 0; + if (event == NULL && len == 0) + return sub; memcpy(sub->data, event, len); } return sub; diff --git a/libavformat/subtitles.h b/libavformat/subtitles.h index 88665663c5..ba162fa503 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 empty 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 to 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, consider a NULL buffer with 0 length as an empty event and do not attempt to modify the packet. 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 | 4 ++++ libavformat/subtitles.h | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-)