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

[Richard Shaffer]

On Fri, May 18, 2018 at 4:09 PM,  wrote:

> From: Richard Shaffer 
>
> 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.
> ---
>
> Changes in this version:
> * Only set metadata updated flag if we have non-timestamp metadata.
> * Fix clearing of metadata updated flag in hls_read_header.
> * Specifically cast operands to unsigned when using bitwise operators.
>
> (This last one is almost certainly pedantic, but it doesn't hurt anything.)
>
>  libavformat/hls.c | 39 ---
>  1 file changed, 24 insertions(+), 15 deletions(-)
>
> diff --git a/libavformat/hls.c b/libavformat/hls.c
> index 3d4f7f2647..342a022975 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,19 @@ static void handle_id3(AVIOContext *pb, struct
> playlist *pls)
>  int64_t timestamp = AV_NOPTS_VALUE;
>
>  parse_id3(pls->ctx, pb, , , , _meta);
> +ff_id3v2_parse_priv_dict(, _meta);
> +av_dict_copy(>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. Note
> that
> + * we only set the update flag if we have metadata other than a
> timestamp.
> + */
> +if (av_dict_count(metadata) > (timestamp == AV_NOPTS_VALUE ? 0 : 1)) {
> +pls->ctx->event_flags = (unsigned)pls->ctx->event_flags |
> +(unsigned)AVFMT_EVENT_FLAG_METADATA_UPDATED;
> +}
>
>  if (timestamp != AV_NOPTS_VALUE) {
>  pls->id3_mpegts_timestamp = timestamp;
> @@ -1037,12 +1040,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(, _meta);
> -av_dict_copy(>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 +1940,16 @@ 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(>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 = (unsigned)pls->ctx->event_flags &
> +~(unsigned)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)
>
>
Just wanted to ping the list one last time and see if I can get anyone to
take a look at this changeset.

Thanks,

-Richard
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Richard Shaffer]

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, <rshaf...@tunein.com> wrote:

> From: Richard Shaffer <rshaf...@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, , , , _meta);
> +ff_id3v2_parse_priv_dict(, _meta);
> +av_dict_copy(>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(, _meta);
> -av_dict_copy(>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(>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)
>
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[Richard Shaffer]

On Fri, May 18, 2018 at 2:54 AM, wm4 <nfx...@googlemail.com> wrote:

> On Thu, 17 May 2018 20:49:42 -0700
> rshaf...@tunein.com wrote:
>
> > From: Richard Shaffer <rshaf...@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, , , , _meta);
> > +ff_id3v2_parse_priv_dict(, _meta);
> > +av_dict_copy(>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(, _meta);
> > -av_dict_copy(>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)
>

Re: [FFmpeg-devel] [PATCH v2] libavformat/http: Refactor and fix additional leaks in get_cookies.

[Richard Shaffer]

On Thu, Apr 19, 2018 at 1:04 PM, wm4 <nfx...@googlemail.com> wrote:

> On Thu, 19 Apr 2018 12:55:00 -0700
> rshaf...@tunein.com wrote:
>
> > From: Richard Shaffer <rshaf...@tunein.com>
> >
> > This refactors get_cookies to simplify some code paths, specifically for
> > skipping logic in the while loop or exiting it. It also simplifies the
> logic
> > for appending additional values to *cookies by replacing
> strlen/malloc/snprintf
> > with one call av_asnprintf.
> >
> > This refactor fixes a bug where the cookie_params AVDictionary would get
> leaked
> > if we failed to allocate a new buffer for writing to *cookies.
> > ---
> > Updated so that next = set_cookies = av_strdup(s->cookies) assignment is
> done
> > on a separate line instead of inside the if conditional.
> >
> >  libavformat/http.c | 65 +++---
> 
> >  1 file changed, 28 insertions(+), 37 deletions(-)
> >
> > diff --git a/libavformat/http.c b/libavformat/http.c
> > index b4a1919f24..d59ffbbbe8 100644
> > --- a/libavformat/http.c
> > +++ b/libavformat/http.c
> > @@ -1015,7 +1015,8 @@ static int process_line(URLContext *h, char *line,
> int line_count,
> >  /**
> >   * Create a string containing cookie values for use as a HTTP cookie
> header
> >   * field value for a particular path and domain from the cookie values
> stored in
> > - * the HTTP protocol context. The cookie string is stored in *cookies.
> > + * the HTTP protocol context. The cookie string is stored in *cookies,
> and may
> > + * be NULL if there are no valid cookies.
> >   *
> >   * @return a negative value if an error condition occurred, 0 otherwise
> >   */
> > @@ -1025,15 +1026,20 @@ static int get_cookies(HTTPContext *s, char
> **cookies, const char *path,
> >  // cookie strings will look like Set-Cookie header field values.
> Multiple
> >  // Set-Cookie fields will result in multiple values delimited by a
> newline
> >  int ret = 0;
> > -char *cookie, *set_cookies = av_strdup(s->cookies), *next =
> set_cookies;
> > -
> > -if (!set_cookies) return AVERROR(EINVAL);
> > +char *cookie, *set_cookies, *next;
> >
> >  // destroy any cookies in the dictionary.
> >  av_dict_free(>cookie_dict);
> >
> > +if (!s->cookies)
> > +return 0;
> > +
> > +next = set_cookies = av_strdup(s->cookies);
> > +if (!next)
> > +return AVERROR(ENOMEM);
> > +
> >  *cookies = NULL;
> > -while ((cookie = av_strtok(next, "\n", ))) {
> > +while ((cookie = av_strtok(next, "\n", )) && !ret) {
> >  AVDictionary *cookie_params = NULL;
> >  AVDictionaryEntry *cookie_entry, *e;
> >
> > @@ -1043,23 +1049,19 @@ static int get_cookies(HTTPContext *s, char
> **cookies, const char *path,
> >
> >  // continue on to the next cookie if this one cannot be parsed
> >  if (parse_set_cookie(cookie, _params))
> > -continue;
> > +goto skip_cookie;
> >
> >  // if the cookie has no value, skip it
> >  cookie_entry = av_dict_get(cookie_params, "", NULL,
> AV_DICT_IGNORE_SUFFIX);
> > -if (!cookie_entry || !cookie_entry->value) {
> > -av_dict_free(_params);
> > -continue;
> > -}
> > +if (!cookie_entry || !cookie_entry->value)
> > +goto skip_cookie;
> >
> >  // if the cookie has expired, don't add it
> >  if ((e = av_dict_get(cookie_params, "expires", NULL, 0)) &&
> e->value) {
> >  struct tm tm_buf = {0};
> >  if (!parse_set_cookie_expiry_time(e->value, _buf)) {
> > -if (av_timegm(_buf) < av_gettime() / 100) {
> > -av_dict_free(_params);
> > -continue;
> > -}
> > +if (av_timegm(_buf) < av_gettime() / 100)
> > +goto skip_cookie;
> >  }
> >  }
> >
> > @@ -1067,42 +1069,31 @@ static int get_cookies(HTTPContext *s, char
> **cookies, const char *path,
> >  if ((e = av_dict_get(cookie_params, "domain", NULL, 0)) &&
> e->value) {
> >  // find the offset comparison is on the min domain (b.com,
> not a.b.com)
> >  int domain_offset = strlen(domain) - strlen(e->value);
> &g

Re: [FFmpeg-devel] [PATCH] libavformat/http: Refactor and fix additional leaks in get_cookies.

[Richard Shaffer]

On Tue, Apr 17, 2018 at 4:37 PM, <rshaf...@tunein.com> wrote:

> From: Richard Shaffer <rshaf...@tunein.com>
>
> This refactors get_cookies to simplify some code paths, specifically for
> skipping logic in the while loop or exiting it. It also simplifies the
> logic
> for appending additional values to *cookies by replacing
> strlen/malloc/snprintf
> with one call av_asnprintf.
>
> This refactor fixes a bug where the cookie_params AVDictionary would get
> leaked
> if we failed to allocate a new buffer for writing to *cookies.
> ---
>  libavformat/http.c | 64 +++---
> 
>  1 file changed, 27 insertions(+), 37 deletions(-)
>
> diff --git a/libavformat/http.c b/libavformat/http.c
> index b4a1919f24..183214c444 100644
> --- a/libavformat/http.c
> +++ b/libavformat/http.c
> @@ -1015,7 +1015,8 @@ static int process_line(URLContext *h, char *line,
> int line_count,
>  /**
>   * Create a string containing cookie values for use as a HTTP cookie
> header
>   * field value for a particular path and domain from the cookie values
> stored in
> - * the HTTP protocol context. The cookie string is stored in *cookies.
> + * the HTTP protocol context. The cookie string is stored in *cookies,
> and may
> + * be NULL if there are no valid cookies.
>   *
>   * @return a negative value if an error condition occurred, 0 otherwise
>   */
> @@ -1025,15 +1026,19 @@ static int get_cookies(HTTPContext *s, char
> **cookies, const char *path,
>  // cookie strings will look like Set-Cookie header field values.
> Multiple
>  // Set-Cookie fields will result in multiple values delimited by a
> newline
>  int ret = 0;
> -char *cookie, *set_cookies = av_strdup(s->cookies), *next =
> set_cookies;
> -
> -if (!set_cookies) return AVERROR(EINVAL);
> +char *cookie, *set_cookies, *next;
>
>  // destroy any cookies in the dictionary.
>  av_dict_free(>cookie_dict);
>
> +if (!s->cookies)
> +return 0;
> +
> +if (!(next = set_cookies = av_strdup(s->cookies)))
> +return AVERROR(ENOMEM);
> +
>  *cookies = NULL;
> -while ((cookie = av_strtok(next, "\n", ))) {
> +while ((cookie = av_strtok(next, "\n", )) && !ret) {
>  AVDictionary *cookie_params = NULL;
>  AVDictionaryEntry *cookie_entry, *e;
>
> @@ -1043,23 +1048,19 @@ static int get_cookies(HTTPContext *s, char
> **cookies, const char *path,
>
>  // continue on to the next cookie if this one cannot be parsed
>  if (parse_set_cookie(cookie, _params))
> -continue;
> +goto skip_cookie;
>
>  // if the cookie has no value, skip it
>  cookie_entry = av_dict_get(cookie_params, "", NULL,
> AV_DICT_IGNORE_SUFFIX);
> -if (!cookie_entry || !cookie_entry->value) {
> -av_dict_free(_params);
> -continue;
> -}
> +if (!cookie_entry || !cookie_entry->value)
> +goto skip_cookie;
>
>  // if the cookie has expired, don't add it
>  if ((e = av_dict_get(cookie_params, "expires", NULL, 0)) &&
> e->value) {
>  struct tm tm_buf = {0};
>  if (!parse_set_cookie_expiry_time(e->value, _buf)) {
> -if (av_timegm(_buf) < av_gettime() / 100) {
> -av_dict_free(_params);
> -continue;
> -}
> +if (av_timegm(_buf) < av_gettime() / 100)
> +goto skip_cookie;
>  }
>  }
>
> @@ -1067,42 +1068,31 @@ static int get_cookies(HTTPContext *s, char
> **cookies, const char *path,
>  if ((e = av_dict_get(cookie_params, "domain", NULL, 0)) &&
> e->value) {
>  // find the offset comparison is on the min domain (b.com,
> not a.b.com)
>  int domain_offset = strlen(domain) - strlen(e->value);
> -if (domain_offset < 0) {
> -av_dict_free(_params);
> -continue;
> -}
> +if (domain_offset < 0)
> +goto skip_cookie;
>
>  // match the cookie domain
> -if (av_strcasecmp([domain_offset], e->value)) {
> -av_dict_free(_params);
> -continue;
> -}
> +if (av_strcasecmp([domain_offset], e->value))
> +goto skip_cookie;
>  }
>
>  // ensure this cookie matches the path
>  e = av_dict_get(cookie_params, "path", NULL, 0);
&

Re: [FFmpeg-devel] [PATCH] avformat/hls: remove redundant code

[Richard Shaffer]

On Mon, Apr 16, 2018 at 10:24 PM, Steven Liu  wrote:

>
>
> > On 16 Apr 2018, at 08:37, Jun Zhao  wrote:
> >
> >
> >
> > On 2018/4/13 20:29, Steven Liu wrote:
> >> 2018-04-13 16:19 GMT+08:00 Jun Zhao :
> >>>
> >>> On 2018/4/12 16:48, Steven Liu wrote:
>  Signed-off-by: Steven Liu 
>  ---
>  libavformat/hls.c | 27 +--
>  1 file changed, 9 insertions(+), 18 deletions(-)
> 
>  diff --git a/libavformat/hls.c b/libavformat/hls.c
>  index ae0545a086..74f0c2ccc5 100644
>  --- a/libavformat/hls.c
>  +++ b/libavformat/hls.c
>  @@ -945,14 +945,8 @@ static struct segment *next_segment(struct
> playlist *pls)
>  return pls->segments[n];
>  }
> 
>  -enum ReadFromURLMode {
>  -READ_NORMAL,
>  -READ_COMPLETE,
>  -};
>  -
>  static int read_from_url(struct playlist *pls, struct segment *seg,
>  - uint8_t *buf, int buf_size,
>  - enum ReadFromURLMode mode)
>  + uint8_t *buf, int buf_size)
>  {
>  int ret;
> 
>  @@ -960,12 +954,9 @@ static int read_from_url(struct playlist *pls,
> struct segment *seg,
>  if (seg->size >= 0)
>  buf_size = FFMIN(buf_size, seg->size - pls->cur_seg_offset);
> 
>  -if (mode == READ_COMPLETE) {
>  -ret = avio_read(pls->input, buf, buf_size);
>  -if (ret != buf_size)
>  -av_log(NULL, AV_LOG_ERROR, "Could not read complete
> segment.\n");
>  -} else
>  -ret = avio_read(pls->input, buf, buf_size);
>  +ret = avio_read(pls->input, buf, buf_size);
>  +if (ret != buf_size)
>  +av_log(NULL, AV_LOG_ERROR, "Could not read complete
> segment.\n");
> 
>  if (ret > 0)
>  pls->cur_seg_offset += ret;
>  @@ -1085,7 +1076,7 @@ static void intercept_id3(struct playlist *pls,
> uint8_t *buf,
>  while (1) {
>  /* see if we can retrieve enough data for ID3 header */
>  if (*len < ID3v2_HEADER_SIZE && buf_size >=
> ID3v2_HEADER_SIZE) {
>  -bytes = read_from_url(pls, seg, buf + *len,
> ID3v2_HEADER_SIZE - *len, READ_COMPLETE);
>  +bytes = read_from_url(pls, seg, buf + *len,
> ID3v2_HEADER_SIZE - *len);
>  if (bytes > 0) {
> 
>  if (bytes == ID3v2_HEADER_SIZE - *len)
>  @@ -1137,7 +1128,7 @@ static void intercept_id3(struct playlist *pls,
> uint8_t *buf,
> 
>  if (remaining > 0) {
>  /* read the rest of the tag in */
>  -if (read_from_url(pls, seg, pls->id3_buf +
> id3_buf_pos, remaining, READ_COMPLETE) != remaining)
>  +if (read_from_url(pls, seg, pls->id3_buf +
> id3_buf_pos, remaining) != remaining)
>  break;
>  id3_buf_pos += remaining;
>  av_log(pls->ctx, AV_LOG_DEBUG, "Stripped additional
> %d HLS ID3 bytes\n", remaining);
>  @@ -1151,7 +1142,7 @@ static void intercept_id3(struct playlist *pls,
> uint8_t *buf,
> 
>  /* re-fill buffer for the caller unless EOF */
>  if (*len >= 0 && (fill_buf || *len == 0)) {
>  -bytes = read_from_url(pls, seg, buf + *len, buf_size - *len,
> READ_NORMAL);
>  +bytes = read_from_url(pls, seg, buf + *len, buf_size - *len);
> 
>  /* ignore error if we already had some data */
>  if (bytes >= 0)
>  @@ -1311,7 +1302,7 @@ static int update_init_section(struct playlist
> *pls, struct segment *seg)
>  av_fast_malloc(>init_sec_buf, >init_sec_buf_size,
> sec_size);
> 
>  ret = read_from_url(pls, seg->init_section, pls->init_sec_buf,
>  -pls->init_sec_buf_size, READ_COMPLETE);
>  +pls->init_sec_buf_size);
> >>> Didn't care ret < pls->init_sec_buf_size ?
> >> avio_read is full size read, so it will return error, or
> >> init_sec_buf_size, as your question, maybe it will happen then the
> >> read_from_url called avio_read_partiall.
> > Thanks the clarify, I think the patch is Ok now.
> pushed
>  ff_format_io_close(pls->parent, >input);
> 
>  if (ret < 0)
>  @@ -1506,7 +1497,7 @@ reload:
>  }
> 
>  seg = current_segment(v);
>  -ret = read_from_url(v, seg, buf, buf_size, READ_NORMAL);
>  +ret = read_from_url(v, seg, buf, buf_size);
>  if (ret > 0) {
>  if (just_opened && v->is_id3_timestamped != 0) {
>  /* Intercept ID3 tags here, elementary audio streams are
> required
> >>> LGTM except one question.
> >>> ___
> >>> ffmpeg-devel mailing list
> >>> ffmpeg-devel@ffmpeg.org
> >>> 

Re: [FFmpeg-devel] [PATCH] libavformat/http: Fix memory leak in get_cookies.

[Richard Shaffer]

On Tue, Apr 17, 2018 at 1:04 PM, wm4 <nfx...@googlemail.com> wrote:

> On Tue, 17 Apr 2018 10:48:16 -0700
> Richard Shaffer <rshaf...@tunein.com> wrote:
>
> > On Fri, Apr 13, 2018 at 4:42 PM, <rshaf...@tunein.com> wrote:
> >
> > > From: Richard Shaffer <rshaf...@tunein.com>
> > >
> > > ---
> > >  libavformat/http.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/libavformat/http.c b/libavformat/http.c
> > > index 983034f083..b4a1919f24 100644
> > > --- a/libavformat/http.c
> > > +++ b/libavformat/http.c
> > > @@ -1103,6 +1103,7 @@ static int get_cookies(HTTPContext *s, char
> > > **cookies, const char *path,
> > >  snprintf(*cookies, str_size, "%s; %s=%s", tmp,
> > > cookie_entry->key, cookie_entry->value);
> > >  av_free(tmp);
> > >  }
> > > +av_dict_free(_params);
> > >  }
> > >
> > >  av_free(set_cookies);
> > > --
> > > 2.15.1 (Apple Git-101)
> > >
> > > Would one of the maintainers mind looking at and applying this? It's
> a
> > one-line change to fix a memory leak.
>
> Not a maintainer, but the "official" maintainer hasn't cared about this
> code for a long time (despite being active FFmpeg contributor). Pushed.
>
> I think there are some more leaks of this allocation, though.
>

Yeah. Darn. I missed see the break statements inside the nested ifs. Those
are a rare case, since they're only executed if an allocation fails, but
they should still be handled. I'll send another patch for that. Maybe I can
refactor the while loop so that there aren't so many breaks or continues. I
did go over the rest of that loop, and I don't see any other cases where
the dictionary isn't freed.

Thanks for committing anyway. At least it fixes the most common code path.


> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/http: Fix memory leak in get_cookies.

[Richard Shaffer]

On Fri, Apr 13, 2018 at 4:42 PM, <rshaf...@tunein.com> wrote:

> From: Richard Shaffer <rshaf...@tunein.com>
>
> ---
>  libavformat/http.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/libavformat/http.c b/libavformat/http.c
> index 983034f083..b4a1919f24 100644
> --- a/libavformat/http.c
> +++ b/libavformat/http.c
> @@ -1103,6 +1103,7 @@ static int get_cookies(HTTPContext *s, char
> **cookies, const char *path,
>  snprintf(*cookies, str_size, "%s; %s=%s", tmp,
> cookie_entry->key, cookie_entry->value);
>  av_free(tmp);
>  }
> +av_dict_free(_params);
>  }
>
>  av_free(set_cookies);
> --
> 2.15.1 (Apple Git-101)
>
> Would one of the maintainers mind looking at and applying this? It's a
one-line change to fix a memory leak.

Thanks,

-Richard
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH v2] avformat/hls: clean up duplicate option fields

[Richard Shaffer]

On Fri, Apr 13, 2018 at 5:14 PM, <rshaf...@tunein.com> wrote:

> From: Richard Shaffer <rshaf...@tunein.com>
>
> The HLSContext struct contains fields which duplicate the data stored in
> the
> avio_opts field. This change removes those fields in favor of avio_opts,
> and
> updates the code accordingly.
> ---
> The original patch caused the buffer pointed to by new_cookies in open_url
> to be
> leaked. The only thing that buffer is used for is to store the value until
> it
> can be passed to av_dict_set. To fix the leak, v2 of the patch simply calls
> av_dict_set with the AV_DICT_DONT_STRDUP_VAL flag, so that the dictionary
> takes
> ownership of the memory instead of copying it again.
>
>  libavformat/hls.c | 74 +++---
> -
>  1 file changed, 9 insertions(+), 65 deletions(-)
>
> diff --git a/libavformat/hls.c b/libavformat/hls.c
> index 1257cd101c..d6158c0ada 100644
> --- a/libavformat/hls.c
> +++ b/libavformat/hls.c
> @@ -202,11 +202,6 @@ typedef struct HLSContext {
>  int64_t first_timestamp;
>  int64_t cur_timestamp;
>  AVIOInterruptCB *interrupt_callback;
> -char *referer;   ///< holds HTTP referer set as
> an AVOption to the HTTP protocol context
> -char *user_agent;///< holds HTTP user agent set
> as an AVOption to the HTTP protocol context
> -char *cookies;   ///< holds HTTP cookie values
> set in either the initial response or as an AVOption to the HTTP protocol
> context
> -char *headers;   ///< holds HTTP headers set as
> an AVOption to the HTTP protocol context
> -char *http_proxy;///< holds the address of the
> HTTP proxy server
>  AVDictionary *avio_opts;
>  int strict_std_compliance;
>  char *allowed_extensions;
> @@ -267,10 +262,6 @@ static void free_playlist_list(HLSContext *c)
>  av_free(pls);
>  }
>  av_freep(>playlists);
> -av_freep(>cookies);
> -av_freep(>user_agent);
> -av_freep(>headers);
> -av_freep(>http_proxy);
>  c->n_playlists = 0;
>  }
>
> @@ -593,14 +584,6 @@ static int ensure_playlist(HLSContext *c, struct
> playlist **pls, const char *url
>  return 0;
>  }
>
> -static void update_options(char **dest, const char *name, void *src)
> -{
> -av_freep(dest);
> -av_opt_get(src, name, AV_OPT_SEARCH_CHILDREN, (uint8_t**)dest);
> -if (*dest && !strlen(*dest))
> -av_freep(dest);
> -}
> -
>  static int open_url_keepalive(AVFormatContext *s, AVIOContext **pb,
>const char *url)
>  {
> @@ -684,12 +667,8 @@ static int open_url(AVFormatContext *s, AVIOContext
> **pb, const char *url,
>  if (!(s->flags & AVFMT_FLAG_CUSTOM_IO))
>  av_opt_get(*pb, "cookies", AV_OPT_SEARCH_CHILDREN,
> (uint8_t**)_cookies);
>
> -if (new_cookies) {
> -av_free(c->cookies);
> -c->cookies = new_cookies;
> -}
> -
> -av_dict_set(, "cookies", c->cookies, 0);
> +if (new_cookies)
> +av_dict_set(, "cookies", new_cookies,
> AV_DICT_DONT_STRDUP_VAL);
>  }
>
>  av_dict_free();
> @@ -736,14 +715,7 @@ static int parse_playlist(HLSContext *c, const char
> *url,
>
>  if (!in) {
>  AVDictionary *opts = NULL;
> -/* Some HLS servers don't like being sent the range header */
> -av_dict_set(, "seekable", "0", 0);
> -
> -// broker prior HTTP options that should be consistent across
> requests
> -av_dict_set(, "user_agent", c->user_agent, 0);
> -av_dict_set(, "cookies", c->cookies, 0);
> -av_dict_set(, "headers", c->headers, 0);
> -av_dict_set(, "http_proxy", c->http_proxy, 0);
> +av_dict_copy(, c->avio_opts, 0);
>
>  if (c->http_persistent)
>  av_dict_set(, "multiple_requests", "1", 0);
> @@ -1169,14 +1141,6 @@ static int open_input(HLSContext *c, struct
> playlist *pls, struct segment *seg,
>  int ret;
>  int is_http = 0;
>
> -// broker prior HTTP options that should be consistent across requests
> -av_dict_set(, "user_agent", c->user_agent, 0);
> -av_dict_set(, "referer", c->referer, 0);
> -av_dict_set(, "cookies", c->cookies, 0);
> -av_dict_set(, "headers", c->headers, 0);
> -av_dict_set(, "http_proxy", c->http_proxy, 0);
> -a

Re: [FFmpeg-devel] [PATCH] libavformat/aac: Parse all ID3 tags present between ADTS frames

[Richard Shaffer]

On Mon, Apr 9, 2018 at 1:05 AM, Mattias Amnefelt  wrote:

> On 2018-04-05 01:00, Mattias Amnefelt wrote:
>
>> On 2018-04-04 09:22, Mattias Amnefelt wrote:
>>
>>> On 2018-04-04 03:42, James Almer wrote:
>>>
 On 4/3/2018 10:40 PM, Carl Eugen Hoyos wrote:

> 2018-04-04 3:38 GMT+02:00, James Almer :
>
>> On 4/3/2018 10:33 PM, Carl Eugen Hoyos wrote:
>>
>>> The "-f aac" looks like a bad idea to me.
>>> It's also true for the tests above, but that's still not reason to
>>> add more.
>>>
>>> Please avoid top-posting here, Carl Eugen
>>>
>> At least in one of them it was added because the sample had too few
>> frames and probing was detecting it with a score of 1, which seemed
>> too
>> fragile.
>>
> I believe that it is good to have a sample that is detected with
> a small score as part of fate.
>
> Carl Eugen
>
 When i asked it was suggested to just force the demuxer. I have no
 opinion one way or another, so feel free to change it.

>>> I have to admit I just copy-n-pasted the test above. I just
>>> double-checked and all the id3 tag tests pass without -f aac now. I'm not
>>> sure if anything has changed since the test was added or not. Do you want a
>>> patch which removes it for all of them?
>>>
>>>
>>> Here's an updated version of the patch without -f aac
>>
>> /mattiasa
>>
>
> Did anyone have any options on the updated patch?
>
>
> /mattiasa
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

This seems like a nice thing to have. If there are no other comments on
Mattias' patch or test, would one of the maintainers be willing to merge it?

-Richard
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/hls: copy rw_timeout from parent to child AVIOContexts.

[Richard Shaffer]

On Tue, Apr 10, 2018 at 4:00 PM, Liu Steven <l...@chinaffmpeg.org> wrote:

>
> > 在 2018年4月11日，上午3:14，Richard Shaffer <rshaf...@tunein.com> 写道：
> >
> > On Tue, Apr 3, 2018 at 5:11 PM, Steven Liu <lingjiujia...@gmail.com>
> wrote:
> >
> >> 2018-04-04 7:53 GMT+08:00 Steven Liu <lingjiujia...@gmail.com>:
> >>>>>
> >>>>> Look at the code:
> >>>>>
> >>>>> 205 char *referer;   ///< holds HTTP referer
> >> set
> >>>>> as an AVOption to the HTTP protocol context
> >>>>> 206 char *user_agent;///< holds HTTP user
> >> agent
> >>>>> set as an AVOption to the HTTP protocol context
> >>>>> 207 char *cookies;   ///< holds HTTP cookie
> >>>>> values set in either the initial response or as an AVOption to the
> HTTP
> >>>>> protocol context
> >>>>> 208 char *headers;   ///< holds HTTP headers
> >> set
> >>>>> as an AVOption to the HTTP protocol context
> >>>>> 209 char *http_proxy;///< holds the address
> of
> >>>>> the HTTP proxy server
> >>>>>
> >>>>> There have some comment for the options.
> >>>>> and reference the code in: hls_read_header / open_input and use the
> >>>>> options.
> >>>>>
> >>>>>
> >>>> That's pretty clear. What I was asking is why the options are stored
> >> both
> >>>> in these fields as well as avio_opts, and this doesn't answer my
> >> question.
> >>>> I was also asking why you object to storing the timeout option in
> >>>> avio_opts, but I'm not understanding the logic in your responses.
> >>>
> >>> no logic problem, just that option be used to HTTP, is that ok?
> >>
> >
> > This is a logic problem for me, because I'm not understanding your logic.
> > What is the reason that avio_opts should only be used for HTTP options?
> >
> >
> >>>
> >>> BTW, how should i reproduce the problem you said?
> >>
> >
> > If you pass the rw_timeout option to either the command-line or to
> > avformat_open_input, it will be applied only to the URL passed on the
> > command line or to the function. When the HLS demuxer tries to open other
> > URLs, such as keying URIs, media playlists or segments, it will not apply
> > the rw_timeout option. As a result, if we fail to receive data from the
> > remote server, it can block the process instead of returning the expected
> > AVERROR(ETIMEDOUT) error.
> >
> >
> >>>
> >>> "
> >>> The rw_timeout option is currently not applied when opening media
> >> playlist,
> >>> segment, or encryption key URLs. This can cause the HLS demuxer to
> block
> >>> indefinitely, even when the rw_timeout option has been specified. This
> >> change
> >>> simply enables carrying over the rw_timeout option when the demuxer
> >> opens these
> >>> URLs.
> >>> "
> >>
> >> So i think add option timeout to do it maybe better than this way. you
> >> can find another formats do it like this.
> >>
> >
> > I think the HLS demuxer is pretty unique. Except for dash, I don't know
> > which other demuxers have to open additional avio to do their work. If
> > there is an example you can show me that illustrates your point, that
> would
> > be appreciated.
> >
> >
> >>>
> >>>>
> >>>>
> >> ___
> >> ffmpeg-devel mailing list
> >> ffmpeg-devel@ffmpeg.org
> >> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> >>
> >
> > Hi Steven,
> >
> > I apologize for the late response. I have been traveling.
> >
> > I understand that you might be busy, and that English is not everyone's
> > first language. However, if you could take the time to give a more
> thorough
> > response, I'd really appreciate it. It's a little bit frustrating for me:
> > I'm asking why avio_opts needs to be restricted to HTTP options, and the
> > answer I seem to keep getting back is "because it's for HTTP options."
> That
> > answer isn't very satisfying, and doesn't help me understand /why/ you
> > think it shoul

Re: [FFmpeg-devel] [PATCH] avformat/hls: copy rw_timeout from parent to child AVIOContexts.

[Richard Shaffer]

On Tue, Apr 3, 2018 at 5:11 PM, Steven Liu  wrote:

> 2018-04-04 7:53 GMT+08:00 Steven Liu :
> >>>
> >>> Look at the code:
> >>>
> >>>  205 char *referer;   ///< holds HTTP referer
> set
> >>> as an AVOption to the HTTP protocol context
> >>>  206 char *user_agent;///< holds HTTP user
> agent
> >>> set as an AVOption to the HTTP protocol context
> >>>  207 char *cookies;   ///< holds HTTP cookie
> >>> values set in either the initial response or as an AVOption to the HTTP
> >>> protocol context
> >>>  208 char *headers;   ///< holds HTTP headers
> set
> >>> as an AVOption to the HTTP protocol context
> >>>  209 char *http_proxy;///< holds the address of
> >>> the HTTP proxy server
> >>>
> >>> There have some comment for the options.
> >>> and reference the code in: hls_read_header / open_input and use the
> >>> options.
> >>>
> >>>
> >> That's pretty clear. What I was asking is why the options are stored
> both
> >> in these fields as well as avio_opts, and this doesn't answer my
> question.
> >> I was also asking why you object to storing the timeout option in
> >> avio_opts, but I'm not understanding the logic in your responses.
> >
> > no logic problem, just that option be used to HTTP, is that ok?
>

This is a logic problem for me, because I'm not understanding your logic.
What is the reason that avio_opts should only be used for HTTP options?


> >
> > BTW, how should i reproduce the problem you said?
>

If you pass the rw_timeout option to either the command-line or to
avformat_open_input, it will be applied only to the URL passed on the
command line or to the function. When the HLS demuxer tries to open other
URLs, such as keying URIs, media playlists or segments, it will not apply
the rw_timeout option. As a result, if we fail to receive data from the
remote server, it can block the process instead of returning the expected
AVERROR(ETIMEDOUT) error.


> >
> > "
> > The rw_timeout option is currently not applied when opening media
> playlist,
> > segment, or encryption key URLs. This can cause the HLS demuxer to block
> > indefinitely, even when the rw_timeout option has been specified. This
> change
> > simply enables carrying over the rw_timeout option when the demuxer
> opens these
> > URLs.
> > "
>
> So i think add option timeout to do it maybe better than this way. you
> can find another formats do it like this.
>

I think the HLS demuxer is pretty unique. Except for dash, I don't know
which other demuxers have to open additional avio to do their work. If
there is an example you can show me that illustrates your point, that would
be appreciated.


> >
> >>
> >>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>

Hi Steven,

I apologize for the late response. I have been traveling.

I understand that you might be busy, and that English is not everyone's
first language. However, if you could take the time to give a more thorough
response, I'd really appreciate it. It's a little bit frustrating for me:
I'm asking why avio_opts needs to be restricted to HTTP options, and the
answer I seem to keep getting back is "because it's for HTTP options." That
answer isn't very satisfying, and doesn't help me understand /why/ you
think it should only be used for HTTP options. It may be that you have a
good reason, but if I can't understand what it is, I'm not going to be able
to provide a better fix.

I can't force anyone to accept my changes or provide more detailed
feedback. However, if we can't understand each other, then at some point I
will have to give up. I'll apply a patch that fixes my problem to a local
fork of ffmpeg. Other people outside of my company won't benefit from the
fix. My company will also have to maintain our own copy of ffmpeg. This
could make it less likely for us to share our changes with the community,
and it will also make it harder for us to benefit from improvements and
fixes to ffmpeg. I don't want to do that, because nobody wins in that
scenario. That is why I'm asking for your help to understand your objection
and suggestion.

Thanks,

-Richard
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/hls: copy rw_timeout from parent to child AVIOContexts.

[Richard Shaffer]

On Tue, Apr 3, 2018 at 12:13 AM, Steven Liu <l...@chinaffmpeg.org> wrote:

>
>
> > On 3 Apr 2018, at 14:06, Richard Shaffer <rshaf...@tunein.com> wrote:
> >
> > On Mon, Apr 2, 2018 at 10:15 PM, Steven Liu <l...@chinaffmpeg.org> wrote:
> >>
> >>
> >>> On 3 Apr 2018, at 12:33, Richard Shaffer <rshaf...@tunein.com> wrote:
> >>>
> >>> On Mon, Apr 2, 2018 at 8:31 PM, Steven Liu <l...@chinaffmpeg.org> wrote:
> >>>>
> >>>>
> >>>>> On 3 Apr 2018, at 09:12, rshaf...@tunein.com wrote:
> >>>>>
> >>>>> From: Richard Shaffer <rshaf...@tunein.com>
> >>>>>
> >>>>> The rw_timeout option is currently not applied when opening media
> playlist,
> >>>>> segment, or encryption key URLs. This can cause the HLS demuxer to
> block
> >>>>> indefinitely, even when the rw_timeout option has been specified.
> This change
> >>>>> simply enables carrying over the rw_timeout option when the demuxer
> opens these
> >>>>> URLs.
> >>>>> ---
> >>>>> libavformat/hls.c | 2 +-
> >>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/libavformat/hls.c b/libavformat/hls.c
> >>>>> index c578bf86e3..6663244ddf 100644
> >>>>> --- a/libavformat/hls.c
> >>>>> +++ b/libavformat/hls.c
> >>>>> @@ -1661,7 +1661,7 @@ static int save_avio_options(AVFormatContext
> *s)
> >>>>> {
> >>>>>   HLSContext *c = s->priv_data;
> >>>>>   static const char * const opts[] = {
> >>>>> -"headers", "http_proxy", "user_agent", "user-agent",
> "cookies", "referer", NULL };
> >>>>> +"headers", "http_proxy", "user_agent", "user-agent",
> "cookies", "referer", "rw_timeout", NULL };
> >>>> This table is used for http header.
> >>>> You could add the option into hls_options.
> >>>
> >>> Thanks for looking at the change. While the options currently in the
> >>> table are related to HTTP and rw_timeout is more general, I'm not
> >>> aware of a reason not to preserve the rw_timeout option here as well.
> >>> It seems unnecessary to define another HLS-specific option for
> >>> rw_timeout when the existing option exists and does what is intended.
> >>> I'm not sure whether you're objecting to the change and/or have a
> >>> different suggestion. Do you mind elaborating on your comment?
> >> Is the rw_timeout in to HTTP RFC? If yes, this is ok, If not, i think
> that is a ffmpeg option , not a http header content.
> >
> > http_proxy isn't tied to an HTTP header value, either. There isn't any
> > indication that avio_opts is intended for specifically HTTP options,
> > or that using it to store other avio options would break something. If
> > you have a reason why this shouldn't be used for other (non-HTTP)
> > options, can you please help me understand your thinking?
> >
> > I also want to spend some more time working on this, because I see
> > that there are multiple fields in HLSContext dealing with avio
> > options: There is the avio_opts field, but also referer, user_agent,
> > cookies, headers and http_proxy. avio_opts seems to be used when
> > opening segments and encryption key URIs, but the other fields are
> > used when reloading a playlist or parsing variant playlist URLs from a
> > master playlist. It's not clear why the options are stored in separate
> > places or whether that is necessary. If you have any insight into that
> > as well, it would be appreciated.
>
> Look at the code:
>
>  205 char *referer;   ///< holds HTTP referer set
> as an AVOption to the HTTP protocol context
>  206 char *user_agent;///< holds HTTP user agent
> set as an AVOption to the HTTP protocol context
>  207 char *cookies;   ///< holds HTTP cookie
> values set in either the initial response or as an AVOption to the HTTP
> protocol context
>  208 char *headers;   ///< holds HTTP headers set
> as an AVOption to the HTTP protocol context
>  209 char *http_proxy;///< holds the address of
> the HTTP proxy server
>
&

Re: [FFmpeg-devel] [PATCH] avformat/hls: copy rw_timeout from parent to child AVIOContexts.

[Richard Shaffer]

On Mon, Apr 2, 2018 at 10:15 PM, Steven Liu <l...@chinaffmpeg.org> wrote:
>
>
>> On 3 Apr 2018, at 12:33, Richard Shaffer <rshaf...@tunein.com> wrote:
>>
>> On Mon, Apr 2, 2018 at 8:31 PM, Steven Liu <l...@chinaffmpeg.org> wrote:
>>>
>>>
>>>> On 3 Apr 2018, at 09:12, rshaf...@tunein.com wrote:
>>>>
>>>> From: Richard Shaffer <rshaf...@tunein.com>
>>>>
>>>> The rw_timeout option is currently not applied when opening media playlist,
>>>> segment, or encryption key URLs. This can cause the HLS demuxer to block
>>>> indefinitely, even when the rw_timeout option has been specified. This 
>>>> change
>>>> simply enables carrying over the rw_timeout option when the demuxer opens 
>>>> these
>>>> URLs.
>>>> ---
>>>> libavformat/hls.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/libavformat/hls.c b/libavformat/hls.c
>>>> index c578bf86e3..6663244ddf 100644
>>>> --- a/libavformat/hls.c
>>>> +++ b/libavformat/hls.c
>>>> @@ -1661,7 +1661,7 @@ static int save_avio_options(AVFormatContext *s)
>>>> {
>>>>HLSContext *c = s->priv_data;
>>>>static const char * const opts[] = {
>>>> -"headers", "http_proxy", "user_agent", "user-agent", "cookies", 
>>>> "referer", NULL };
>>>> +"headers", "http_proxy", "user_agent", "user-agent", "cookies", 
>>>> "referer", "rw_timeout", NULL };
>>> This table is used for http header.
>>> You could add the option into hls_options.
>>
>> Thanks for looking at the change. While the options currently in the
>> table are related to HTTP and rw_timeout is more general, I'm not
>> aware of a reason not to preserve the rw_timeout option here as well.
>> It seems unnecessary to define another HLS-specific option for
>> rw_timeout when the existing option exists and does what is intended.
>> I'm not sure whether you're objecting to the change and/or have a
>> different suggestion. Do you mind elaborating on your comment?
> Is the rw_timeout in to HTTP RFC? If yes, this is ok, If not, i think that is 
> a ffmpeg option , not a http header content.

http_proxy isn't tied to an HTTP header value, either. There isn't any
indication that avio_opts is intended for specifically HTTP options,
or that using it to store other avio options would break something. If
you have a reason why this shouldn't be used for other (non-HTTP)
options, can you please help me understand your thinking?

I also want to spend some more time working on this, because I see
that there are multiple fields in HLSContext dealing with avio
options: There is the avio_opts field, but also referer, user_agent,
cookies, headers and http_proxy. avio_opts seems to be used when
opening segments and encryption key URIs, but the other fields are
used when reloading a playlist or parsing variant playlist URLs from a
master playlist. It's not clear why the options are stored in separate
places or whether that is necessary. If you have any insight into that
as well, it would be appreciated.

>>
>>>>const char * const * opt = opts;
>>>>uint8_t *buf;
>>>>int ret = 0;
>>>> --
>>>> 2.15.1 (Apple Git-101)
>>>>
>>>> ___
>>>> ffmpeg-devel mailing list
>>>> ffmpeg-devel@ffmpeg.org
>>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> Thanks
>>> Steven
>>>
>>>
>>>
>>>
>>>
>> ___
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> Thanks
> Steven
>
>
>
>
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat/hls: copy rw_timeout from parent to child AVIOContexts.

[Richard Shaffer]

On Mon, Apr 2, 2018 at 8:31 PM, Steven Liu <l...@chinaffmpeg.org> wrote:
>
>
>> On 3 Apr 2018, at 09:12, rshaf...@tunein.com wrote:
>>
>> From: Richard Shaffer <rshaf...@tunein.com>
>>
>> The rw_timeout option is currently not applied when opening media playlist,
>> segment, or encryption key URLs. This can cause the HLS demuxer to block
>> indefinitely, even when the rw_timeout option has been specified. This change
>> simply enables carrying over the rw_timeout option when the demuxer opens 
>> these
>> URLs.
>> ---
>> libavformat/hls.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavformat/hls.c b/libavformat/hls.c
>> index c578bf86e3..6663244ddf 100644
>> --- a/libavformat/hls.c
>> +++ b/libavformat/hls.c
>> @@ -1661,7 +1661,7 @@ static int save_avio_options(AVFormatContext *s)
>> {
>> HLSContext *c = s->priv_data;
>> static const char * const opts[] = {
>> -"headers", "http_proxy", "user_agent", "user-agent", "cookies", 
>> "referer", NULL };
>> +"headers", "http_proxy", "user_agent", "user-agent", "cookies", 
>> "referer", "rw_timeout", NULL };
> This table is used for http header.
> You could add the option into hls_options.

Thanks for looking at the change. While the options currently in the
table are related to HTTP and rw_timeout is more general, I'm not
aware of a reason not to preserve the rw_timeout option here as well.
It seems unnecessary to define another HLS-specific option for
rw_timeout when the existing option exists and does what is intended.
I'm not sure whether you're objecting to the change and/or have a
different suggestion. Do you mind elaborating on your comment?

>> const char * const * opt = opts;
>> uint8_t *buf;
>> int ret = 0;
>> --
>> 2.15.1 (Apple Git-101)
>>
>> ___
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> Thanks
> Steven
>
>
>
>
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/aac: Parse all ID3 tags present between ADTS frames

[Richard Shaffer]

It seems like some software can insert an additional ID3v2 tag instead
of appending a frame to an existing one. One could argue that that is
somewhat broken, but I agree it's better to handle it instead of
returning an error. The changes in aacdec.c look ok to me.

The fate sample you provided has two tags at the beginning of the
file. These will actually get parsed by avformat_open_input calling
ff_id3v2_read_dict and not by the adts demuxer. The ff_id3v2_read_dict
code path will already parse multiple tags, which is why the test
passes. In order to exercise your code change, the ID3v2 tags would
have to be between ADTS frames.

-Richard

On Mon, Apr 2, 2018 at 4:35 AM, Mattias Amnefelt  wrote:
> This is a follow-up to https://patchwork.ffmpeg.org/patch/7477/ and changes
> so all ID3 tags between ADTS frames gets parsed, not just the first one.
>
> Sample media for the included fate test is available at
> http://mattias.amnefe.lt/tmp/id3v2_two_tags.aac
>
>
>
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/aac: Parse ID3 tags between ADTS frames.

[Richard Shaffer]

There was only one version of this one. Thanks for applying.

On Mon, Feb 12, 2018 at 1:17 PM, wm4 <nfx...@googlemail.com> wrote:
> On Thu,  1 Feb 2018 18:37:45 -0800
> rshaf...@tunein.com wrote:
>
>> From: Richard Shaffer <rshaf...@tunein.com>
>>
>> While rare, ID3 tags may be inserted between ADTS frames. This change enables
>> parsing them and setting the appropriate metadata updated event flag.
>> ---
>> I have encountered a streaming provider that I must support which does this.
>> There are indications that it isn't totally off the wall, and that it does
>> happen in the wild:
>> * https://www.w3.org/TR/mse-byte-stream-format-mpeg-audio/
>>   (See specifically sections 3 and 4.)
>> * https://github.com/video-dev/hls.js/issues/508
>>
>> That being said, the providers that do this are in the minority. It seems 
>> most
>> streaming providers will do one of the following:
>>  1. If using .aac segments inside of HLS, use the #EXTINF for text metadata 
>> and
>> use an ID3 tag at the head of the segment for things like timing 
>> metadata.
>>  2. If streaming raw AAC over HTTP, use Icy metadata.
>>
>> Aside from comments on the code, I'd be interested in any opinion about 
>> whether
>> this is something that should or should not be supported in libavformat.
>>
>> Thanks,
>>
>> -Richard
>>
>>  libavformat/aacdec.c | 45 +++--
>>  1 file changed, 43 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavformat/aacdec.c b/libavformat/aacdec.c
>> index 36d558ff54..5ec706bdc7 100644
>> --- a/libavformat/aacdec.c
>> +++ b/libavformat/aacdec.c
>> @@ -22,8 +22,10 @@
>>
>>  #include "libavutil/intreadwrite.h"
>>  #include "avformat.h"
>> +#include "avio_internal.h"
>>  #include "internal.h"
>>  #include "id3v1.h"
>> +#include "id3v2.h"
>>  #include "apetag.h"
>>
>>  #define ADTS_HEADER_SIZE 7
>> @@ -116,13 +118,52 @@ static int adts_aac_read_header(AVFormatContext *s)
>>  return 0;
>>  }
>>
>> +static int handle_id3(AVFormatContext *s, AVPacket *pkt)
>> +{
>> +AVDictionary *metadata = NULL;
>> +AVIOContext ioctx;
>> +ID3v2ExtraMeta *id3v2_extra_meta = NULL;
>> +int ret;
>> +
>> +ret = av_append_packet(s->pb, pkt, ff_id3v2_tag_len(pkt->data) - 
>> pkt->size);
>> +if (ret < 0) {
>> +av_packet_unref(pkt);
>> +return ret;
>> +}
>> +
>> +ffio_init_context(, pkt->data, pkt->size, 0, NULL, NULL, NULL, 
>> NULL);
>> +ff_id3v2_read_dict(, , ID3v2_DEFAULT_MAGIC, 
>> _extra_meta);
>> +if ((ret = ff_id3v2_parse_priv_dict(, _extra_meta)) < 0)
>> +goto error;
>> +
>> +if (metadata) {
>> +if ((ret = av_dict_copy(>metadata, metadata, 0)) < 0)
>> +goto error;
>> +s->event_flags |= AVFMT_EVENT_FLAG_METADATA_UPDATED;
>> +}
>> +
>> +error:
>> +av_packet_unref(pkt);
>> +ff_id3v2_free_extra_meta(_extra_meta);
>> +av_dict_free();
>> +
>> +return ret;
>> +}
>> +
>>  static int adts_aac_read_packet(AVFormatContext *s, AVPacket *pkt)
>>  {
>>  int ret, fsize;
>>
>> -ret = av_get_packet(s->pb, pkt, ADTS_HEADER_SIZE);
>> +ret = av_get_packet(s->pb, pkt, FFMAX(ID3v2_HEADER_SIZE, 
>> ADTS_HEADER_SIZE));
>> +
>> +if (ret >= ID3v2_HEADER_SIZE && ff_id3v2_match(pkt->data, 
>> ID3v2_DEFAULT_MAGIC)) {
>> +if ((ret = handle_id3(s, pkt)) >= 0)
>> +ret = av_get_packet(s->pb, pkt, ADTS_HEADER_SIZE);
>> +}
>> +
>>  if (ret < 0)
>>  return ret;
>> +
>>  if (ret < ADTS_HEADER_SIZE) {
>>  av_packet_unref(pkt);
>>  return AVERROR(EIO);
>> @@ -139,7 +180,7 @@ static int adts_aac_read_packet(AVFormatContext *s, 
>> AVPacket *pkt)
>>  return AVERROR_INVALIDDATA;
>>  }
>>
>> -ret = av_append_packet(s->pb, pkt, fsize - ADTS_HEADER_SIZE);
>> +ret = av_append_packet(s->pb, pkt, fsize - pkt->size);
>>  if (ret < 0)
>>  av_packet_unref(pkt);
>>
>
> Applied. (I hope this is the most recent version...)
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/aac: Parse ID3 tags between ADTS frames.

[Richard Shaffer]

On Fri, Feb 9, 2018 at 1:21 PM, wm4 <nfx...@googlemail.com> wrote:
> On Fri, 9 Feb 2018 12:54:29 -0800
> Richard Shaffer <rshaf...@tunein.com> wrote:
>
>> I just wanted to send a final ping about this patch. While most
>> streams will not encounter the case that it addresses, those that do
>> will have a pretty bad experience. The demuxer's read_packet function
>> currently only reads 7 bytes (ADTS_HEADER_SIZE) of input. If the first
>> 12 bits aren't the sync word, it immediately gives up and returns
>> AVERROR_INVALIDDATA. This means that if it encounters an ID3 tag, it
>> won't re-sync unless the tag length just happens to be a multiple of 7
>> bytes.
>>
>> I'm assuming that the demuxer should be enhanced to search input for
>> the next sync word. (That might be some of the additional work that
>> James mentioned, and is something I'd be interested in working on.) In
>> the mean time, I think this patch addresses a likely case of lost
>> synchronization, while also providing some utility to API users.
>>
>> I know that people are probably busy, so I don't want to be annoying
>> or overly persistent. However, if anyone has a little time to review
>> this and provide feedback or accept the changes, I'd appreciate it.
>
> I think I forgot about this patch. I can apply it on Monday or so,
> unless someone else has comments. James Almer wanted a test - what's
> the status on this and does anyone demand that this is pushed only with
> such a test added?

I was also able to provide a patch and sample file to add a fate test.

Patch: https://patchwork.ffmpeg.org/patch/7504/
Sample: http://ffmpeg.org/pipermail/ffmpeg-devel/2018-February/224906.html

I think I sent that out late on a Saturday, so it probably got
overlooked in the shuffle.

-Richard


> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/aac: Parse ID3 tags between ADTS frames.

[Richard Shaffer]

I just wanted to send a final ping about this patch. While most
streams will not encounter the case that it addresses, those that do
will have a pretty bad experience. The demuxer's read_packet function
currently only reads 7 bytes (ADTS_HEADER_SIZE) of input. If the first
12 bits aren't the sync word, it immediately gives up and returns
AVERROR_INVALIDDATA. This means that if it encounters an ID3 tag, it
won't re-sync unless the tag length just happens to be a multiple of 7
bytes.

I'm assuming that the demuxer should be enhanced to search input for
the next sync word. (That might be some of the additional work that
James mentioned, and is something I'd be interested in working on.) In
the mean time, I think this patch addresses a likely case of lost
synchronization, while also providing some utility to API users.

I know that people are probably busy, so I don't want to be annoying
or overly persistent. However, if anyone has a little time to review
this and provide feedback or accept the changes, I'd appreciate it.

Thanks,

-Richard


On Wed, Feb 7, 2018 at 9:40 AM, Richard Shaffer <rshaf...@tunein.com> wrote:
> I did send the sample file and a unit test for this. If there are no
> issues or other comments, would you guys be willing to merge it?
>
> -Richard
>
> On Fri, Feb 2, 2018 at 11:15 AM, Richard Shaffer <rshaf...@tunein.com> wrote:
>> On Fri, Feb 2, 2018 at 5:52 AM, James Almer <jamr...@gmail.com> wrote:
>>> On 2/1/2018 11:37 PM, rshaf...@tunein.com wrote:
>>>> From: Richard Shaffer <rshaf...@tunein.com>
>>>>
>>>> While rare, ID3 tags may be inserted between ADTS frames. This change 
>>>> enables
>>>> parsing them and setting the appropriate metadata updated event flag.
>>>> ---
>>>> I have encountered a streaming provider that I must support which does 
>>>> this.
>>>> There are indications that it isn't totally off the wall, and that it does
>>>> happen in the wild:
>>>> * https://www.w3.org/TR/mse-byte-stream-format-mpeg-audio/
>>>>   (See specifically sections 3 and 4.)
>>>> * https://github.com/video-dev/hls.js/issues/508
>>>>
>>>> That being said, the providers that do this are in the minority. It seems 
>>>> most
>>>> streaming providers will do one of the following:
>>>>  1. If using .aac segments inside of HLS, use the #EXTINF for text 
>>>> metadata and
>>>> use an ID3 tag at the head of the segment for things like timing 
>>>> metadata.
>>>>  2. If streaming raw AAC over HTTP, use Icy metadata.
>>>>
>>>> Aside from comments on the code, I'd be interested in any opinion about 
>>>> whether
>>>> this is something that should or should not be supported in libavformat.
>>>>
>>>> Thanks,
>>>>
>>>> -Richard
>>>
>>> Can you provide a sample and add a fate test for this? To avoid
>>> regressions in the future. This demuxer still needs some work.
>>>
>>> Use the new probetags() function you added in your other id3v2 test if
>>> the tags between samples in the file are the only ones, or if they
>>> differ in some way from the ones at the beginning of the file, so
>>> ffprobe will report them instead.
>>> Alternatively, see fate-adts-demux in tests/fate/demux.mak
>>
>> Oh, somebody did merge that test. How cool.
>>
>> I don't think we can test this with ffprobe, though. It will just read
>> tags at the head of the file and not in the middle.
>>
>> I have at least one sample that's ok for me to share. I'll work on a
>> separate patch to add a fate test.
>>
>> -Richard
>>
>>> ___
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/aac: Parse ID3 tags between ADTS frames.

[Richard Shaffer]

I did send the sample file and a unit test for this. If there are no
issues or other comments, would you guys be willing to merge it?

-Richard

On Fri, Feb 2, 2018 at 11:15 AM, Richard Shaffer <rshaf...@tunein.com> wrote:
> On Fri, Feb 2, 2018 at 5:52 AM, James Almer <jamr...@gmail.com> wrote:
>> On 2/1/2018 11:37 PM, rshaf...@tunein.com wrote:
>>> From: Richard Shaffer <rshaf...@tunein.com>
>>>
>>> While rare, ID3 tags may be inserted between ADTS frames. This change 
>>> enables
>>> parsing them and setting the appropriate metadata updated event flag.
>>> ---
>>> I have encountered a streaming provider that I must support which does this.
>>> There are indications that it isn't totally off the wall, and that it does
>>> happen in the wild:
>>> * https://www.w3.org/TR/mse-byte-stream-format-mpeg-audio/
>>>   (See specifically sections 3 and 4.)
>>> * https://github.com/video-dev/hls.js/issues/508
>>>
>>> That being said, the providers that do this are in the minority. It seems 
>>> most
>>> streaming providers will do one of the following:
>>>  1. If using .aac segments inside of HLS, use the #EXTINF for text metadata 
>>> and
>>> use an ID3 tag at the head of the segment for things like timing 
>>> metadata.
>>>  2. If streaming raw AAC over HTTP, use Icy metadata.
>>>
>>> Aside from comments on the code, I'd be interested in any opinion about 
>>> whether
>>> this is something that should or should not be supported in libavformat.
>>>
>>> Thanks,
>>>
>>> -Richard
>>
>> Can you provide a sample and add a fate test for this? To avoid
>> regressions in the future. This demuxer still needs some work.
>>
>> Use the new probetags() function you added in your other id3v2 test if
>> the tags between samples in the file are the only ones, or if they
>> differ in some way from the ones at the beginning of the file, so
>> ffprobe will report them instead.
>> Alternatively, see fate-adts-demux in tests/fate/demux.mak
>
> Oh, somebody did merge that test. How cool.
>
> I don't think we can test this with ffprobe, though. It will just read
> tags at the head of the file and not in the middle.
>
> I have at least one sample that's ok for me to share. I'll work on a
> separate patch to add a fate test.
>
> -Richard
>
>> ___
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] fate: add aac id3v2 demux test

[Richard Shaffer]

Attaching sample file for the test.

-Richard

On Sat, Feb 3, 2018 at 11:24 PM,  <rshaf...@tunein.com> wrote:
> From: Richard Shaffer <rshaf...@tunein.com>
>
> A basic test for demuxing raw AAC (ADTS) with ID3v2 tags.
> ---
> This is related to the patch 'libavformat/aac: Parse ID3 tags between ADTS
> frames', and will fail without it. The test file contains an ID3 tag at the
> beginning as well as a second tag between frames. While the test doesn't check
> that the tags' data have been parsed correctly, without the aforementioned
> patch, the demuxing will fail on the second tag and return an invalid data
> error.
>
> The sample will be attached in a separate email.
>
> Thanks,
>
> -Richard
>
>  tests/fate/demux.mak|   3 +-
>  tests/ref/fate/adts-id3v2-demux | 240 
> 
>  2 files changed, 242 insertions(+), 1 deletion(-)
>  create mode 100644 tests/ref/fate/adts-id3v2-demux
>
> diff --git a/tests/fate/demux.mak b/tests/fate/demux.mak
> index 9427ac30c8..306904b9de 100644
> --- a/tests/fate/demux.mak
> +++ b/tests/fate/demux.mak
> @@ -1,9 +1,10 @@
>  FATE_SAMPLES_DEMUX-$(call DEMDEC, AVI, FRAPS) += fate-avio-direct
>  fate-avio-direct: CMD = framecrc -avioflags direct -i 
> $(TARGET_SAMPLES)/fraps/fraps-v5-bouncing-balls-partial.avi -avioflags direct
>
> -FATE_SAMPLES_DEMUX-$(call DEMDEC, AAC, AAC) += fate-adts-demux 
> fate-adts-id3v1-demux
> +FATE_SAMPLES_DEMUX-$(call DEMDEC, AAC, AAC) += fate-adts-demux 
> fate-adts-id3v1-demux fate-adts-id3v2-demux
>  fate-adts-demux: CMD = crc -i $(TARGET_SAMPLES)/aac/ct_faac-adts.aac -c:a 
> copy
>  fate-adts-id3v1-demux: CMD = framecrc -f aac -i 
> $(TARGET_SAMPLES)/aac/id3v1.aac -c:a copy
> +fate-adts-id3v2-demux: CMD = framecrc -f aac -i 
> $(TARGET_SAMPLES)/aac/id3v2.aac -c:a copy
>
>  FATE_SAMPLES_DEMUX-$(CONFIG_AEA_DEMUXER) += fate-aea-demux
>  fate-aea-demux: CMD = crc -i $(TARGET_SAMPLES)/aea/chirp.aea -c:a copy
> diff --git a/tests/ref/fate/adts-id3v2-demux b/tests/ref/fate/adts-id3v2-demux
> new file mode 100644
> index 00..db00e3b81e
> --- /dev/null
> +++ b/tests/ref/fate/adts-id3v2-demux
> @@ -0,0 +1,240 @@
> +#tb 0: 1/28224000
> +#media_type 0: audio
> +#codec_id 0: aac
> +#sample_rate 0: 48000
> +#channel_layout 0: 4
> +#channel_layout_name 0: mono
> +0,  0,  0,   602112,  126, 0x639a3a5b
> +0, 602112, 602112,   602112,  135, 0x5b1f3ced
> +0,1204224,1204224,   602112,  123, 0xfcb73863
> +0,1806336,1806336,   602112,  126, 0x639a3a5b
> +0,2408448,2408448,   602112,  135, 0x5b1f3ced
> +0,3010560,3010560,   602112,  123, 0xfcb73863
> +0,3612672,3612672,   602112,  144, 0xa0434540
> +0,4214784,4214784,   602112,  119, 0x45053cc1
> +0,4816896,4816896,   602112,  111, 0x23043aaf
> +0,5419008,5419008,   602112,  126, 0x693a3a67
> +0,6021120,6021120,   602112,  149, 0x31304a34
> +0,6623232,6623232,   602112,  111, 0x21603aab
> +0,7225344,7225344,   602112,  132, 0xe42d43b3
> +0,7827456,7827456,   602112,  135, 0x5b1f3ced
> +0,8429568,8429568,   602112,  123, 0xfe8b3867
> +0,9031680,9031680,   602112,  144, 0xa26b4544
> +0,9633792,9633792,   602112,  129, 0xf7de3bc7
> +0,   10235904,   10235904,   602112,  111, 0x1fbc3aa7
> +0,   10838016,   10838016,   602112,  126, 0x657a3a5f
> +0,   11440128,   11440128,   602112,  140, 0xdb6542ec
> +0,   12042240,   12042240,   602112,  123, 0xfcb73863
> +0,   12644352,   12644352,   602112,  138, 0xad7e44b6
> +0,   13246464,   13246464,   602112,  119, 0x46c93cc5
> +0,   13848576,   13848576,   602112,  123, 0xfe8b3867
> +0,   14450688,   14450688,   602112,  144, 0xa26b4544
> +0,   15052800,   15052800,   602112,  129, 0xf7de3bc7
> +0,   15654912,   15654912,   602112,  111, 0x1fbc3aa7
> +0,   16257024,   16257024,   602112,  126, 0x657a3a5f
> +0,   16859136,   16859136,   602112,  140, 0xdb6542ec
> +0,   17461248,   17461248,   602112,  123, 0xfcb73863
> +0,   18063360,   18063360,   602112,  138, 0xad7e44b6
> +0,   18665472,   18665472,   602112,  119, 0x46c93cc5
> +0,   19267584,   19267584,   602112,  123, 0xfe8b3867
> +0,   19869696,   19869696,   602112,  144, 0xa26b4544
> +0,   20471808,   20471808,   602112,  129, 0xf7de3bc7
> +0,   21073920,   21073920,   602112,  111, 0x1fbc3aa7
> +0,   21676032,   21676032,   602112,  126, 0x657a3a5f
> +0,   22278144,   22278144,   602112,  140, 0xdb6542ec
> +0,   22880256,   22880256,   602112,  123, 0xfcb73863
> +0, 

Re: [FFmpeg-devel] [PATCH] libavformat/hls: Support metadata updates from subdemuxers

[Richard Shaffer]

On Saturday, February 3, 2018, wm4 <nfx...@googlemail.com> wrote:

> On Fri,  2 Feb 2018 12:59:45 -0800
> rshaf...@tunein.com wrote:
>
> > From: Richard Shaffer <rshaf...@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 iteration #2.
>
> You can pass "-v2" as argument to git send-email to automatically add
> "v2" to the subject.
>
> Oh, thanks. I guess I should have read the man page.

> >
> > In this version, we don't try to copy metadata from subdemuxer streams,
> only
> > from the AVFormatContext. The case where metadata is updated on a
> sub-demuxer
> > stream would have probably never happened.
>
> Oh, I thought it was the other way around.
>
> > The above change made the code that was previously in the
> > update_metadata_from_subdemuxer function a little simpler. That
> function was
> > also kind of ugly, because in one case it read and set flags, and in
> another it
> > didn't. It seemed simpler just to move the respective updates into
> read_header
> > and read_packet, respectively, so that's what I did.
> >
> > -Richard
> >
> >  libavformat/hls.c | 20 
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/libavformat/hls.c b/libavformat/hls.c
> > index 9bd54c84cc..c578bf86e3 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(, _meta);
> >  av_dict_copy(>ctx->metadata, metadata, 0);
> >  pls->id3_initial = metadata;
> >
> > @@ -1960,6 +1961,7 @@ static int hls_read_header(AVFormatContext *s)
> >  if (pls->id3_deferred_extra && pls->ctx->nb_streams == 1) {
> >  ff_id3v2_parse_apic(pls->ctx, >id3_deferred_extra);
> >  avformat_queue_attached_pictures(pls->ctx);
> > +ff_id3v2_parse_priv(pls->ctx, >id3_deferred_extra);
> >  ff_id3v2_free_extra_meta(>id3_deferred_extra);
> >  pls->id3_deferred_extra = NULL;
> >  }
> > @@ -1986,6 +1988,13 @@ 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.
> > + */
> > +if (pls->n_main_streams)
> > +av_dict_copy(>main_streams[0]->metadata,
> pls->ctx->metadata, 0);
> > +
> >  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 +2179,17 @@ static int hls_read_packet(AVFormatContext *s,
> AVPacket *pkt)
> >  return ret;
> >  }
> >
> > +// If sub-demuxer reports updated metadata, copy it to the
> first stream
> > +// and set its AVSTREAM_EVENT_FLAG_METADATA_UPDATED flag.
> > +if (pls->ctx->event_flags & AVFMT_EVENT_FLAG_METADATA_UPDATED)
> {
> > +if (pls->n_main_streams) {
> > +st = pls->main_streams[0];
> > +av_dict_copy(>metadata, pls->ctx->metadata, 0);
> > +st->event_flags |= AVSTREAM_EVENT_FLAG_METADATA_
> UPDATED;
> > +}
> > +pls->ctx->event_flags &= ~AVFMT_EVENT_FLAG_METADATA_
> UPDATED;
> > +}
> > +
> >  /* 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;
>
> Fine then. If it works ad nobody else has comments, I can push on Monday
> or so.
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/aac: Parse ID3 tags between ADTS frames.

[Richard Shaffer]

On Fri, Feb 2, 2018 at 5:52 AM, James Almer <jamr...@gmail.com> wrote:
> On 2/1/2018 11:37 PM, rshaf...@tunein.com wrote:
>> From: Richard Shaffer <rshaf...@tunein.com>
>>
>> While rare, ID3 tags may be inserted between ADTS frames. This change enables
>> parsing them and setting the appropriate metadata updated event flag.
>> ---
>> I have encountered a streaming provider that I must support which does this.
>> There are indications that it isn't totally off the wall, and that it does
>> happen in the wild:
>> * https://www.w3.org/TR/mse-byte-stream-format-mpeg-audio/
>>   (See specifically sections 3 and 4.)
>> * https://github.com/video-dev/hls.js/issues/508
>>
>> That being said, the providers that do this are in the minority. It seems 
>> most
>> streaming providers will do one of the following:
>>  1. If using .aac segments inside of HLS, use the #EXTINF for text metadata 
>> and
>> use an ID3 tag at the head of the segment for things like timing 
>> metadata.
>>  2. If streaming raw AAC over HTTP, use Icy metadata.
>>
>> Aside from comments on the code, I'd be interested in any opinion about 
>> whether
>> this is something that should or should not be supported in libavformat.
>>
>> Thanks,
>>
>> -Richard
>
> Can you provide a sample and add a fate test for this? To avoid
> regressions in the future. This demuxer still needs some work.
>
> Use the new probetags() function you added in your other id3v2 test if
> the tags between samples in the file are the only ones, or if they
> differ in some way from the ones at the beginning of the file, so
> ffprobe will report them instead.
> Alternatively, see fate-adts-demux in tests/fate/demux.mak

Oh, somebody did merge that test. How cool.

I don't think we can test this with ffprobe, though. It will just read
tags at the head of the file and not in the middle.

I have at least one sample that's ok for me to share. I'll work on a
separate patch to add a fate test.

-Richard

> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] libavformat/aac: Parse ID3 tags between ADTS frames.

[Richard Shaffer]

On Thu, Feb 1, 2018 at 10:02 PM, wm4 <nfx...@googlemail.com> wrote:
> On Thu,  1 Feb 2018 18:37:45 -0800
> rshaf...@tunein.com wrote:
>
>> From: Richard Shaffer <rshaf...@tunein.com>
>>
>> While rare, ID3 tags may be inserted between ADTS frames. This change enables
>> parsing them and setting the appropriate metadata updated event flag.
>> ---
>> I have encountered a streaming provider that I must support which does this.
>> There are indications that it isn't totally off the wall, and that it does
>> happen in the wild:
>> * https://www.w3.org/TR/mse-byte-stream-format-mpeg-audio/
>>   (See specifically sections 3 and 4.)
>> * https://github.com/video-dev/hls.js/issues/508
>>
>> That being said, the providers that do this are in the minority. It seems 
>> most
>> streaming providers will do one of the following:
>>  1. If using .aac segments inside of HLS, use the #EXTINF for text metadata 
>> and
>> use an ID3 tag at the head of the segment for things like timing 
>> metadata.
>>  2. If streaming raw AAC over HTTP, use Icy metadata.
>>
>> Aside from comments on the code, I'd be interested in any opinion about 
>> whether
>> this is something that should or should not be supported in libavformat.
>>
>> Thanks,
>>
>> -Richard
>>
>>  libavformat/aacdec.c | 45 +++--
>>  1 file changed, 43 insertions(+), 2 deletions(-)
>>
>> diff --git a/libavformat/aacdec.c b/libavformat/aacdec.c
>> index 36d558ff54..5ec706bdc7 100644
>> --- a/libavformat/aacdec.c
>> +++ b/libavformat/aacdec.c
>> @@ -22,8 +22,10 @@
>>
>>  #include "libavutil/intreadwrite.h"
>>  #include "avformat.h"
>> +#include "avio_internal.h"
>>  #include "internal.h"
>>  #include "id3v1.h"
>> +#include "id3v2.h"
>>  #include "apetag.h"
>>
>>  #define ADTS_HEADER_SIZE 7
>> @@ -116,13 +118,52 @@ static int adts_aac_read_header(AVFormatContext *s)
>>  return 0;
>>  }
>>
>> +static int handle_id3(AVFormatContext *s, AVPacket *pkt)
>> +{
>> +AVDictionary *metadata = NULL;
>> +AVIOContext ioctx;
>> +ID3v2ExtraMeta *id3v2_extra_meta = NULL;
>> +int ret;
>> +
>> +ret = av_append_packet(s->pb, pkt, ff_id3v2_tag_len(pkt->data) - 
>> pkt->size);
>> +if (ret < 0) {
>> +av_packet_unref(pkt);
>> +return ret;
>> +}
>> +
>> +ffio_init_context(, pkt->data, pkt->size, 0, NULL, NULL, NULL, 
>> NULL);
>> +ff_id3v2_read_dict(, , ID3v2_DEFAULT_MAGIC, 
>> _extra_meta);
>> +if ((ret = ff_id3v2_parse_priv_dict(, _extra_meta)) < 0)
>> +goto error;
>> +
>> +if (metadata) {
>> +if ((ret = av_dict_copy(>metadata, metadata, 0)) < 0)
>> +goto error;
>
> AFAIK that would merge the existing metadata with the new one (i.e. not
> delete entries that are in the new metadata). But not sure if this is
> intended, or how exactly it should work. Intuitively I'd say it should
> completely replace previous ID3v2s.

That's right:
* If new metadata has a key that already exists in metadata, the old
value will be replaced.
* If new metadata has a key that doesn't exist in metadata, it will be added.
* Any other keys/values already in old metadata will be preserved.

This seems to be the behavior of Icy as well as other demuxers that
support updates. I think it's slightly better to update this way
rather than completely replacing the entire dictionary, but I don't
feel too strongly about it.

>
>> +s->event_flags |= AVFMT_EVENT_FLAG_METADATA_UPDATED;
>> +}
>> +
>> +error:
>> +av_packet_unref(pkt);
>> +ff_id3v2_free_extra_meta(_extra_meta);
>> +av_dict_free();
>> +
>> +return ret;
>> +}
>> +
>>  static int adts_aac_read_packet(AVFormatContext *s, AVPacket *pkt)
>>  {
>>  int ret, fsize;
>>
>> -ret = av_get_packet(s->pb, pkt, ADTS_HEADER_SIZE);
>> +ret = av_get_packet(s->pb, pkt, FFMAX(ID3v2_HEADER_SIZE, 
>> ADTS_HEADER_SIZE));
>> +
>> +if (ret >= ID3v2_HEADER_SIZE && ff_id3v2_match(pkt->data, 
>> ID3v2_DEFAULT_MAGIC)) {
>> +if ((ret = handle_id3(s, pkt)) >= 0)
>> +ret = av_get_packet(s->pb, pkt, ADTS_HEADER_SIZE);
>> +}
>> +
>>  if (ret < 0)
>>  return ret;
>> +
>>  if (ret < ADTS_HEADER_SIZE) {
>

Re: [FFmpeg-devel] [PATCH] libavformat/hls: Support metadata updates from subdemuxers

[Richard Shaffer]

On Thu, Feb 1, 2018 at 10:18 PM, wm4 <nfx...@googlemail.com> wrote:
> On Thu,  1 Feb 2018 18:44:34 -0800
> rshaf...@tunein.com wrote:
>
>> From: Richard Shaffer <rshaf...@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(, _meta);
>>  av_dict_copy(>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(>metadata, pls->ctx->metadata, 0);
>> +} else if (pls->ctx->event_flags & 
>> AVFMT_EVENT_FLAG_METADATA_UPDATED) {
>> +av_dict_copy(>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(>metadata, ist->metadata, 0);
>> +} else if (ist->event_flags & AVSTREAM_EVENT_FLAG_METADATA_UPDATED) 
>> {
>> +av_dict_copy(>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, >id3_deferred_extra);
>>  avformat_queue_attached_pictures(pls->ctx);
>> +ff_id3v2_parse_priv(pls->ctx, >id3_deferred_extra);
>>  ff_id3v2_free_extra_meta(

Re: [FFmpeg-devel] [PATCH] id3v2: fix unsynchronization

[Richard Shaffer]

LGTM, for whatever my opinion is worth.

Also easier to follow than the old code.

-Richard

On Tue, Jan 30, 2018 at 4:47 AM, wm4  wrote:
> On Tue, 30 Jan 2018 13:43:25 +0100
> wm4  wrote:
>
>> The ID3v2 "unsynchronization scheme" requires replacing any 0xFF 0x00
>> sequences with 0xFF. This has to be done on every byte of the source
>> data, while the current code skipped a byte after a replacement. This
>> meant 0xFF 0x00 0xFF 00 was translated to 0xFF 0xFF 0x00 instead of 0xFF
>> 0xFF. It feels a bit messy to do this correctly with the avio use. But
>> fortunately, this translation can be done in-place, so we can just do it
>> in memory.
>>
>> Inspired by what taglib does.
>> ---
>> Sample (which had corrupted cover art, displays fine with the fix):
>> https://0x0.st/sbQ9.bin (unfortunately a bit too large for FATE)
>> ---
>>  libavformat/id3v2.c | 26 ++
>>  1 file changed, 14 insertions(+), 12 deletions(-)
>>
>> diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
>> index b80178d67a..f7de26a1d8 100644
>> --- a/libavformat/id3v2.c
>> +++ b/libavformat/id3v2.c
>> @@ -976,19 +976,21 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary 
>> **metadata,
>>  }
>>  }
>>  if (unsync || tunsync) {
>> -int64_t end = avio_tell(pb) + tlen;
>> -uint8_t *b;
>> -
>> -b = buffer;
>> -while (avio_tell(pb) < end && b - buffer < tlen && 
>> !pb->eof_reached) {
>> -*b++ = avio_r8(pb);
>> -if (*(b - 1) == 0xff && avio_tell(pb) < end - 1 &&
>> -b - buffer < tlen &&
>> -!pb->eof_reached ) {
>> -uint8_t val = avio_r8(pb);
>> -*b++ = val ? val : avio_r8(pb);
>> -}
>> +uint8_t *b = buffer;
>> +uint8_t *t = buffer;
>> +uint8_t *end = t + tlen;
>> +
>> +if (avio_read(pb, buffer, tlen) != tlen) {
>> +av_log(s, AV_LOG_ERROR, "Failed to read tag data\n");
>> +goto seek;
>>  }
>> +
>> +while (t != end) {
>> +*b++ = *t++;
>> +if (t != end && t[-1] == 0xff && !t[0])
>> +t++;
>> +}
>> +
>>  ffio_init_context(_local, buffer, b - buffer, 0, NULL, 
>> NULL, NULL,
>>NULL);
>>  tlen = b - buffer;
>
> Also I just saw that 9ae80e6a9cefcab61e867256ba19ef78a4bfe0cb seems to
> claim that the current code is correct (and my commit essentially
> reverts it). Taglib decodes the tag correctly, though.
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] fate: add id3v2 test

[Richard Shaffer]

Maybe a result of my mail client. Let me send from git send-email.

-Richard

On Thu, Jan 25, 2018 at 11:03 PM, Richard Shaffer <rshaf...@tunein.com> wrote:
> From f7398407c1f5822e1536ce03d46c885b2ad00c38 Mon Sep 17 00:00:00 2001
> From: Richard Shaffer <rshaf...@tunein.com>
> Date: Thu, 25 Jan 2018 19:54:59 -0800
> Subject: [PATCH] fate: add id3v2 test
>
> Adds basic unit test for parsing ID3v2 tags.
> ---
> This follows the suggestion to use the ffprobe utility instead of a
> test program.
>
> The required test sample is attached.
>
> -Richard
>
> tests/Makefile | 1 +
> tests/fate-run.sh | 4 
> tests/fate/id3v2.mak | 6 ++
> tests/ref/fate/id3v2-read | 5 +
> 4 files changed, 16 insertions(+)
> create mode 100644 tests/fate/id3v2.mak
> create mode 100644 tests/ref/fate/id3v2-read
>
> diff --git a/tests/Makefile b/tests/Makefile
> index 14b9601378..327e3f4420 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -130,6 +130,7 @@ include $(SRC_PATH)/tests/fate/gapless.mak
> include $(SRC_PATH)/tests/fate/gif.mak
> include $(SRC_PATH)/tests/fate/h264.mak
> include $(SRC_PATH)/tests/fate/hevc.mak
> +include $(SRC_PATH)/tests/fate/id3v2.mak
> include $(SRC_PATH)/tests/fate/image.mak
> include $(SRC_PATH)/tests/fate/indeo.mak
> include $(SRC_PATH)/tests/fate/libavcodec.mak
> diff --git a/tests/fate-run.sh b/tests/fate-run.sh
> index 05f4ca5e20..82862b7ef4 100755
> --- a/tests/fate-run.sh
> +++ b/tests/fate-run.sh
> @@ -88,6 +88,10 @@ probefmt(){
> run ffprobe${PROGSUF} -show_entries format=format_name -print_format
> default=nw=1:nk=1 -v 0 "$@"
> }
> +probetags(){
> + run ffprobe${PROGSUF} -show_entries format_tags -v 0 "$@"
> +}
> +
> runlocal(){
> test "${V:-0}" -gt 0 && echo ${base}/"$@" ${base} >&3
> ${base}/"$@" ${base}
> diff --git a/tests/fate/id3v2.mak b/tests/fate/id3v2.mak
> new file mode 100644
> index 00..af60ee12b5
> --- /dev/null
> +++ b/tests/fate/id3v2.mak
> @@ -0,0 +1,6 @@
> +ID3V2_TEST_FILE=tests/data/id3v2-test.mp3
> +
> +FATE_SAMPLES_ID3V2-$(CONFIG_MP3_DEMUXER) += fate-id3v2-read
> +fate-id3v2-read: CMD = probetags $(TARGET_SAMPLES)/id3v2/id3v2-test.mp3
> +
> +FATE_SAMPLES_FFPROBE += $(FATE_SAMPLES_ID3V2-yes)
> \ No newline at end of file
> diff --git a/tests/ref/fate/id3v2-read b/tests/ref/fate/id3v2-read
> new file mode 100644
> index 00..965c8695e8
> --- /dev/null
> +++ b/tests/ref/fate/id3v2-read
> @@ -0,0 +1,5 @@
> +[FORMAT]
> +TAG:title=id3v2-test
> +TAG:id3v2_priv.testowner=testdata
> +TAG:id3v2_priv.testowner2=\x00\x01\x02
> +[/FORMAT]
> --
> 2.14.3 (Apple Git-98)
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] fate: add id3v2 test

[Richard Shaffer]

>From f7398407c1f5822e1536ce03d46c885b2ad00c38 Mon Sep 17 00:00:00 2001
From: Richard Shaffer <rshaf...@tunein.com>
Date: Thu, 25 Jan 2018 19:54:59 -0800
Subject: [PATCH] fate: add id3v2 test

Adds basic unit test for parsing ID3v2 tags.
---
This follows the suggestion to use the ffprobe utility instead of a
test program.

The required test sample is attached.

-Richard

tests/Makefile | 1 +
tests/fate-run.sh | 4 
tests/fate/id3v2.mak | 6 ++
tests/ref/fate/id3v2-read | 5 +
4 files changed, 16 insertions(+)
create mode 100644 tests/fate/id3v2.mak
create mode 100644 tests/ref/fate/id3v2-read

diff --git a/tests/Makefile b/tests/Makefile
index 14b9601378..327e3f4420 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -130,6 +130,7 @@ include $(SRC_PATH)/tests/fate/gapless.mak
include $(SRC_PATH)/tests/fate/gif.mak
include $(SRC_PATH)/tests/fate/h264.mak
include $(SRC_PATH)/tests/fate/hevc.mak
+include $(SRC_PATH)/tests/fate/id3v2.mak
include $(SRC_PATH)/tests/fate/image.mak
include $(SRC_PATH)/tests/fate/indeo.mak
include $(SRC_PATH)/tests/fate/libavcodec.mak
diff --git a/tests/fate-run.sh b/tests/fate-run.sh
index 05f4ca5e20..82862b7ef4 100755
--- a/tests/fate-run.sh
+++ b/tests/fate-run.sh
@@ -88,6 +88,10 @@ probefmt(){
run ffprobe${PROGSUF} -show_entries format=format_name -print_format
default=nw=1:nk=1 -v 0 "$@"
}
+probetags(){
+ run ffprobe${PROGSUF} -show_entries format_tags -v 0 "$@"
+}
+
runlocal(){
test "${V:-0}" -gt 0 && echo ${base}/"$@" ${base} >&3
${base}/"$@" ${base}
diff --git a/tests/fate/id3v2.mak b/tests/fate/id3v2.mak
new file mode 100644
index 00..af60ee12b5
--- /dev/null
+++ b/tests/fate/id3v2.mak
@@ -0,0 +1,6 @@
+ID3V2_TEST_FILE=tests/data/id3v2-test.mp3
+
+FATE_SAMPLES_ID3V2-$(CONFIG_MP3_DEMUXER) += fate-id3v2-read
+fate-id3v2-read: CMD = probetags $(TARGET_SAMPLES)/id3v2/id3v2-test.mp3
+
+FATE_SAMPLES_FFPROBE += $(FATE_SAMPLES_ID3V2-yes)
\ No newline at end of file
diff --git a/tests/ref/fate/id3v2-read b/tests/ref/fate/id3v2-read
new file mode 100644
index 00..965c8695e8
--- /dev/null
+++ b/tests/ref/fate/id3v2-read
@@ -0,0 +1,5 @@
+[FORMAT]
+TAG:title=id3v2-test
+TAG:id3v2_priv.testowner=testdata
+TAG:id3v2_priv.testowner2=\x00\x01\x02
+[/FORMAT]
-- 
2.14.3 (Apple Git-98)


id3v2-test.mp3
Description: audio/mpeg
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] fate: add id3v2 tests

[Richard Shaffer]

On Tue, Jan 23, 2018 at 6:15 PM, James Almer  wrote:
>
>
> ffprobe -show_format is probably best for an id3v2 tag. Add a new
> probeformat() function to fate-run.sh like
>
> probeformat(){
> run ffprobe${PROGSUF} -show_format -v 0 "$@"
> }
>
> Then call it with the sample in question a argument. See how exif.mak
> uses probeframes().
>

Thanks, James. That's helpful. I'll take a look today.

-Richard

> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] fate: add id3v2 tests

[Richard Shaffer]

On Tue, Jan 23, 2018 at 2:32 PM, wm4  wrote:

>
> I think this test program is pretty nice, though usually we try to get
> by with running the "ffmpeg" utility to test the libs. Having a
> relatively big program to test some obscure functionality might have a
> maintenance burden. I'm not sure how others feel about it; if nobody
> has a strong opinion on this I'll probably apply this in a week or so.
>

It's definitely not my desire to create a maintenance burden for anyone. I
used the ffmpeg utility initially to test my changes, and I'm sure that
shell scripting would have been easier than writing a C program.

There were pretty good templates for other test programs, but I didn't come
across a collection of shell scripts or Makefile targets that seemed to
illustrate a clear pattern. I'll look a little harder. If there is a
pointer to an example test, though, that would probably be useful.

Thanks,

-Richard

> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat: add option to parse/store ID3 PRIV tags in metadata.

[Richard Shaffer]

On Mon, Jan 22, 2018 at 3:18 PM, wm4 <nfx...@googlemail.com> wrote:

> On Wed, 17 Jan 2018 16:34:39 -0800
> rshaf...@tunein.com wrote:
>
> > From: Richard Shaffer <rshaf...@tunein.com>
> >
> > Enables getting access to ID3 PRIV tags from the command-line or
> metadata API
> > when demuxing. The PRIV owner is stored as the metadata key prepended
> with
> > "id3v2_priv.", and the data is stored as the metadata value. As PRIV
> tags may
> > contain arbitrary data, non-printable characters, including NULL bytes,
> are
> > escaped as \xXX.
> >
> > Similarly, any metadata tags that begin with "id3v2_priv." are inserted
> as ID3
> > PRIV tags into the output (assuming the format supports ID3). \xXX
> sequences in
> > the value are un-escaped to their byte value.
> > ---
> >
> > Sorry. One more update. I realized there was a possibility of reading
> past the
> > end of input in id3v2_put_priv, so I added a fix.
> >
> > -Richard
> >
> >  libavformat/id3v2.c| 48 +
> >  libavformat/id3v2.h| 15 
> >  libavformat/id3v2enc.c | 64 ++
> 
> >  libavformat/utils.c|  2 ++
> >  4 files changed, 129 insertions(+)
> >
> > diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
> > index 6c216ba7a2..e46174d7c7 100644
> > --- a/libavformat/id3v2.c
> > +++ b/libavformat/id3v2.c
> > @@ -33,6 +33,7 @@
> >  #endif
> >
> >  #include "libavutil/avstring.h"
> > +#include "libavutil/bprint.h"
> >  #include "libavutil/dict.h"
> >  #include "libavutil/intreadwrite.h"
> >  #include "avio_internal.h"
> > @@ -1224,3 +1225,50 @@ end:
> >  av_freep();
> >  return ret;
> >  }
> > +
> > +int ff_id3v2_parse_priv_dict(AVDictionary **metadata, ID3v2ExtraMeta
> **extra_meta)
> > +{
> > +ID3v2ExtraMeta *cur;
> > +int dict_flags = AV_DICT_DONT_OVERWRITE | AV_DICT_DONT_STRDUP_KEY |
> AV_DICT_DONT_STRDUP_VAL;
> > +
> > +for (cur = *extra_meta; cur; cur = cur->next) {
> > +if (!strcmp(cur->tag, "PRIV")) {
> > +ID3v2ExtraMetaPRIV *priv = cur->data;
> > +AVBPrint bprint;
> > +char * escaped, * key;
> > +int i, ret;
> > +
> > +if ((key = av_asprintf(ID3v2_PRIV_METADATA_PREFIX "%s",
> priv->owner)) == NULL) {
> > +return AVERROR(ENOMEM);
> > +}
> > +
> > +av_bprint_init(, priv->datasize + 1,
> AV_BPRINT_SIZE_UNLIMITED);
> > +
> > +for (i = 0; i < priv->datasize; i++) {
> > +if (priv->data[i] < 32 || priv->data[i] > 126 ||
> priv->data[i] == '\\') {
> > +av_bprintf(, "\\x%02x", priv->data[i]);
> > +} else {
> > +av_bprint_chars(, priv->data[i], 1);
> > +}
> > +}
> > +
> > +if ((ret = av_bprint_finalize(, )) < 0) {
> > +av_free(key);
> > +return ret;
> > +}
> > +
> > +if ((ret = av_dict_set(metadata, key, escaped, dict_flags))
> < 0) {
> > +av_free(key);
> > +av_free(escaped);
> > +return ret;
> > +}
> > +}
> > +}
> > +
> > +return 0;
> > +}
> > +
> > +int ff_id3v2_parse_priv(AVFormatContext *s, ID3v2ExtraMeta
> **extra_meta)
> > +{
> > +return ff_id3v2_parse_priv_dict(>metadata, extra_meta);
> > +}
> > \ No newline at end of file
> > diff --git a/libavformat/id3v2.h b/libavformat/id3v2.h
> > index 5e64ead096..9de0bee374 100644
> > --- a/libavformat/id3v2.h
> > +++ b/libavformat/id3v2.h
> > @@ -39,6 +39,8 @@
> >  #define ID3v2_FLAG_ENCRYPTION  0x0004
> >  #define ID3v2_FLAG_COMPRESSION 0x0008
> >
> > +#define ID3v2_PRIV_METADATA_PREFIX "id3v2_priv."
> > +
> >  enum ID3v2Encoding {
> >  ID3v2_ENCODING_ISO8859  = 0,
> >  ID3v2_ENCODING_UTF16BOM = 1,
> > @@ -167,6 +169,19 @@ int ff_id3v2_parse_apic(AVFormatContext *s,
> ID3v2ExtraMeta **extra_meta);
> >   */
> >  int ff_id3v2_parse_chapters(AVFormatContext *s, ID3v2ExtraMeta
> **extra_meta);
> >
> > +/**
> > + * Parse PRIV tags into a dictionary. The PRIV owner

Re: [FFmpeg-devel] [PATCH] avformat: add option to parse/store ID3 PRIV tags in metadata.

[Richard Shaffer]

I wanted to push this thread to the top of the list one more time.

wm4 was kind enough to make some comments on the original patch, which I
think I've addressed. It would be good to know where it stands. If it might
be useful to other people, it would be nice to have it accepted in ffmpeg,
or to have some additional comments if more changes are needed.

On the other hand, I don't want to be a squeaky wheel. If there is no
response or interest, I won't have any hard feelings.

Thanks again,

-Richard

On Wed, Jan 17, 2018 at 4:34 PM, <rshaf...@tunein.com> wrote:

> From: Richard Shaffer <rshaf...@tunein.com>
>
> Enables getting access to ID3 PRIV tags from the command-line or metadata
> API
> when demuxing. The PRIV owner is stored as the metadata key prepended with
> "id3v2_priv.", and the data is stored as the metadata value. As PRIV tags
> may
> contain arbitrary data, non-printable characters, including NULL bytes, are
> escaped as \xXX.
>
> Similarly, any metadata tags that begin with "id3v2_priv." are inserted as
> ID3
> PRIV tags into the output (assuming the format supports ID3). \xXX
> sequences in
> the value are un-escaped to their byte value.
> ---
>
> Sorry. One more update. I realized there was a possibility of reading past
> the
> end of input in id3v2_put_priv, so I added a fix.
>
> -Richard
>
>  libavformat/id3v2.c| 48 +
>  libavformat/id3v2.h| 15 
>  libavformat/id3v2enc.c | 64 ++
> 
>  libavformat/utils.c|  2 ++
>  4 files changed, 129 insertions(+)
>
> diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
> index 6c216ba7a2..e46174d7c7 100644
> --- a/libavformat/id3v2.c
> +++ b/libavformat/id3v2.c
> @@ -33,6 +33,7 @@
>  #endif
>
>  #include "libavutil/avstring.h"
> +#include "libavutil/bprint.h"
>  #include "libavutil/dict.h"
>  #include "libavutil/intreadwrite.h"
>  #include "avio_internal.h"
> @@ -1224,3 +1225,50 @@ end:
>  av_freep();
>  return ret;
>  }
> +
> +int ff_id3v2_parse_priv_dict(AVDictionary **metadata, ID3v2ExtraMeta
> **extra_meta)
> +{
> +ID3v2ExtraMeta *cur;
> +int dict_flags = AV_DICT_DONT_OVERWRITE | AV_DICT_DONT_STRDUP_KEY |
> AV_DICT_DONT_STRDUP_VAL;
> +
> +for (cur = *extra_meta; cur; cur = cur->next) {
> +if (!strcmp(cur->tag, "PRIV")) {
> +ID3v2ExtraMetaPRIV *priv = cur->data;
> +AVBPrint bprint;
> +char * escaped, * key;
> +int i, ret;
> +
> +if ((key = av_asprintf(ID3v2_PRIV_METADATA_PREFIX "%s",
> priv->owner)) == NULL) {
> +return AVERROR(ENOMEM);
> +}
> +
> +av_bprint_init(, priv->datasize + 1,
> AV_BPRINT_SIZE_UNLIMITED);
> +
> +for (i = 0; i < priv->datasize; i++) {
> +if (priv->data[i] < 32 || priv->data[i] > 126 ||
> priv->data[i] == '\\') {
> +av_bprintf(, "\\x%02x", priv->data[i]);
> +} else {
> +av_bprint_chars(, priv->data[i], 1);
> +}
> +}
> +
> +if ((ret = av_bprint_finalize(, )) < 0) {
> +av_free(key);
> +return ret;
> +}
> +
> +if ((ret = av_dict_set(metadata, key, escaped, dict_flags)) <
> 0) {
> +av_free(key);
> +av_free(escaped);
> +return ret;
> +}
> +}
> +}
> +
> +return 0;
> +}
> +
> +int ff_id3v2_parse_priv(AVFormatContext *s, ID3v2ExtraMeta **extra_meta)
> +{
> +return ff_id3v2_parse_priv_dict(>metadata, extra_meta);
> +}
> \ No newline at end of file
> diff --git a/libavformat/id3v2.h b/libavformat/id3v2.h
> index 5e64ead096..9de0bee374 100644
> --- a/libavformat/id3v2.h
> +++ b/libavformat/id3v2.h
> @@ -39,6 +39,8 @@
>  #define ID3v2_FLAG_ENCRYPTION  0x0004
>  #define ID3v2_FLAG_COMPRESSION 0x0008
>
> +#define ID3v2_PRIV_METADATA_PREFIX "id3v2_priv."
> +
>  enum ID3v2Encoding {
>  ID3v2_ENCODING_ISO8859  = 0,
>  ID3v2_ENCODING_UTF16BOM = 1,
> @@ -167,6 +169,19 @@ int ff_id3v2_parse_apic(AVFormatContext *s,
> ID3v2ExtraMeta **extra_meta);
>   */
>  int ff_id3v2_parse_chapters(AVFormatContext *s, ID3v2ExtraMeta
> **extra_meta);
>
> +/**
> + * Parse PRIV tags into a dictionary. The PRIV owner is the metadata key.
> The
> + * PRIV data is the value, with non-printable characters escaped.
> + */
> +int ff_

Re: [FFmpeg-devel] [PATCH] avformat: add option to parse/store ID3 PRIV tags in metadata.

[Richard Shaffer]

On Wed, Jan 17, 2018 at 11:38 AM, wm4 <nfx...@googlemail.com> wrote:

> On Wed, 17 Jan 2018 11:10:26 -0800
> Richard Shaffer <rshaf...@tunein.com> wrote:
>
> > [...]
> > I'm also not sure if an option for compatibility is needed. It's
> > > probably fine to prefix the name with something (maybe "id3v2_priv."?),
> > > and always export it.
> > >
> >
> > I guess my thought was that users of ffmpeg/ffprobe might have some
> > scripting or programming around the metadata API, such as dumping
> metadata
> > with -f ffmetadata and then mapping it to a stream later. In those cases,
> > this change might suddenly cause behavior that they weren't expecting.
>
> That's probably a lost cause. The "metadata" the FFmpeg API is full of
> container specific things and rather arbitrary. I don't think it
> matters to invent and export a few new keys.
>
> What needs to be assured is that the entry names can't clash with
> generic tag names (there's a list in avformat.h), which is why I
> suggested adding a prefix to the name.
>
> Although if metadata entries are very long (like big binary blobs)
> there might be a problem. I don't know what's the use of those priv
> tags though.
>

The tags that I have seen are short. However, most ID3v2 frames have a
maximum length of 256MB, even text information frames. If it is a concern,
we should probably implement a limit for ID3 tags generally.

>
> > Maybe that would be mitigated to some degree if we could also map
> > 'id3v2_priv' back to PRIV tags on output. That would actually address a
> use
> > case that I have and would be super from my standpoint. I'll work on
> > implementing that. Please let me know if you have additional thoughts.
>
> Probably makes sense.
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel

Re: [FFmpeg-devel] [PATCH] avformat: add option to parse/store ID3 PRIV tags in metadata.

[Richard Shaffer]

Thanks for reviewing; it's much appreciated. I responded to one of the
comments in-line. I'll work on updating the patch to address your comments.

On Wed, Jan 17, 2018 at 10:21 AM, wm4 <nfx...@googlemail.com> wrote:

> On Fri, 12 Jan 2018 13:13:05 -0800
> rshaf...@tunein.com wrote:
>
> > From: Richard Shaffer <rshaf...@tunein.com>
> >
> > Enables getting access to ID3 PRIV tags from the command-line or
> metadata API
> > when demuxing. The PRIV owner is stored as the metadata key, and the
> data is
> > stored as the metadata value. As PRIV tags may contain arbitrary data,
> non-
> > printable characters, including NULL bytes, are escaped as \xXX.
> >
> > As this introduces a change in behavior, it must be enabled by setting
> the
> > 'id3v2_parse_priv' option.
> > ---
> >
> > I want to be able to expose PRIV tags using an existing API, but not
> sure if
> > this is the best approach. In particular, PRIV data may be of any type,
> while
> > metadata (and the AVDictionary type it uses) expresses values as
> strings. Any
> > feedback on the approach or specifics would be much appreciated,
> especially if
> > there is a suggestion for a better way to accomplish this.
> >
> > -Richard
> >
> >  libavformat/aacdec.c | 40 +++-
> >  libavformat/id3v2.c  | 40 
> >  libavformat/id3v2.h  | 13 +
> >  libavformat/mp3dec.c |  2 ++
> >  libavformat/utils.c  |  4 
> >  5 files changed, 90 insertions(+), 9 deletions(-)
> >
> > diff --git a/libavformat/aacdec.c b/libavformat/aacdec.c
> > index 36d558ff54..46e10f34af 100644
> > --- a/libavformat/aacdec.c
> > +++ b/libavformat/aacdec.c
> > @@ -21,6 +21,7 @@
> >   */
> >
> >  #include "libavutil/intreadwrite.h"
> > +#include "libavutil/opt.h"
> >  #include "avformat.h"
> >  #include "internal.h"
> >  #include "id3v1.h"
> > @@ -28,6 +29,11 @@
> >
> >  #define ADTS_HEADER_SIZE 7
> >
> > +typedef struct AACDemuxContext {
> > +AVClass *class;
> > +int id3v2_parse_priv;
> > +} AACDemuxContext;
> > +
> >  static int adts_aac_probe(AVProbeData *p)
> >  {
> >  int max_frames = 0, first_frames = 0;
> > @@ -146,14 +152,30 @@ static int adts_aac_read_packet(AVFormatContext
> *s, AVPacket *pkt)
> >  return ret;
> >  }
> >
> > +static const AVOption aac_options[] = {
> > +{ "id3v2_parse_priv",
> > +"parse ID3v2 PRIV tags", offsetof(AACDemuxContext,
> id3v2_parse_priv),
> > +AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1,
> AV_OPT_FLAG_DECODING_PARAM },
> > +{ NULL },
> > +};
> > +
> > +static const AVClass aac_class = {
> > +.class_name = "aac",
> > +.item_name  = av_default_item_name,
> > +.option = aac_options,
> > +.version= LIBAVUTIL_VERSION_INT,
> > +};
> > +
> >  AVInputFormat ff_aac_demuxer = {
> > -.name = "aac",
> > -.long_name= NULL_IF_CONFIG_SMALL("raw ADTS AAC (Advanced Audio
> Coding)"),
> > -.read_probe   = adts_aac_probe,
> > -.read_header  = adts_aac_read_header,
> > -.read_packet  = adts_aac_read_packet,
> > -.flags= AVFMT_GENERIC_INDEX,
> > -.extensions   = "aac",
> > -.mime_type= "audio/aac,audio/aacp,audio/x-aac",
> > -.raw_codec_id = AV_CODEC_ID_AAC,
> > +.name   = "aac",
> > +.long_name  = NULL_IF_CONFIG_SMALL("raw ADTS AAC (Advanced
> Audio Coding)"),
> > +.read_probe = adts_aac_probe,
> > +.read_header= adts_aac_read_header,
> > +.read_packet= adts_aac_read_packet,
> > +.flags  = AVFMT_GENERIC_INDEX,
> > +.priv_class = _class,
> > +.priv_data_size = sizeof(AACDemuxContext),
> > +.extensions = "aac",
> > +.mime_type  = "audio/aac,audio/aacp,audio/x-aac",
> > +.raw_codec_id   = AV_CODEC_ID_AAC,
> >  };
>
> AAC and mp3 are by far not the only formats that can use ID3v2. FFmpeg
> accepts ID3v2 tags on basically all file formats. So just adding a
> private option to aac and mp3 doesn't make that much sense.


Fair point.

>
>
I'm also not sure if an option for compatibility is needed. It's
> probably fine to prefix the name with something (maybe "id3v2_priv."?),
> and always export it.
>

I guess 

Re: [FFmpeg-devel] [PATCH] avformat: add option to parse/store ID3 PRIV tags in metadata.

[Richard Shaffer]

I just want to ping the list again to see if anyone would be willing to
have a look at this change set. I sent it off last time on a Friday
evening, so I'm not sure if maybe it was just forgotten or missed over the
weekend, or if it's just not interesting to anyone else.

If this isn't useful to anyone else, that would also be good to know. Any
feedback at all would be appreciated if anyone is willing.

Much thanks,

-Richard

On Fri, Jan 12, 2018 at 1:13 PM, <rshaf...@tunein.com> wrote:

> From: Richard Shaffer <rshaf...@tunein.com>
>
> Enables getting access to ID3 PRIV tags from the command-line or metadata
> API
> when demuxing. The PRIV owner is stored as the metadata key, and the data
> is
> stored as the metadata value. As PRIV tags may contain arbitrary data, non-
> printable characters, including NULL bytes, are escaped as \xXX.
>
> As this introduces a change in behavior, it must be enabled by setting the
> 'id3v2_parse_priv' option.
> ---
>
> I want to be able to expose PRIV tags using an existing API, but not sure
> if
> this is the best approach. In particular, PRIV data may be of any type,
> while
> metadata (and the AVDictionary type it uses) expresses values as strings.
> Any
> feedback on the approach or specifics would be much appreciated,
> especially if
> there is a suggestion for a better way to accomplish this.
>
> -Richard
>
>  libavformat/aacdec.c | 40 +++-
>  libavformat/id3v2.c  | 40 
>  libavformat/id3v2.h  | 13 +
>  libavformat/mp3dec.c |  2 ++
>  libavformat/utils.c  |  4 
>  5 files changed, 90 insertions(+), 9 deletions(-)
>
> diff --git a/libavformat/aacdec.c b/libavformat/aacdec.c
> index 36d558ff54..46e10f34af 100644
> --- a/libavformat/aacdec.c
> +++ b/libavformat/aacdec.c
> @@ -21,6 +21,7 @@
>   */
>
>  #include "libavutil/intreadwrite.h"
> +#include "libavutil/opt.h"
>  #include "avformat.h"
>  #include "internal.h"
>  #include "id3v1.h"
> @@ -28,6 +29,11 @@
>
>  #define ADTS_HEADER_SIZE 7
>
> +typedef struct AACDemuxContext {
> +AVClass *class;
> +int id3v2_parse_priv;
> +} AACDemuxContext;
> +
>  static int adts_aac_probe(AVProbeData *p)
>  {
>  int max_frames = 0, first_frames = 0;
> @@ -146,14 +152,30 @@ static int adts_aac_read_packet(AVFormatContext *s,
> AVPacket *pkt)
>  return ret;
>  }
>
> +static const AVOption aac_options[] = {
> +{ "id3v2_parse_priv",
> +"parse ID3v2 PRIV tags", offsetof(AACDemuxContext,
> id3v2_parse_priv),
> +AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, AV_OPT_FLAG_DECODING_PARAM
> },
> +{ NULL },
> +};
> +
> +static const AVClass aac_class = {
> +.class_name = "aac",
> +.item_name  = av_default_item_name,
> +.option = aac_options,
> +.version= LIBAVUTIL_VERSION_INT,
> +};
> +
>  AVInputFormat ff_aac_demuxer = {
> -.name = "aac",
> -.long_name= NULL_IF_CONFIG_SMALL("raw ADTS AAC (Advanced Audio
> Coding)"),
> -.read_probe   = adts_aac_probe,
> -.read_header  = adts_aac_read_header,
> -.read_packet  = adts_aac_read_packet,
> -.flags= AVFMT_GENERIC_INDEX,
> -.extensions   = "aac",
> -.mime_type= "audio/aac,audio/aacp,audio/x-aac",
> -.raw_codec_id = AV_CODEC_ID_AAC,
> +.name   = "aac",
> +.long_name  = NULL_IF_CONFIG_SMALL("raw ADTS AAC (Advanced Audio
> Coding)"),
> +.read_probe = adts_aac_probe,
> +.read_header= adts_aac_read_header,
> +.read_packet= adts_aac_read_packet,
> +.flags  = AVFMT_GENERIC_INDEX,
> +.priv_class = _class,
> +.priv_data_size = sizeof(AACDemuxContext),
> +.extensions = "aac",
> +.mime_type  = "audio/aac,audio/aacp,audio/x-aac",
> +.raw_codec_id   = AV_CODEC_ID_AAC,
>  };
> diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
> index 6c216ba7a2..dd151dd7f2 100644
> --- a/libavformat/id3v2.c
> +++ b/libavformat/id3v2.c
> @@ -33,6 +33,7 @@
>  #endif
>
>  #include "libavutil/avstring.h"
> +#include "libavutil/bprint.h"
>  #include "libavutil/dict.h"
>  #include "libavutil/intreadwrite.h"
>  #include "avio_internal.h"
> @@ -1224,3 +1225,42 @@ end:
>  av_freep();
>  return ret;
>  }
> +
> +int ff_id3v2_parse_priv_dict(AVDictionary **metadata, ID3v2ExtraMeta
> **extra_meta)
> +{
> +ID3v2ExtraMeta *cur;
> +int

Re: [FFmpeg-devel] [PATCH 3/3] http: fix memory leak in parse_cookie.

[Richard Shaffer]

On Thu, Jan 11, 2018 at 2:49 PM, Moritz Barsnick  wrote:

> On Thu, Jan 11, 2018 at 14:28:52 -0800, rshaf...@tunein.com wrote:
>
> > [PATCH 3/3] http: fix memory leak in parse_cookie.g
>
> > This is my first time contributing.
>
> Where are the other two of the three ("3/3") patches then? (Not
> important, but it's confusing on the mailing list.)
>

I apologize for the 3/3 in the subject. Patches 1 and 2 deal with something
else, and I'll submit those for review later.


> > The problem and the solution seem pretty straightforward, but if I'm
> missing something, please do point me in the right direction.
>
> I believe the way you submitted this, this text would be part of your
> commit message. If you desire to type it into the email, please put it
> below the three dashes:


Good to know. If someone is willing to review the substance of the patch,
hopefully they'll remove my introduction before committing.

> ---
> >  libavformat/http.c | 1 +
> >  1 file changed, 1 insertion(+)
>
> I can't comment on the actual change.
>
> Cheers,
> Moritz
> ___
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel