[FFmpeg-devel] libavformat/hls: Support metadata updates from subdemuxers

Submitted by rshaffer@tunein.com on Feb. 2, 2018, 2:44 a.m.

Details

Message ID 20180202024434.68757-1-rshaffer@tunein.com
State New
Headers show

Commit Message

rshaffer@tunein.com Feb. 2, 2018, 2:44 a.m.
From: Richard Shaffer <rshaffer@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(+)

Comments

wm4 Feb. 2, 2018, 6:18 a.m.
On Thu,  1 Feb 2018 18:44:34 -0800
rshaffer@tunein.com wrote:

> From: Richard Shaffer <rshaffer@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.

> +    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?

> +    }
> +
> +    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.

> +
>  /* 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).

>          add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_AUDIO);
>          add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_VIDEO);
>          add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_SUBTITLE);
> @@ -2170,6 +2206,8 @@ static int hls_read_packet(AVFormatContext *s, AVPacket *pkt)
>              return ret;
>          }
>  
> +        update_metadata_from_subdemuxer(pls, 0);
> +
>          /* check if noheader flag has been cleared by the subdemuxer */
>          if (pls->has_noheader_flag && !(pls->ctx->ctx_flags & AVFMTCTX_NOHEADER)) {
>              pls->has_noheader_flag = 0;
rshaffer@tunein.com Feb. 2, 2018, 7:50 a.m.
On Thu, Feb 1, 2018 at 10:18 PM, wm4 <nfxjfg@googlemail.com> wrote:
> On Thu,  1 Feb 2018 18:44:34 -0800
> rshaffer@tunein.com wrote:
>
>> From: Richard Shaffer <rshaffer@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.

>
>> +    }
>> +
>> +    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.

>
>> +
>>  /* 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.

>
>>          add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_AUDIO);
>>          add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_VIDEO);
>>          add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_SUBTITLE);
>> @@ -2170,6 +2206,8 @@ static int hls_read_packet(AVFormatContext *s, AVPacket *pkt)
>>              return ret;
>>          }
>>
>> +        update_metadata_from_subdemuxer(pls, 0);
>> +
>>          /* check if noheader flag has been cleared by the subdemuxer */
>>          if (pls->has_noheader_flag && !(pls->ctx->ctx_flags & AVFMTCTX_NOHEADER)) {
>>              pls->has_noheader_flag = 0;
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
wm4 Feb. 2, 2018, 11:52 a.m.
On Thu, 1 Feb 2018 23:50:02 -0800
Richard Shaffer <rshaffer@tunein.com> wrote:

> On Thu, Feb 1, 2018 at 10:18 PM, wm4 <nfxjfg@googlemail.com> wrote:
> > On Thu,  1 Feb 2018 18:44:34 -0800
> > rshaffer@tunein.com wrote:
> >  
> >> From: Richard Shaffer <rshaffer@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.)

Patch hide | download patch | download mbox

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) {
+    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;
+        }
+    }
+
+    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;
+        }
+    }
+}
+
 /* 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);
+
         add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_AUDIO);
         add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_VIDEO);
         add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_SUBTITLE);
@@ -2170,6 +2206,8 @@  static int hls_read_packet(AVFormatContext *s, AVPacket *pkt)
             return ret;
         }
 
+        update_metadata_from_subdemuxer(pls, 0);
+
         /* check if noheader flag has been cleared by the subdemuxer */
         if (pls->has_noheader_flag && !(pls->ctx->ctx_flags & AVFMTCTX_NOHEADER)) {
             pls->has_noheader_flag = 0;