diff mbox series

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

Message ID 20240326005639.27000-1-toots@rastageeks.org
State New
Headers show
Series [FFmpeg-devel,1/2] libavformat/hls.c: support in-stream ID3 metadata update. | expand

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Romain Beauxis March 26, 2024, 12:56 a.m. UTC
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.

---
 libavformat/hls.c | 54 ++++++++++++++++++++++++++++-------------------
 1 file changed, 32 insertions(+), 22 deletions(-)

Comments

Romain Beauxis March 28, 2024, 10:51 p.m. UTC | #1
On Mon, Mar 25, 2024, 19:58 Romain Beauxis <toots@rastageeks.org> wrote:

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


Hello!

Any interest in reviewing this?


---
>  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)
>
>
Steven Liu March 29, 2024, 8:44 a.m. UTC | #2
> 在 2024年3月29日,06:51,Romain Beauxis <toots@rastageeks.org> 写道:
> 
> On Mon, Mar 25, 2024, 19:58 Romain Beauxis <toots@rastageeks.org> wrote:
> 
>> 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.
>> 
> 
> 
> Hello!
> 
> Any interest in reviewing this?
Let me test these patchset need some days, 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".
Steven Liu March 31, 2024, 10:52 a.m. UTC | #3
> On Mar 29, 2024, at 06:51, Romain Beauxis <toots@rastageeks.org> wrote:
> 
> On Mon, Mar 25, 2024, 19:58 Romain Beauxis <toots@rastageeks.org> wrote:
> 
>> 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.
>> 
> 
> 
> Hello!
> 
> Any interest in reviewing this?

Patchset looks good to me, looks better than before patch.
> 
> 
> ---
>> 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".
>
Romain Beauxis March 31, 2024, 5:46 p.m. UTC | #4
On Sun, Mar 31, 2024, 05:52 Liu Steven <lq@chinaffmpeg.org> wrote:

>
>
> > On Mar 29, 2024, at 06:51, Romain Beauxis <toots@rastageeks.org> wrote:
> >
> > On Mon, Mar 25, 2024, 19:58 Romain Beauxis <toots@rastageeks.org> wrote:
> >
> >> 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.
> >>
> >
> >
> > Hello!
> >
> > Any interest in reviewing this?
>
> Patchset looks good to me, looks better than before patch.
>

Great! Happy to provide the fate samples too.


> ---
> >> 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".
> >
>
> _______________________________________________
> 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".
>
Romain Beauxis April 6, 2024, 5:48 p.m. UTC | #5
On Sun, Mar 31, 2024, 12:46 Romain Beauxis <romain.beauxis@gmail.com> wrote:

>
>
> On Sun, Mar 31, 2024, 05:52 Liu Steven <lq@chinaffmpeg.org> wrote:
>
>>
>>
>> > On Mar 29, 2024, at 06:51, Romain Beauxis <toots@rastageeks.org> wrote:
>> >
>> > On Mon, Mar 25, 2024, 19:58 Romain Beauxis <toots@rastageeks.org>
>> wrote:
>> >
>> >> 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.
>> >>
>> >
>> >
>> > Hello!
>> >
>> > Any interest in reviewing this?
>>
>> Patchset looks good to me, looks better than before patch.
>>
>
> Great! Happy to provide the fate samples too.
>
>
Hi!

Any update on this?

Let me know if I can do anything to help!




> > ---
>> >> 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".
>> >
>>
>> _______________________________________________
>> 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".
>>
>
Steven Liu April 7, 2024, 10:44 a.m. UTC | #6
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.
>
> ---
>  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/


Thanks
Steven
Romain Beauxis April 7, 2024, 2:46 p.m. UTC | #7
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.
> >
> > ---
> >  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".
>
Romain Beauxis April 11, 2024, 2:04 p.m. UTC | #8
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".
>>
>
diff mbox series

Patch

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);
     }