Re: [FFmpeg-devel] [PATCH v7 1/5] avformat/subtitles: extend ff_subtitles_queue_insert() to support empty events

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

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

_______________________________________________
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".