[FFmpeg-devel,v2] avformat/hls: Properly expose intercepted ID3 tags to the API.

Submitted by rshaffer@tunein.com on May 18, 2018, 11:09 p.m.

Details

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

Commit Message

rshaffer@tunein.com May 18, 2018, 11:09 p.m.
From: Richard Shaffer <rshaffer@tunein.com>

The HLS demuxer will process any ID3 tags at the beginning of a segment in
order to obtain timestamp data. However, when this data was parsed on a second
or subsequent segment, the updated metadata would be discarded. This change
preserves the data and also sets the AVSTREAM_EVENT_FLAG_METADATA_UPDATED
event flag when appropriate.
---

Changes in this version:
* Only set metadata updated flag if we have non-timestamp metadata.
* Fix clearing of metadata updated flag in hls_read_header.
* Specifically cast operands to unsigned when using bitwise operators.

(This last one is almost certainly pedantic, but it doesn't hurt anything.)

 libavformat/hls.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

Comments

rshaffer@tunein.com May 30, 2018, 4:39 p.m.
On Fri, May 18, 2018 at 4:09 PM, <rshaffer@tunein.com> wrote:

> From: Richard Shaffer <rshaffer@tunein.com>
>
> The HLS demuxer will process any ID3 tags at the beginning of a segment in
> order to obtain timestamp data. However, when this data was parsed on a
> second
> or subsequent segment, the updated metadata would be discarded. This change
> preserves the data and also sets the AVSTREAM_EVENT_FLAG_METADATA_UPDATED
> event flag when appropriate.
> ---
>
> Changes in this version:
> * Only set metadata updated flag if we have non-timestamp metadata.
> * Fix clearing of metadata updated flag in hls_read_header.
> * Specifically cast operands to unsigned when using bitwise operators.
>
> (This last one is almost certainly pedantic, but it doesn't hurt anything.)
>
>  libavformat/hls.c | 39 ++++++++++++++++++++++++---------------
>  1 file changed, 24 insertions(+), 15 deletions(-)
>
> diff --git a/libavformat/hls.c b/libavformat/hls.c
> index 3d4f7f2647..342a022975 100644
> --- a/libavformat/hls.c
> +++ b/libavformat/hls.c
> @@ -982,18 +982,8 @@ static void parse_id3(AVFormatContext *s, AVIOContext
> *pb,
>  }
>
>  /* Check if the ID3 metadata contents have changed */
> -static int id3_has_changed_values(struct playlist *pls, AVDictionary
> *metadata,
> -                                  ID3v2ExtraMetaAPIC *apic)
> +static int id3_has_changed_values(struct playlist *pls,
> ID3v2ExtraMetaAPIC *apic)
>  {
> -    AVDictionaryEntry *entry = NULL;
> -    AVDictionaryEntry *oldentry;
> -    /* check that no keys have changed values */
> -    while ((entry = av_dict_get(metadata, "", entry,
> AV_DICT_IGNORE_SUFFIX))) {
> -        oldentry = av_dict_get(pls->id3_initial, entry->key, NULL,
> AV_DICT_MATCH_CASE);
> -        if (!oldentry || strcmp(oldentry->value, entry->value) != 0)
> -            return 1;
> -    }
> -
>      /* check if apic appeared */
>      if (apic && (pls->ctx->nb_streams != 2 || !pls->ctx->streams[1]->
> attached_pic.data))
>          return 1;
> @@ -1019,6 +1009,19 @@ static void handle_id3(AVIOContext *pb, struct
> playlist *pls)
>      int64_t timestamp = AV_NOPTS_VALUE;
>
>      parse_id3(pls->ctx, pb, &metadata, &timestamp, &apic, &extra_meta);
> +    ff_id3v2_parse_priv_dict(&metadata, &extra_meta);
> +    av_dict_copy(&pls->ctx->metadata, metadata, 0);
> +    /*
> +     * If we've handled any ID3 metadata here, it's not going to be seen
> by the
> +     * sub-demuxer. In the case that we got here because of an IO call
> during
> +     * hls_read_header, this will be cleared. Otherwise, it provides the
> +     * necessary hint to hls_read_packet that there is new metadata. Note
> that
> +     * we only set the update flag if we have metadata other than a
> timestamp.
> +     */
> +    if (av_dict_count(metadata) > (timestamp == AV_NOPTS_VALUE ? 0 : 1)) {
> +        pls->ctx->event_flags = (unsigned)pls->ctx->event_flags |
> +            (unsigned)AVFMT_EVENT_FLAG_METADATA_UPDATED;
> +    }
>
>      if (timestamp != AV_NOPTS_VALUE) {
>          pls->id3_mpegts_timestamp = timestamp;
> @@ -1037,12 +1040,10 @@ 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;
>
>      } else {
> -        if (!pls->id3_changed && id3_has_changed_values(pls, metadata,
> apic)) {
> +        if (!pls->id3_changed && id3_has_changed_values(pls, apic)) {
>              avpriv_report_missing_feature(pls->ctx, "Changing ID3
> metadata in HLS audio elementary stream");
>              pls->id3_changed = 1;
>          }
> @@ -1939,8 +1940,16 @@ static int hls_read_header(AVFormatContext *s)
>           * Copy any metadata from playlist to main streams, but do not set
>           * event flags.
>           */
> -        if (pls->n_main_streams)
> +        if (pls->n_main_streams) {
>              av_dict_copy(&pls->main_streams[0]->metadata,
> pls->ctx->metadata, 0);
> +            /*
> +             * If we've intercepted metadata, we will have set this event
> flag.
> +             * Clear it to avoid confusion, since otherwise we will flag
> it as
> +             * new metadata on the next call to hls_read_packet.
> +             */
> +            pls->ctx->event_flags = (unsigned)pls->ctx->event_flags &
> +                ~(unsigned)AVFMT_EVENT_FLAG_METADATA_UPDATED;
> +        }
>
>          add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_AUDIO);
>          add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_VIDEO);
> --
> 2.15.1 (Apple Git-101)
>
>
Just wanted to ping the list one last time and see if I can get anyone to
take a look at this changeset.

Thanks,

-Richard

Patch hide | download patch | download mbox

diff --git a/libavformat/hls.c b/libavformat/hls.c
index 3d4f7f2647..342a022975 100644
--- a/libavformat/hls.c
+++ b/libavformat/hls.c
@@ -982,18 +982,8 @@  static void parse_id3(AVFormatContext *s, AVIOContext *pb,
 }
 
 /* Check if the ID3 metadata contents have changed */
-static int id3_has_changed_values(struct playlist *pls, AVDictionary *metadata,
-                                  ID3v2ExtraMetaAPIC *apic)
+static int id3_has_changed_values(struct playlist *pls, ID3v2ExtraMetaAPIC *apic)
 {
-    AVDictionaryEntry *entry = NULL;
-    AVDictionaryEntry *oldentry;
-    /* check that no keys have changed values */
-    while ((entry = av_dict_get(metadata, "", entry, AV_DICT_IGNORE_SUFFIX))) {
-        oldentry = av_dict_get(pls->id3_initial, entry->key, NULL, AV_DICT_MATCH_CASE);
-        if (!oldentry || strcmp(oldentry->value, entry->value) != 0)
-            return 1;
-    }
-
     /* check if apic appeared */
     if (apic && (pls->ctx->nb_streams != 2 || !pls->ctx->streams[1]->attached_pic.data))
         return 1;
@@ -1019,6 +1009,19 @@  static void handle_id3(AVIOContext *pb, struct playlist *pls)
     int64_t timestamp = AV_NOPTS_VALUE;
 
     parse_id3(pls->ctx, pb, &metadata, &timestamp, &apic, &extra_meta);
+    ff_id3v2_parse_priv_dict(&metadata, &extra_meta);
+    av_dict_copy(&pls->ctx->metadata, metadata, 0);
+    /*
+     * If we've handled any ID3 metadata here, it's not going to be seen by the
+     * sub-demuxer. In the case that we got here because of an IO call during
+     * hls_read_header, this will be cleared. Otherwise, it provides the
+     * necessary hint to hls_read_packet that there is new metadata. Note that
+     * we only set the update flag if we have metadata other than a timestamp.
+     */
+    if (av_dict_count(metadata) > (timestamp == AV_NOPTS_VALUE ? 0 : 1)) {
+        pls->ctx->event_flags = (unsigned)pls->ctx->event_flags |
+            (unsigned)AVFMT_EVENT_FLAG_METADATA_UPDATED;
+    }
 
     if (timestamp != AV_NOPTS_VALUE) {
         pls->id3_mpegts_timestamp = timestamp;
@@ -1037,12 +1040,10 @@  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;
 
     } else {
-        if (!pls->id3_changed && id3_has_changed_values(pls, metadata, apic)) {
+        if (!pls->id3_changed && id3_has_changed_values(pls, apic)) {
             avpriv_report_missing_feature(pls->ctx, "Changing ID3 metadata in HLS audio elementary stream");
             pls->id3_changed = 1;
         }
@@ -1939,8 +1940,16 @@  static int hls_read_header(AVFormatContext *s)
          * Copy any metadata from playlist to main streams, but do not set
          * event flags.
          */
-        if (pls->n_main_streams)
+        if (pls->n_main_streams) {
             av_dict_copy(&pls->main_streams[0]->metadata, pls->ctx->metadata, 0);
+            /*
+             * If we've intercepted metadata, we will have set this event flag.
+             * Clear it to avoid confusion, since otherwise we will flag it as
+             * new metadata on the next call to hls_read_packet.
+             */
+            pls->ctx->event_flags = (unsigned)pls->ctx->event_flags &
+                ~(unsigned)AVFMT_EVENT_FLAG_METADATA_UPDATED;
+        }
 
         add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_AUDIO);
         add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_VIDEO);