| Message ID | 20180518034942.6480-1-rshaffer@tunein.com |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, 17 May 2018 20:49:42 -0700 rshaffer@tunein.com wrote: > From: Richard Shaffer <rshaffer@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, ×tamp, &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. > > 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. > + } > > add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_AUDIO); > add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_VIDEO);
On Fri, May 18, 2018 at 2:54 AM, wm4 <nfxjfg@googlemail.com> wrote: > On Thu, 17 May 2018 20:49:42 -0700 > rshaffer@tunein.com wrote: > > > From: Richard Shaffer <rshaffer@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, ×tamp, &apic, &extra_meta); > > + ff_id3v2_parse_priv_dict(&metadata, &extra_meta); > > + av_dict_copy(&pls->ctx->metadata, metadata, 0); > > + /* > > + * If we've handled any ID3 metadata here, it's not going to be > seen by the > > + * sub-demuxer. In the case that we got here because of an IO call > during > > + * hls_read_header, this will be cleared. Otherwise, it provides the > > + * necessary hint to hls_read_packet that there is new metadata. > > + */ > > + pls->ctx->event_flags |= AVFMT_EVENT_FLAG_METADATA_UPDATED; > > Can't ID3 tags happen in large quantities with that ID3 timestamp hack > (curse whoever invented it)? Wouldn't that lead to a large number of > redundant events? Not sure though, I don't have the overview. > Yes, that's a good point. If timestamps are in ID3 tags, they'll be at the start of every segment. I can think of several ways to handle that: Option 1: Technically it is updated metadata, so mark it as updated. Leave it up to the API caller to figure out whether they care about it or not. Option 2: Filter out timestamp ID3 frames, and only mark it as updated if some other ID3 tag or frame is present. Option 3: Compare the new metadata with the last seen metadata, and only set the event flag if something besides the timestamp has actually changed. That would filter out false updates for the API consumer, but I'm pretty sure nothing else that handles metadata updates works this way. Any thoughts? Personally I'd lean towards 1 or 2. > > > > > if (timestamp != AV_NOPTS_VALUE) { > > pls->id3_mpegts_timestamp = timestamp; > > @@ -1037,12 +1036,10 @@ static void handle_id3(AVIOContext *pb, struct > playlist *pls) > > /* demuxer not yet opened, defer picture attachment */ > > pls->id3_deferred_extra = extra_meta; > > > > - ff_id3v2_parse_priv_dict(&metadata, &extra_meta); > > - av_dict_copy(&pls->ctx->metadata, metadata, 0); > > pls->id3_initial = metadata; > > > > } else { > > - if (!pls->id3_changed && id3_has_changed_values(pls, metadata, > apic)) { > > + if (!pls->id3_changed && id3_has_changed_values(pls, apic)) { > > avpriv_report_missing_feature(pls->ctx, "Changing ID3 > metadata in HLS audio elementary stream"); > > pls->id3_changed = 1; > > } > > @@ -1939,8 +1936,15 @@ static int hls_read_header(AVFormatContext *s) > > * Copy any metadata from playlist to main streams, but do not > set > > * event flags. > > */ > > - if (pls->n_main_streams) > > + if (pls->n_main_streams) { > > av_dict_copy(&pls->main_streams[0]->metadata, > pls->ctx->metadata, 0); > > + /* > > + * If we've intercepted metadata, we will have set this > event flag. > > + * Clear it to avoid confusion, since otherwise we will > flag it as > > + * new metadata on the next call to hls_read_packet. > > + */ > > + pls->ctx->event_flags = ~AVFMT_EVENT_FLAG_METADATA_UPDATED; > > I think ~(unsigned)AVFMT_EVENT_... would be preferable for maximum > correctness. > That's not the only reason this is wrong. I'll fix it. > > > + } > > > > add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_AUDIO); > > add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_VIDEO); > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Fri, 18 May 2018 11:54:52 -0700 Richard Shaffer <rshaffer@tunein.com> wrote: > On Fri, May 18, 2018 at 2:54 AM, wm4 <nfxjfg@googlemail.com> wrote: > > > On Thu, 17 May 2018 20:49:42 -0700 > > rshaffer@tunein.com wrote: > > > > > From: Richard Shaffer <rshaffer@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, ×tamp, &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.
I just wanted to send a reminder about this patch... wm4 had some concerns about publishing a metadata update on each timestamp (which would essentially be on each segment). I updated it to not set the metadata updated event flag in those cases, although it will still add that metadata to the dictionary. I'm not sure if this is exactly the compromise he had in mind. Any comments on this would be appreciated. Of course, if anyone else has an opinion or can take the time to review this, that would be great, too. Much thanks, -Richard On Thu, May 17, 2018 at 8:49 PM, <rshaffer@tunein.com> wrote: > From: Richard Shaffer <rshaffer@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, ×tamp, &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; > > 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; > + } > > add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_AUDIO); > add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_VIDEO); > -- > 2.15.1 (Apple Git-101) > >
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, ×tamp, &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; 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; + } add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_AUDIO); add_metadata_from_renditions(s, pls, AVMEDIA_TYPE_VIDEO);
From: Richard Shaffer <rshaffer@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(-)