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

[Marvin Scholz <epirat07@gmail.com>]

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-request@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-request@ffmpeg.org with subject "unsubscribe".