Re: [FFmpeg-devel] [PATCH v2 1/2] Parse ogg/flac header again after processing a new chained ogg bitstream.

[Romain Beauxis <romain.beauxis@gmail.com>]

Le mer. 29 janv. 2025 à 17:40, Marvin Scholz <epirat07@gmail.com> a écrit :
>
>
>
> On 29 Jan 2025, at 15:40, Romain Beauxis wrote:
>
> > This patch makes sure that ogg/flac headers are parsed again when
> > encountering a new logic stream inside a chained ogg bistream[1].
> >
> > This patches makes it possible to retrieve metadata in chained ogg/flac
> > bitstreams. It is particularly important because ogg/flac is one of the
> > only (if not the only one) lossless container supported over
HTTP/icecast.
> >
> > The patch has been tested with various ogg/flac encoders and appears to
> > work fine with ffmpeg.
> >
> > Changes since last version:
> > * Make sure to clear the stream's metadata before parsing again.
> >
> > 1: https://xiph.org/ogg/doc/oggstream.html
> >
>
> Hi, thanks a lot for trying to solve this, see remarks inline below:
>
> > ---
> >  libavformat/oggdec.c       | 7 +++++--
> >  libavformat/oggparseflac.c | 2 ++
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavformat/oggdec.c b/libavformat/oggdec.c
> > index 5339fdd32c..d986e19817 100644
> > --- a/libavformat/oggdec.c
> > +++ b/libavformat/oggdec.c
> > @@ -239,8 +239,11 @@ static int ogg_replace_stream(AVFormatContext *s,
uint32_t serial, char *magic,
> >      os->start_trimming = 0;
> >      os->end_trimming = 0;
> >
> > -    /* Chained files have extradata as a new packet */
> > -    if (codec == &ff_opus_codec)
> > +    /* Parse opus and flac header on new chained bitstream.
> > +     * For opus, header contains required extradata as new packet
> > +     * For both formats, this makes it possible to read chained
metadata. */
> > +    if (codec == &ff_opus_codec ||
> > +        codec == &ff_flac_codec)
> >          os->header = -1;
> >
> >      return i;
> > diff --git a/libavformat/oggparseflac.c b/libavformat/oggparseflac.c
> > index f25ed9cc15..932907fa1a 100644
> > --- a/libavformat/oggparseflac.c
> > +++ b/libavformat/oggparseflac.c
> > @@ -72,6 +72,8 @@ flac_header (AVFormatContext * s, int idx)
> >
> >          avpriv_set_pts_info(st, 64, 1, samplerate);
> >      } else if (mdt == FLAC_METADATA_TYPE_VORBIS_COMMENT) {
> > +        /* New metadata packet; release old data. */
> > +        av_dict_free(&st->metadata);
>
> I do not think it’s fine to change this mid-demuxing, as the caller
> has no way to know this changed „behind its back“ so may still hold
> on to the old pointer and run into invalid memory access.
>
> I checked with James and he suggested that maybe propagating the new
> metadata with AVPacket’s side_data might be a better approach, similar
> to AV_PKT_DATA_NEW_EXTRADATA.

Thank you for the review!

Do you mean the st->metadata dictionary?

The nature of `av_dict` is to be an ever-changing pointer; each time a
value is changed, the underlying pointer is changed so I think keeping a
long-term pointer on it is a programming error.

It's also worth noting that this is the same method used currently by the
vorbis decoder.

That being said, updating the AVStream is not natural and requires the user
to constantly check on it.

It would be more natural to have:
* The headers packets flow out of the demuxer as it is now so anyone
operating at this level can see them and act accordingly.
* Have the decoded frame carry the new metadata, which is a more natural
place for it.

I'll look into this. Hopefully this is a better approach and also one that
could be generalized to all ogg-encapsulated formats..

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