| Message ID | 20180202023745.68519-1-rshaffer@tunein.com |
|---|---|
| State | Accepted |
| Commit | e023334661e6eafcf638ffc2a780fd495fc25ec9 |
| Headers | show |
On Thu, 1 Feb 2018 18:37:45 -0800 rshaffer@tunein.com wrote: > From: Richard Shaffer <rshaffer@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(&ioctx, pkt->data, pkt->size, 0, NULL, NULL, NULL, NULL); > + ff_id3v2_read_dict(&ioctx, &metadata, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta); > + if ((ret = ff_id3v2_parse_priv_dict(&metadata, &id3v2_extra_meta)) < 0) > + goto error; > + > + if (metadata) { > + if ((ret = av_dict_copy(&s->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. > + s->event_flags |= AVFMT_EVENT_FLAG_METADATA_UPDATED; > + } > + > +error: > + av_packet_unref(pkt); > + ff_id3v2_free_extra_meta(&id3v2_extra_meta); > + av_dict_free(&metadata); > + > + 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); > I think that's the right approach. The demuxer should filter out such tags, and exporting them to the API user is a good idea too.
On Thu, Feb 1, 2018 at 10:02 PM, wm4 <nfxjfg@googlemail.com> wrote: > On Thu, 1 Feb 2018 18:37:45 -0800 > rshaffer@tunein.com wrote: > >> From: Richard Shaffer <rshaffer@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(&ioctx, pkt->data, pkt->size, 0, NULL, NULL, NULL, NULL); >> + ff_id3v2_read_dict(&ioctx, &metadata, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta); >> + if ((ret = ff_id3v2_parse_priv_dict(&metadata, &id3v2_extra_meta)) < 0) >> + goto error; >> + >> + if (metadata) { >> + if ((ret = av_dict_copy(&s->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(&id3v2_extra_meta); >> + av_dict_free(&metadata); >> + >> + 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); >> > > I think that's the right approach. The demuxer should filter out such > tags, and exporting them to the API user is a good idea too. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Fri, 2 Feb 2018 00:06:47 -0800 Richard Shaffer <rshaffer@tunein.com> wrote: > On Thu, Feb 1, 2018 at 10:02 PM, wm4 <nfxjfg@googlemail.com> wrote: > > On Thu, 1 Feb 2018 18:37:45 -0800 > > rshaffer@tunein.com wrote: > > > >> From: Richard Shaffer <rshaffer@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(&ioctx, pkt->data, pkt->size, 0, NULL, NULL, NULL, NULL); > >> + ff_id3v2_read_dict(&ioctx, &metadata, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta); > >> + if ((ret = ff_id3v2_parse_priv_dict(&metadata, &id3v2_extra_meta)) < 0) > >> + goto error; > >> + > >> + if (metadata) { > >> + if ((ret = av_dict_copy(&s->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. Feels a bit strange to me. But maybe it's better to be consistent with existing behavior, and also it's likely that tag updates use the same tag keys. So I'm fine with this currently.
On 2/1/2018 11:37 PM, rshaffer@tunein.com wrote: > From: Richard Shaffer <rshaffer@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
On Fri, Feb 2, 2018 at 5:52 AM, James Almer <jamrial@gmail.com> wrote: > On 2/1/2018 11:37 PM, rshaffer@tunein.com wrote: >> From: Richard Shaffer <rshaffer@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
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 <rshaffer@tunein.com> wrote: > On Fri, Feb 2, 2018 at 5:52 AM, James Almer <jamrial@gmail.com> wrote: >> On 2/1/2018 11:37 PM, rshaffer@tunein.com wrote: >>> From: Richard Shaffer <rshaffer@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
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 <rshaffer@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 <rshaffer@tunein.com> wrote: >> On Fri, Feb 2, 2018 at 5:52 AM, James Almer <jamrial@gmail.com> wrote: >>> On 2/1/2018 11:37 PM, rshaffer@tunein.com wrote: >>>> From: Richard Shaffer <rshaffer@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
On Fri, 9 Feb 2018 12:54:29 -0800 Richard Shaffer <rshaffer@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?
On Fri, Feb 9, 2018 at 1:21 PM, wm4 <nfxjfg@googlemail.com> wrote: > On Fri, 9 Feb 2018 12:54:29 -0800 > Richard Shaffer <rshaffer@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
On Thu, 1 Feb 2018 18:37:45 -0800 rshaffer@tunein.com wrote: > From: Richard Shaffer <rshaffer@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(&ioctx, pkt->data, pkt->size, 0, NULL, NULL, NULL, NULL); > + ff_id3v2_read_dict(&ioctx, &metadata, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta); > + if ((ret = ff_id3v2_parse_priv_dict(&metadata, &id3v2_extra_meta)) < 0) > + goto error; > + > + if (metadata) { > + if ((ret = av_dict_copy(&s->metadata, metadata, 0)) < 0) > + goto error; > + s->event_flags |= AVFMT_EVENT_FLAG_METADATA_UPDATED; > + } > + > +error: > + av_packet_unref(pkt); > + ff_id3v2_free_extra_meta(&id3v2_extra_meta); > + av_dict_free(&metadata); > + > + 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...)
There was only one version of this one. Thanks for applying. On Mon, Feb 12, 2018 at 1:17 PM, wm4 <nfxjfg@googlemail.com> wrote: > On Thu, 1 Feb 2018 18:37:45 -0800 > rshaffer@tunein.com wrote: > >> From: Richard Shaffer <rshaffer@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(&ioctx, pkt->data, pkt->size, 0, NULL, NULL, NULL, NULL); >> + ff_id3v2_read_dict(&ioctx, &metadata, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta); >> + if ((ret = ff_id3v2_parse_priv_dict(&metadata, &id3v2_extra_meta)) < 0) >> + goto error; >> + >> + if (metadata) { >> + if ((ret = av_dict_copy(&s->metadata, metadata, 0)) < 0) >> + goto error; >> + s->event_flags |= AVFMT_EVENT_FLAG_METADATA_UPDATED; >> + } >> + >> +error: >> + av_packet_unref(pkt); >> + ff_id3v2_free_extra_meta(&id3v2_extra_meta); >> + av_dict_free(&metadata); >> + >> + 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
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(&ioctx, pkt->data, pkt->size, 0, NULL, NULL, NULL, NULL); + ff_id3v2_read_dict(&ioctx, &metadata, ID3v2_DEFAULT_MAGIC, &id3v2_extra_meta); + if ((ret = ff_id3v2_parse_priv_dict(&metadata, &id3v2_extra_meta)) < 0) + goto error; + + if (metadata) { + if ((ret = av_dict_copy(&s->metadata, metadata, 0)) < 0) + goto error; + s->event_flags |= AVFMT_EVENT_FLAG_METADATA_UPDATED; + } + +error: + av_packet_unref(pkt); + ff_id3v2_free_extra_meta(&id3v2_extra_meta); + av_dict_free(&metadata); + + 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);
From: Richard Shaffer <rshaffer@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(-)