Le jeu. 30 janv. 2025 à 08:08, Romain Beauxis <romain.beau...@gmail.com> a écrit : > > > > > Le mer. 29 janv. 2025 à 17:40, Marvin Scholz <epira...@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-requ...@ffmpeg.org with subject "unsubscribe".
