Re: [FFmpeg-devel] [PATCH 1/2] libavformat/hls.c: support in-stream ID3 metadata update.

[Romain Beauxis <romain.beauxis@gmail.com>]

Hi all,

Le dim. 7 avr. 2024 à 09:46, Romain Beauxis <romain.beauxis@gmail.com> a
écrit :

>
>
> Le dim. 7 avr. 2024 à 05:44, Steven Liu <lingjiujianke@gmail.com> a
> écrit :
>
>> Romain Beauxis <toots@rastageeks.org> 于2024年3月26日周二 08:58写道：
>> >
>> > This patch adds support for updating HLS metadata passed as ID3 frames.
>> >
>> > This seems like a pretty straight-forward improvement. Updating the
>> > metadaata of the first stream seems to be the mechanism is other places
>> > in the code and works as expected.
>>
>>
Would it be possible to get this patch committed? My understanding is that
it has been reviewed so this should be the next step?

The second patch adding FATE tests could be committed separately if it
causes issues. The samples for it are here:
https://www.dropbox.com/scl/fo/1x74ztoa6yo9q49ignfnt/h?rlkey=xvg5nhgjr515gm6b375evm8n4&dl=0
and
should be placed into a $FATE_SAMPLES/hls-adts-meta directory.

Thanks!


> > ---
>> >  libavformat/hls.c | 54 ++++++++++++++++++++++++++++-------------------
>> >  1 file changed, 32 insertions(+), 22 deletions(-)
>> >
>> > diff --git a/libavformat/hls.c b/libavformat/hls.c
>> > index f6b44c2e35..ba6634d57a 100644
>> > --- a/libavformat/hls.c
>> > +++ b/libavformat/hls.c
>> > @@ -93,6 +93,12 @@ enum PlaylistType {
>> >      PLS_TYPE_VOD
>> >  };
>> >
>> > +#define ID3_PRIV_OWNER_TS
>> "com.apple.streaming.transportStreamTimestamp"
>> > +#define ID3_PRIV_OWNER_AUDIO_SETUP
>> "com.apple.streaming.audioDescription"
>> > +
>> > +#define ID3v2_PRIV_OWNER_TS ID3v2_PRIV_METADATA_PREFIX
>> ID3_PRIV_OWNER_TS
>> > +#define ID3v2_PRIV_OWNER_AUDIO_SETUP ID3v2_PRIV_METADATA_PREFIX
>> ID3_PRIV_OWNER_AUDIO_SETUP
>> > +
>> >  /*
>> >   * Each playlist has its own demuxer. If it currently is active,
>> >   * it has an open AVIOContext too, and potentially an AVPacket
>> > @@ -150,9 +156,7 @@ struct playlist {
>> >      int64_t id3_offset; /* in stream original tb */
>> >      uint8_t* id3_buf; /* temp buffer for id3 parsing */
>> >      unsigned int id3_buf_size;
>> > -    AVDictionary *id3_initial; /* data from first id3 tag */
>> > -    int id3_found; /* ID3 tag found at some point */
>> > -    int id3_changed; /* ID3 tag data has changed at some point */
>> > +    AVDictionary *last_id3; /* data from the last id3 tag */
>> >      ID3v2ExtraMeta *id3_deferred_extra; /* stored here until
>> subdemuxer is opened */
>> >
>> >      HLSAudioSetupInfo audio_setup_info;
>> > @@ -270,7 +274,7 @@ static void free_playlist_list(HLSContext *c)
>> >          av_freep(&pls->main_streams);
>> >          av_freep(&pls->renditions);
>> >          av_freep(&pls->id3_buf);
>> > -        av_dict_free(&pls->id3_initial);
>> > +        av_dict_free(&pls->last_id3);
>> >          ff_id3v2_free_extra_meta(&pls->id3_deferred_extra);
>> >          av_freep(&pls->init_sec_buf);
>> >          av_packet_free(&pls->pkt);
>> > @@ -1083,15 +1087,13 @@ static void parse_id3(AVFormatContext *s,
>> AVIOContext *pb,
>> >                        AVDictionary **metadata, int64_t *dts,
>> HLSAudioSetupInfo *audio_setup_info,
>> >                        ID3v2ExtraMetaAPIC **apic, ID3v2ExtraMeta
>> **extra_meta)
>> >  {
>> > -    static const char id3_priv_owner_ts[] =
>> "com.apple.streaming.transportStreamTimestamp";
>> > -    static const char id3_priv_owner_audio_setup[] =
>> "com.apple.streaming.audioDescription";
>> >      ID3v2ExtraMeta *meta;
>> >
>> >      ff_id3v2_read_dict(pb, metadata, ID3v2_DEFAULT_MAGIC, extra_meta);
>> >      for (meta = *extra_meta; meta; meta = meta->next) {
>> >          if (!strcmp(meta->tag, "PRIV")) {
>> >              ID3v2ExtraMetaPRIV *priv = &meta->data.priv;
>> > -            if (priv->datasize == 8 && !av_strncasecmp(priv->owner,
>> id3_priv_owner_ts, 44)) {
>> > +            if (priv->datasize == 8 && !av_strncasecmp(priv->owner,
>> ID3_PRIV_OWNER_TS, strlen(ID3_PRIV_OWNER_TS))) {
>> >                  /* 33-bit MPEG timestamp */
>> >                  int64_t ts = AV_RB64(priv->data);
>> >                  av_log(s, AV_LOG_DEBUG, "HLS ID3 audio timestamp
>> %"PRId64"\n", ts);
>> > @@ -1099,7 +1101,9 @@ static void parse_id3(AVFormatContext *s,
>> AVIOContext *pb,
>> >                      *dts = ts;
>> >                  else
>> >                      av_log(s, AV_LOG_ERROR, "Invalid HLS ID3 audio
>> timestamp %"PRId64"\n", ts);
>> > -            } else if (priv->datasize >= 8 &&
>> !av_strncasecmp(priv->owner, id3_priv_owner_audio_setup, 36)) {
>> > +            } else if (priv->datasize >= 8 &&
>> > +                       !av_strncasecmp(priv->owner,
>> ID3_PRIV_OWNER_AUDIO_SETUP, 36) &&
>> > +                       audio_setup_info) {
>> >                  ff_hls_senc_read_audio_setup_info(audio_setup_info,
>> priv->data, priv->datasize);
>> >              }
>> >          } else if (!strcmp(meta->tag, "APIC") && apic)
>> > @@ -1113,9 +1117,10 @@ static int id3_has_changed_values(struct
>> playlist *pls, AVDictionary *metadata,
>> >  {
>> >      const AVDictionaryEntry *entry = NULL;
>> >      const AVDictionaryEntry *oldentry;
>> > +
>> >      /* check that no keys have changed values */
>> >      while ((entry = av_dict_iterate(metadata, entry))) {
>> > -        oldentry = av_dict_get(pls->id3_initial, entry->key, NULL,
>> AV_DICT_MATCH_CASE);
>> > +        oldentry = av_dict_get(pls->last_id3, entry->key, NULL,
>> AV_DICT_MATCH_CASE);
>> >          if (!oldentry || strcmp(oldentry->value, entry->value) != 0)
>> >              return 1;
>> >      }
>> > @@ -1143,35 +1148,40 @@ static void handle_id3(AVIOContext *pb, struct
>> playlist *pls)
>> >      ID3v2ExtraMetaAPIC *apic = NULL;
>> >      ID3v2ExtraMeta *extra_meta = NULL;
>> >      int64_t timestamp = AV_NOPTS_VALUE;
>> > +    // Only set audio_setup_info on first id3 chunk.
>> > +    HLSAudioSetupInfo *audio_setup_info = pls->last_id3 ? NULL :
>> &pls->audio_setup_info;
>> >
>> > -    parse_id3(pls->ctx, pb, &metadata, &timestamp,
>> &pls->audio_setup_info, &apic, &extra_meta);
>> > +    parse_id3(pls->ctx, pb, &metadata, &timestamp, audio_setup_info,
>> &apic, &extra_meta);
>> >
>> > -    if (timestamp != AV_NOPTS_VALUE) {
>> > +    if (pls->id3_mpegts_timestamp == AV_NOPTS_VALUE && timestamp !=
>> AV_NOPTS_VALUE) {
>> >          pls->id3_mpegts_timestamp = timestamp;
>> >          pls->id3_offset = 0;
>> >      }
>> >
>> > -    if (!pls->id3_found) {
>> > -        /* initial ID3 tags */
>> > -        av_assert0(!pls->id3_deferred_extra);
>> > -        pls->id3_found = 1;
>> > -
>> > +    if (id3_has_changed_values(pls, metadata, apic)) {
>> >          /* get picture attachment and set text metadata */
>> >          if (pls->ctx->nb_streams)
>> >              ff_id3v2_parse_apic(pls->ctx, extra_meta);
>> > -        else
>> > +        else {
>> > +            av_assert0(!pls->id3_deferred_extra);
>> >              /* demuxer not yet opened, defer picture attachment */
>> >              pls->id3_deferred_extra = extra_meta;
>> > +        }
>> >
>> >          ff_id3v2_parse_priv_dict(&metadata, extra_meta);
>> > +
>> > +        av_dict_set(&metadata, ID3v2_PRIV_OWNER_TS, NULL, 0);
>> > +        av_dict_set(&metadata, ID3v2_PRIV_OWNER_AUDIO_SETUP, NULL, 0);
>> > +
>> > +        av_dict_free(&pls->ctx->metadata);
>> >          av_dict_copy(&pls->ctx->metadata, metadata, 0);
>> > -        pls->id3_initial = metadata;
>> >
>> > +        if (pls->n_main_streams)
>> > +            av_dict_copy(&pls->main_streams[0]->metadata, metadata, 0);
>> > +
>> > +        if (pls->last_id3) av_dict_free(&pls->last_id3);
>> > +        pls->last_id3 = metadata;
>> >      } else {
>> > -        if (!pls->id3_changed && id3_has_changed_values(pls, metadata,
>> apic)) {
>> > -            avpriv_report_missing_feature(pls->parent, "Changing ID3
>> metadata in HLS audio elementary stream");
>> > -            pls->id3_changed = 1;
>> > -        }
>> >          av_dict_free(&metadata);
>> >      }
>> >
>> > --
>> > 2.39.3 (Apple Git-145)
>> >
>> > _______________________________________________
>> > ffmpeg-devel mailing list
>> > ffmpeg-devel@ffmpeg.org
>> > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>> >
>> > To unsubscribe, visit link above, or email
>> > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>
>> I look at the second patch result:
>>
>> make: *** [fate-hls-adts-meta-demux] Error 1
>> cpu_flags(raw) = 0x000813DB
>> cpu_flags_str(raw) = mmx mmxext sse sse2 sse3 ssse3 sse4.1 sse4.2 cmov
>> aesni
>> cpu_flags(effective) = 0x000813DB
>> cpu_flags_str(effective) = mmx mmxext sse sse2 sse3 ssse3 sse4.1
>> sse4.2 cmov aesni
>> threads = 1 (cpu_count = 5)
>> make: Target 'fate' not remade because of errors.
>>
>>
>>
>>
>> https://patchwork.ffmpeg.org/project/ffmpeg/patch/20240326005639.27000-2-toots@rastageeks.org/
>>
>>
> Ok. The samples are here:
> https://www.dropbox.com/scl/fo/1x74ztoa6yo9q49ignfnt/h?rlkey=xvg5nhgjr515gm6b375evm8n4&dl=0
>
> If you place them under $FATE_SAMPLES/hls-adts-meta the test suite should
> pass.
>
> Are you the right person to also upload the samples?
>
> Thanks for your time on this!
>
>
>>
>> Thanks
>> Steven
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>
>
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".