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. > ff_vorbis_stream_comment(s, st, os->buf + os->pstart + 4, os->psize > - 4); > } > > -- > 2.39.5 (Apple Git-154) > > _______________________________________________ > 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". _______________________________________________ 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".
