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

[Richard Shaffer]

On Fri, May 18, 2018 at 2:54 AM, wm4 <nfxjfg at googlemail.com> wrote:

> On Thu, 17 May 2018 20:49:42 -0700
> rshaffer at tunein.com wrote:
>
> > From: Richard Shaffer <rshaffer at 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.
> > ---
> >  libavformat/hls.c | 34 +++++++++++++++++++---------------
> >  1 file changed, 19 insertions(+), 15 deletions(-)
> >
> > diff --git a/libavformat/hls.c b/libavformat/hls.c
> > index 3d4f7f2647..3e2edb3484 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,15 @@ 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.
> > +     */
> > +    pls->ctx->event_flags |= AVFMT_EVENT_FLAG_METADATA_UPDATED;
>
> Can't ID3 tags happen in large quantities with that ID3 timestamp hack
> (curse whoever invented it)? Wouldn't that lead to a large number of
> redundant events? Not sure though, I don't have the overview.
>

Yes, that's a good point. If timestamps are in ID3 tags, they'll be at the
start of every segment. I can think of several ways to handle that:

Option 1: Technically it is updated metadata, so mark it as updated. Leave
it up to the API caller to figure out whether they care about it or not.

Option 2: Filter out timestamp ID3 frames, and only mark it as updated if
some other ID3 tag or frame is present.

Option 3: Compare the new metadata with the last seen metadata, and only
set the event flag if something besides the timestamp has actually changed.
That would filter out false updates for the API consumer, but I'm pretty
sure nothing else that handles metadata updates works this way.

Any thoughts? Personally I'd lean towards 1 or 2.


>
> >
> >      if (timestamp != AV_NOPTS_VALUE) {
> >          pls->id3_mpegts_timestamp = timestamp;
> > @@ -1037,12 +1036,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 +1936,15 @@ 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 = ~AVFMT_EVENT_FLAG_METADATA_UPDATED;
>
> I think ~(unsigned)AVFMT_EVENT_... would be preferable for maximum
> correctness.
>

That's not the only reason this is wrong. I'll fix it.


>
> > +        }
> >
> >          add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_AUDIO);
> >          add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_VIDEO);
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>