[FFmpeg-devel] [PATCH] libavformat/hls: Support metadata updates from subdemuxers
Richard Shaffer
rshaffer at tunein.com
Fri Feb 2 09:50:02 EET 2018
On Thu, Feb 1, 2018 at 10:18 PM, wm4 <nfxjfg at googlemail.com> wrote:
> On Thu, 1 Feb 2018 18:44:34 -0800
> rshaffer at tunein.com wrote:
>
>> From: Richard Shaffer <rshaffer at tunein.com>
>>
>> If a subdemuxer has the updated metadata event flag set, the metadata is copied
>> to the corresponding stream. The flag is cleared on the subdemuxer and the
>> appropriate event flag is set on the stream.
>> ---
>> This is semi-related to a patch I recently sent to enable parsing ID3 tags from
>> .aac files between ADTS headers. However, it may be generically useful for
>> other segment formats that support metadata updates.
>>
>> -Richard
>>
>> libavformat/hls.c | 38 ++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 38 insertions(+)
>>
>> diff --git a/libavformat/hls.c b/libavformat/hls.c
>> index 9bd54c84cc..e48845de34 100644
>> --- a/libavformat/hls.c
>> +++ b/libavformat/hls.c
>> @@ -1062,6 +1062,7 @@ 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;
>>
>> @@ -1589,6 +1590,34 @@ static void add_metadata_from_renditions(AVFormatContext *s, struct playlist *pl
>> }
>> }
>>
>> +/* update metadata on main streams, if necessary */
>> +static void update_metadata_from_subdemuxer(struct playlist *pls, int ignore_flags) {
>
> Normally we put the { on a separate line for functions.
I knew that but forgot. I'll fix it in the next iteration.
>
>> + int i;
>> +
>> + if (pls->n_main_streams) {
>> + AVStream *st = pls->main_streams[0];
>> + if (ignore_flags) {
>> + av_dict_copy(&st->metadata, pls->ctx->metadata, 0);
>> + } else if (pls->ctx->event_flags & AVFMT_EVENT_FLAG_METADATA_UPDATED) {
>> + av_dict_copy(&st->metadata, pls->ctx->metadata, 0);
>> + pls->ctx->event_flags &= ~AVFMT_EVENT_FLAG_METADATA_UPDATED;
>> + st->event_flags |= AVSTREAM_EVENT_FLAG_METADATA_UPDATED;
>> + }
>
> I don't get understand this: why only stream 0? Isn't this done below
> already?
The code above copies metadata from pls->ctx->metadata, but the code
below copies from pls->ctx->streams[i]->metadata for each stream in
the subdemuxer. I think this currently would only apply to oggvorbis
and nut demuxers. Maybe it's not a relevant use case for those, in
which case I could just remove the for loop. I could also add a
comment so that it's clear without having to look three times at the
code.
>
>> + }
>> +
>> + for (i = 0; i < pls->ctx->nb_streams; i++) {
>> + AVStream *ist = pls->ctx->streams[i];
>> + AVStream *st = pls->main_streams[i];
>> + if (ignore_flags) {
>> + av_dict_copy(&st->metadata, ist->metadata, 0);
>> + } else if (ist->event_flags & AVSTREAM_EVENT_FLAG_METADATA_UPDATED) {
>> + av_dict_copy(&st->metadata, ist->metadata, 0);
>> + ist->event_flags &= ~AVSTREAM_EVENT_FLAG_METADATA_UPDATED;
>> + st->event_flags |= AVSTREAM_EVENT_FLAG_METADATA_UPDATED;
>> + }
>> + }
>> +}
>
> Like mentioned in the other patch, av_dict_copy not clearing the target
> dict might be unintended.
The intention is to not remove existing keys in the metadata, but only
update keys that are new or changed. We do also set metadata in
add_stream_to_programs and add_metadata_from_renditions. Presumably we
don't want to delete that. This also seems to be the behavior when we
have updated data from other demuxers or Icy, so I wanted to implement
the same behavior here.
>
>> +
>> /* if timestamp was in valid range: returns 1 and sets seq_no
>> * if not: returns 0 and sets seq_no to closest segment */
>> static int find_timestamp_in_playlist(HLSContext *c, struct playlist *pls,
>> @@ -1960,6 +1989,7 @@ static int hls_read_header(AVFormatContext *s)
>> if (pls->id3_deferred_extra && pls->ctx->nb_streams == 1) {
>> ff_id3v2_parse_apic(pls->ctx, &pls->id3_deferred_extra);
>> avformat_queue_attached_pictures(pls->ctx);
>> + ff_id3v2_parse_priv(pls->ctx, &pls->id3_deferred_extra);
>> ff_id3v2_free_extra_meta(&pls->id3_deferred_extra);
>> pls->id3_deferred_extra = NULL;
>> }
>> @@ -1986,6 +2016,12 @@ static int hls_read_header(AVFormatContext *s)
>> if (ret < 0)
>> goto fail;
>>
>> + /*
>> + * Copy any metadata from playlist to main streams, but do not set
>> + * event flags.
>> + */
>> + update_metadata_from_subdemuxer(pls, 1);
>> +
>
> Possibly would be nicer to drop the ignore_flags parameter, and just
> unset the event flag at the end of read_header (maybe even in generic
> code).
When we are in hls_read_header, the sub-demuxers are not going to set
the event flag, and so I want to unconditionally copy the metadata. If
we're in hls_read_packet, though, I only want to copy metadata if the
subdemuxer says it's been updated. Maybe the better thing to do is
just update the metadata inline here in hls_read_header, since it's
only a few lines of code, and remove the extra parameter and logic
from update_metadata_from_subdemuxer.
>
>> add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_AUDIO);
>> add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_VIDEO);
>> add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_SUBTITLE);
>> @@ -2170,6 +2206,8 @@ static int hls_read_packet(AVFormatContext *s, AVPacket *pkt)
>> return ret;
>> }
>>
>> + update_metadata_from_subdemuxer(pls, 0);
>> +
>> /* check if noheader flag has been cleared by the subdemuxer */
>> if (pls->has_noheader_flag && !(pls->ctx->ctx_flags & AVFMTCTX_NOHEADER)) {
>> pls->has_noheader_flag = 0;
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
More information about the ffmpeg-devel
mailing list