[FFmpeg-devel] [PATCH] libavformat/hls: Support metadata updates from subdemuxers
wm4
nfxjfg at googlemail.com
Fri Feb 2 13:52:00 EET 2018
On Thu, 1 Feb 2018 23:50:02 -0800
Richard Shaffer <rshaffer at tunein.com> wrote:
> On Thu, Feb 1, 2018 at 10:18 PM, wm4 <nfxjfg at googlemail.com> wrote:
> > On Thu, 1 Feb 2018 18:44:34 -0800
> > rshaffer at tunein.com wrote:
> >
> >> From: Richard Shaffer <rshaffer at tunein.com>
> >>
> >> If a subdemuxer has the updated metadata event flag set, the metadata is copied
> >> to the corresponding stream. The flag is cleared on the subdemuxer and the
> >> appropriate event flag is set on the stream.
> >> ---
> >> This is semi-related to a patch I recently sent to enable parsing ID3 tags from
> >> .aac files between ADTS headers. However, it may be generically useful for
> >> other segment formats that support metadata updates.
> >>
> >> -Richard
> >>
> >> libavformat/hls.c | 38 ++++++++++++++++++++++++++++++++++++++
> >> 1 file changed, 38 insertions(+)
> >>
> >> diff --git a/libavformat/hls.c b/libavformat/hls.c
> >> index 9bd54c84cc..e48845de34 100644
> >> --- a/libavformat/hls.c
> >> +++ b/libavformat/hls.c
> >> @@ -1062,6 +1062,7 @@ static void handle_id3(AVIOContext *pb, struct playlist *pls)
> >> /* demuxer not yet opened, defer picture attachment */
> >> pls->id3_deferred_extra = extra_meta;
> >>
> >> + ff_id3v2_parse_priv_dict(&metadata, &extra_meta);
> >> av_dict_copy(&pls->ctx->metadata, metadata, 0);
> >> pls->id3_initial = metadata;
> >>
> >> @@ -1589,6 +1590,34 @@ static void add_metadata_from_renditions(AVFormatContext *s, struct playlist *pl
> >> }
> >> }
> >>
> >> +/* update metadata on main streams, if necessary */
> >> +static void update_metadata_from_subdemuxer(struct playlist *pls, int ignore_flags) {
> >
> > Normally we put the { on a separate line for functions.
>
> I knew that but forgot. I'll fix it in the next iteration.
>
> >
> >> + int i;
> >> +
> >> + if (pls->n_main_streams) {
> >> + AVStream *st = pls->main_streams[0];
> >> + if (ignore_flags) {
> >> + av_dict_copy(&st->metadata, pls->ctx->metadata, 0);
> >> + } else if (pls->ctx->event_flags & AVFMT_EVENT_FLAG_METADATA_UPDATED) {
> >> + av_dict_copy(&st->metadata, pls->ctx->metadata, 0);
> >> + pls->ctx->event_flags &= ~AVFMT_EVENT_FLAG_METADATA_UPDATED;
> >> + st->event_flags |= AVSTREAM_EVENT_FLAG_METADATA_UPDATED;
> >> + }
> >
> > I don't get understand this: why only stream 0? Isn't this done below
> > already?
>
> The code above copies metadata from pls->ctx->metadata, but the code
> below copies from pls->ctx->streams[i]->metadata for each stream in
> the subdemuxer. I think this currently would only apply to oggvorbis
> and nut demuxers. Maybe it's not a relevant use case for those, in
> which case I could just remove the for loop. I could also add a
> comment so that it's clear without having to look three times at the
> code.
Yeah, it was hard to spot. If it'll be effectively dead code, I'd say
it's better to just remove this. Nobody is going to try to use ogg or
nut fragments with HLS.
> >
> >> + }
> >> +
> >> + for (i = 0; i < pls->ctx->nb_streams; i++) {
> >> + AVStream *ist = pls->ctx->streams[i];
> >> + AVStream *st = pls->main_streams[i];
> >> + if (ignore_flags) {
> >> + av_dict_copy(&st->metadata, ist->metadata, 0);
> >> + } else if (ist->event_flags & AVSTREAM_EVENT_FLAG_METADATA_UPDATED) {
> >> + av_dict_copy(&st->metadata, ist->metadata, 0);
> >> + ist->event_flags &= ~AVSTREAM_EVENT_FLAG_METADATA_UPDATED;
> >> + st->event_flags |= AVSTREAM_EVENT_FLAG_METADATA_UPDATED;
> >> + }
> >> + }
> >> +}
> >
> > Like mentioned in the other patch, av_dict_copy not clearing the target
> > dict might be unintended.
>
> The intention is to not remove existing keys in the metadata, but only
> update keys that are new or changed. We do also set metadata in
> add_stream_to_programs and add_metadata_from_renditions. Presumably we
> don't want to delete that. This also seems to be the behavior when we
> have updated data from other demuxers or Icy, so I wanted to implement
> the same behavior here.
Fine.
> >
> >> +
> >> /* if timestamp was in valid range: returns 1 and sets seq_no
> >> * if not: returns 0 and sets seq_no to closest segment */
> >> static int find_timestamp_in_playlist(HLSContext *c, struct playlist *pls,
> >> @@ -1960,6 +1989,7 @@ static int hls_read_header(AVFormatContext *s)
> >> if (pls->id3_deferred_extra && pls->ctx->nb_streams == 1) {
> >> ff_id3v2_parse_apic(pls->ctx, &pls->id3_deferred_extra);
> >> avformat_queue_attached_pictures(pls->ctx);
> >> + ff_id3v2_parse_priv(pls->ctx, &pls->id3_deferred_extra);
> >> ff_id3v2_free_extra_meta(&pls->id3_deferred_extra);
> >> pls->id3_deferred_extra = NULL;
> >> }
> >> @@ -1986,6 +2016,12 @@ static int hls_read_header(AVFormatContext *s)
> >> if (ret < 0)
> >> goto fail;
> >>
> >> + /*
> >> + * Copy any metadata from playlist to main streams, but do not set
> >> + * event flags.
> >> + */
> >> + update_metadata_from_subdemuxer(pls, 1);
> >> +
> >
> > Possibly would be nicer to drop the ignore_flags parameter, and just
> > unset the event flag at the end of read_header (maybe even in generic
> > code).
>
> When we are in hls_read_header, the sub-demuxers are not going to set
> the event flag, and so I want to unconditionally copy the metadata. If
> we're in hls_read_packet, though, I only want to copy metadata if the
> subdemuxer says it's been updated. Maybe the better thing to do is
> just update the metadata inline here in hls_read_header, since it's
> only a few lines of code, and remove the extra parameter and logic
> from update_metadata_from_subdemuxer.
I didn't consider that the event flag is initially not set. (I first
though that doesn't really happen in practice, but I guess the generic
libavformat ID3v2 reading code will handle that initial ID3v2, and not
set the event flags.)
No opinion then. Maybe you could try and see whether updating inline
looks prettier. (Maybe there's a good place where it iterates the
streams anyway, then it'd be just a single call to copy the dict.)
More information about the ffmpeg-devel
mailing list