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

wm4 nfxjfg at googlemail.com
Fri May 18 22:08:47 EEST 2018


On Fri, 18 May 2018 11:54:52 -0700
Richard Shaffer <rshaffer at tunein.com> wrote:

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

2 sounds good.

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

Oh, didn't notice.


More information about the ffmpeg-devel mailing list