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 jeu. 30 janv. 2025 à 08:08, Romain Beauxis <romain.beauxis@gmail.com> a
écrit :
>
>
>
>
> 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..

I'm sending another version of the patch. It pushes the metadata decoding
to the codec decoding.

This makes sense from separation of concern perspective: packets keep
seeing the full encoded data and decoded frames see the decoded metadata.

However, this adds a dependency on libavformat to libavcodec.

I also looked at moving the vorbis comment parsing to libavutil but there's
a lot of metadata definitions and utils that at located in libavformat at
the moment so perhaps it makes sense this way.

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