[FFmpeg-devel] [PATCH 1/2] avformat/hls demuxer: Add WebVTT subtitle support

[Soft Works]

> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces at ffmpeg.org> On Behalf Of
> Andreas Rheinhardt
> Sent: Freitag, 21. Februar 2025 10:18
> To: ffmpeg-devel at ffmpeg.org
> Subject: Re: [FFmpeg-devel] [PATCH 1/2] avformat/hls demuxer: Add WebVTT
> subtitle support
> 
> softworkz:
> > From: softworkz <softworkz at hotmail.com>
> >

[...]

> >      /* Open the demuxer for each playlist */
> >      for (i = 0; i < c->n_playlists; i++) {
> >          struct playlist *pls = c->playlists[i];
> > @@ -2107,8 +2230,12 @@ static int hls_read_header(AVFormatContext
> *s)
> >              return AVERROR(ENOMEM);
> >          }
> >
> > -        ffio_init_context(&pls->pb, pls->read_buffer, INITIAL_BUFFER_SIZE, 0,
> pls,
> > -                          read_data, NULL, NULL);
> > +        if (pls->is_subtitle)
> > +            ffio_init_context(&pls->pb, (unsigned char*)av_strdup(vtt_sample),
> (int)strlen(vtt_sample), 0, pls,
> > +                                        NULL, NULL, NULL);
> > +        else
> > +            ffio_init_context(&pls->pb, pls->read_buffer, INITIAL_BUFFER_SIZE,
> 0, pls,
> > +                                        read_data_continuous, NULL, NULL);
> 
> 1. Unchecked av_strdup().

Yup, thanks.

> 2. Is duplicating the string even needed? Can't we simply set the
> AVIOContext to NULL before closing the AVFormatContext?

The lifetime of these two is not aligned. Also, I wouldn't want to 
make assumptions as to what is being done with that buffer at
other places of the code (where the constant might be subject 
of an attempt to get freed.

[...]

> > +        if (pls->is_subtitle) {
> > +            avformat_free_context(pls->ctx);
> 
> Doesn't the copy of vtt_sample leak here?

Unless I'm overseeing something, the FFIOContext owns the buffer with the
copied vtt_sample string, and that FFIOContext, is owned by the playlist 
(pls->pb). It is freed in free_playlist_list():

	av_freep(&pls->pb.pub.buffer);

In that function it would be ugly to make a distinction depending on which 
type of memory the buffer would be carrying, no?

Thanks
sw