Re: [FFmpeg-devel] [PATCH] libavformat/aac: Parse all ID3 tags present between ADTS frames
On Wed, Apr 11, 2018 at 12:42:07PM -0700, Richard Shaffer wrote: > 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? i dont think iam maintainer of aac/ac3 but will apply as this was laying around for a while and is rather simple so shouldnt need that lomng for a reply thus it appears noone has a comment ... thanks [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB No snowflake in an avalanche ever feels responsible. -- Voltaire signature.asc Description: PGP signature ___ 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
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] libavformat/aac: Parse all ID3 tags present between ADTS frames
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
Re: [FFmpeg-devel] [PATCH] libavformat/aac: Parse all ID3 tags present between ADTS frames
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 From 1d5ab8e48502c570b562ef4fdb6cf674f1d91e93 Mon Sep 17 00:00:00 2001 From: Mattias Amnefelt Date: Mon, 2 Apr 2018 11:30:40 +0200 Subject: [PATCH] libavformat/aac: Parse all ID3 tags present between ADTS frames Some ADTS streams can have multiple ID3 tags between frames. This change parses all of them, rather than just the first one. Signed-off-by: Mattias Amnefelt --- libavformat/aacdec.c | 14 +- tests/fate/demux.mak | 3 +- tests/ref/fate/adts-id3v2-two-tags-demux | 475 +++ 3 files changed, 486 insertions(+), 6 deletions(-) create mode 100644 tests/ref/fate/adts-id3v2-two-tags-demux diff --git a/libavformat/aacdec.c b/libavformat/aacdec.c index 5ec706bdc7..685458b911 100644 --- a/libavformat/aacdec.c +++ b/libavformat/aacdec.c @@ -154,11 +154,15 @@ static int adts_aac_read_packet(AVFormatContext *s, AVPacket *pkt) { int ret, fsize; -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); +// Parse all the ID3 headers between frames +while (1) { +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) { +continue; +} +} +break; } if (ret < 0) diff --git a/tests/fate/demux.mak b/tests/fate/demux.mak index 306904b9de..ef9e677821 100644 --- a/tests/fate/demux.mak +++ b/tests/fate/demux.mak @@ -1,10 +1,11 @@ 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-adts-id3v2-demux +FATE_SAMPLES_DEMUX-$(call DEMDEC, AAC, AAC) += fate-adts-demux fate-adts-id3v1-demux fate-adts-id3v2-demux fate-adts-id3v2-two-tags-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-adts-id3v2-two-tags-demux: CMD = framecrc -i $(TARGET_SAMPLES)/aac/id3v2_two_tags.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-two-tags-demux b/tests/ref/fate/adts-id3v2-two-tags-demux new file mode 100644 index 00..4fffd2e767 --- /dev/null +++ b/tests/ref/fate/adts-id3v2-two-tags-demux @@ -0,0 +1,475 @@ +#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, 128, 0x23291993 +0, 602112, 602112, 602112, 128, 0x23291993 +0,1204224,1204224, 602112, 128, 0x23291993 +0,1806336,1806336, 602112, 128, 0x23291993 +0,2408448,2408448, 602112, 128, 0x23291993 +0,3010560,3010560, 602112, 128, 0x23291993 +0,3612672,3612672, 602112, 128, 0x23291993 +0,4214784,4214784, 602112, 128, 0x23291993 +0,4816896,4816896, 602112, 128, 0x23291993 +0,5419008,5419008, 602112, 128, 0x23291993 +0,6021120,6021120, 602112, 128, 0x23291993 +0,6623232,6623232, 602112, 128, 0x23291993 +0,7225344,7225344, 602112, 128, 0x23291993 +0,7827456,7827456, 602112, 128,
Re: [FFmpeg-devel] [PATCH] libavformat/aac: Parse all ID3 tags present between ADTS frames
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: 2018-04-03 7:58 GMT+02:00, Mattias Amnefelt : Yes, my feeling was also that it's better to handle this when possible. You are of course correct that the two tags needs to be inbetween frames. Sorry about that, I stripped the sample down too much. I updated with a sample which has two frames. This new sample fails the test without the patch. +fate-adts-id3v2-two-tags-demux: CMD = framecrc -f aac -i $(TARGET_SAMPLES)/aac/id3v2_two_tags.aac -c:a copy 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? /mattiasa ___ 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
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: >>> 2018-04-03 7:58 GMT+02:00, Mattias Amnefelt : Yes, my feeling was also that it's better to handle this when possible. You are of course correct that the two tags needs to be inbetween frames. Sorry about that, I stripped the sample down too much. I updated with a sample which has two frames. This new sample fails the test without the patch. >>> +fate-adts-id3v2-two-tags-demux: CMD = framecrc -f aac -i $(TARGET_SAMPLES)/aac/id3v2_two_tags.aac -c:a copy >>> >>> 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. ___ 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
2018-04-04 3:38 GMT+02:00, James Almer : > On 4/3/2018 10:33 PM, Carl Eugen Hoyos wrote: >> 2018-04-03 7:58 GMT+02:00, Mattias Amnefelt : >>> Yes, my feeling was also that it's better to handle this when possible. >>> >>> You are of course correct that the two tags needs to be inbetween >>> frames. Sorry about that, I stripped the sample down too much. I updated >>> with a sample which has two frames. This new sample fails the test >>> without the patch. >> >>> +fate-adts-id3v2-two-tags-demux: CMD = framecrc -f aac -i >>> $(TARGET_SAMPLES)/aac/id3v2_two_tags.aac -c:a copy >> >> 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 ___ 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
On 4/3/2018 10:33 PM, Carl Eugen Hoyos wrote: > 2018-04-03 7:58 GMT+02:00, Mattias Amnefelt : >> Yes, my feeling was also that it's better to handle this when possible. >> >> You are of course correct that the two tags needs to be inbetween >> frames. Sorry about that, I stripped the sample down too much. I updated >> with a sample which has two frames. This new sample fails the test >> without the patch. > >> +fate-adts-id3v2-two-tags-demux: CMD = framecrc -f aac -i >> $(TARGET_SAMPLES)/aac/id3v2_two_tags.aac -c:a copy > > 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. If that's not the case with this new sample then i agree with you it's not a good idea. ___ 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
2018-04-03 7:58 GMT+02:00, Mattias Amnefelt : > Yes, my feeling was also that it's better to handle this when possible. > > You are of course correct that the two tags needs to be inbetween > frames. Sorry about that, I stripped the sample down too much. I updated > with a sample which has two frames. This new sample fails the test > without the patch. > +fate-adts-id3v2-two-tags-demux: CMD = framecrc -f aac -i > $(TARGET_SAMPLES)/aac/id3v2_two_tags.aac -c:a copy 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 ___ 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
On Tue, Apr 03, 2018 at 07:58:53AM +0200, Mattias Amnefelt wrote: > Yes, my feeling was also that it's better to handle this when possible. > > You are of course correct that the two tags needs to be inbetween frames. > Sorry about that, I stripped the sample down too much. I updated with a > sample which has two frames. This new sample fails the test without the > patch. > > /mattiasa > > On 2018-04-02 23:25, Richard Shaffer wrote: > >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 > > > libavformat/aacdec.c | 14 > tests/fate/demux.mak |3 > tests/ref/fate/adts-id3v2-two-tags-demux | 475 > +++ > 3 files changed, 486 insertions(+), 6 deletions(-) > 95d2c77bc7fa82503377e09da98fe17e1b3eb302 > 0001-libavformat-aac-Parse-all-ID3-tags-present-between-A.patch > From 8d587983b6cc5c535e29f0898d4cac433cd0a609 Mon Sep 17 00:00:00 2001 > From: Mattias Amnefelt > Date: Mon, 2 Apr 2018 11:30:40 +0200 > Subject: [PATCH] libavformat/aac: Parse all ID3 tags present between ADTS > frames > > Some ADTS streams can have multiple ID3 tags between frames. This > change parses all of them, rather than just the first one. uploaded aac fate sample the patch should be ok thx [...] -- Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB It is what and why we do it that matters, not just one of them. signature.asc Description: PGP signature ___ 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
Yes, my feeling was also that it's better to handle this when possible. You are of course correct that the two tags needs to be inbetween frames. Sorry about that, I stripped the sample down too much. I updated with a sample which has two frames. This new sample fails the test without the patch. /mattiasa On 2018-04-02 23:25, Richard Shaffer wrote: 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 From 8d587983b6cc5c535e29f0898d4cac433cd0a609 Mon Sep 17 00:00:00 2001 From: Mattias Amnefelt Date: Mon, 2 Apr 2018 11:30:40 +0200 Subject: [PATCH] libavformat/aac: Parse all ID3 tags present between ADTS frames Some ADTS streams can have multiple ID3 tags between frames. This change parses all of them, rather than just the first one. Signed-off-by: Mattias Amnefelt --- libavformat/aacdec.c | 14 +- tests/fate/demux.mak | 3 +- tests/ref/fate/adts-id3v2-two-tags-demux | 475 +++ 3 files changed, 486 insertions(+), 6 deletions(-) create mode 100644 tests/ref/fate/adts-id3v2-two-tags-demux diff --git a/libavformat/aacdec.c b/libavformat/aacdec.c index 5ec706bdc7..685458b911 100644 --- a/libavformat/aacdec.c +++ b/libavformat/aacdec.c @@ -154,11 +154,15 @@ static int adts_aac_read_packet(AVFormatContext *s, AVPacket *pkt) { int ret, fsize; -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); +// Parse all the ID3 headers between frames +while (1) { +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) { +continue; +} +} +break; } if (ret < 0) diff --git a/tests/fate/demux.mak b/tests/fate/demux.mak index 306904b9de..fc581f81ad 100644 --- a/tests/fate/demux.mak +++ b/tests/fate/demux.mak @@ -1,10 +1,11 @@ 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-adts-id3v2-demux +FATE_SAMPLES_DEMUX-$(call DEMDEC, AAC, AAC) += fate-adts-demux fate-adts-id3v1-demux fate-adts-id3v2-demux fate-adts-id3v2-two-tags-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-adts-id3v2-two-tags-demux: CMD = framecrc -f aac -i $(TARGET_SAMPLES)/aac/id3v2_two_tags.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-two-tags-demux b/tests/ref/fate/adts-id3v2-two-tags-demux new file mode 100644 index 00..4fffd2e767 --- /dev/null +++ b/tests/ref/fate/adts-id3v2-two-tags-demux @@ -0,0 +1,475 @@ +#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, 128, 0x23291993 +0, 602112, 602112, 602112, 128, 0x23291993 +0,1204224,1204224, 602112, 128, 0x23291993 +0,1806336,1806336, 602112, 128, 0x23291993 +0,2408448,2408448, 602112, 1
Re: [FFmpeg-devel] [PATCH] libavformat/aac: Parse all ID3 tags present between ADTS frames
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
[FFmpeg-devel] [PATCH] libavformat/aac: Parse all ID3 tags present between ADTS frames
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 From 2f681aa27c216a1aa8d8f2b643289dbf087de306 Mon Sep 17 00:00:00 2001 From: Mattias Amnefelt Date: Mon, 2 Apr 2018 11:30:40 +0200 Subject: [PATCH] libavformat/aac: Parse all ID3 tags present between ADTS frames Some ADTS streams can have multiple ID3 tags between frames. This change parses all of them, rather than just the first one. Signed-off-by: Mattias Amnefelt --- libavformat/aacdec.c | 14 +- tests/fate/demux.mak | 3 +- tests/ref/fate/adts-id3v2-two-tags-demux | 241 +++ 3 files changed, 252 insertions(+), 6 deletions(-) create mode 100644 tests/ref/fate/adts-id3v2-two-tags-demux diff --git a/libavformat/aacdec.c b/libavformat/aacdec.c index 5ec706bdc7..685458b911 100644 --- a/libavformat/aacdec.c +++ b/libavformat/aacdec.c @@ -154,11 +154,15 @@ static int adts_aac_read_packet(AVFormatContext *s, AVPacket *pkt) { int ret, fsize; -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); +// Parse all the ID3 headers between frames +while (1) { +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) { +continue; +} +} +break; } if (ret < 0) diff --git a/tests/fate/demux.mak b/tests/fate/demux.mak index 306904b9de..fc581f81ad 100644 --- a/tests/fate/demux.mak +++ b/tests/fate/demux.mak @@ -1,10 +1,11 @@ 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-adts-id3v2-demux +FATE_SAMPLES_DEMUX-$(call DEMDEC, AAC, AAC) += fate-adts-demux fate-adts-id3v1-demux fate-adts-id3v2-demux fate-adts-id3v2-two-tags-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-adts-id3v2-two-tags-demux: CMD = framecrc -f aac -i $(TARGET_SAMPLES)/aac/id3v2_two_tags.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-two-tags-demux b/tests/ref/fate/adts-id3v2-two-tags-demux new file mode 100644 index 00..61463f4996 --- /dev/null +++ b/tests/ref/fate/adts-id3v2-two-tags-demux @@ -0,0 +1,241 @@ +#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, 128, 0x23291993 +0, 602112, 602112, 602112, 128, 0x23291993 +0,1204224,1204224, 602112, 128, 0x23291993 +0,1806336,1806336, 602112, 128, 0x23291993 +0,2408448,2408448, 602112, 128, 0x23291993 +0,3010560,3010560, 602112, 128, 0x23291993 +0,3612672,3612672, 602112, 128, 0x23291993 +0,4214784,4214784, 602112, 128, 0x23291993 +0,4816896,4816896, 602112, 128, 0x23291993 +0,5419008,5419008, 602112, 128, 0x23291993 +0,6021120,6021120, 602112, 128, 0x23291993 +0,6623232,6623232, 602112, 128, 0x23291993 +0,7225344,7225344, 602112, 128, 0x23291993 +0,7827456,7827456, 602112, 128, 0x23291993 +0,8429568,8429568, 602112, 128, 0x23291993 +0,9031680,9031680, 602112, 128, 0x23291993 +0,9633792,9633792, 602112, 128, 0x23291993 +0, 10235904, 10235904, 602112, 128, 0x23291993 +0, 10838016, 10838016, 602112, 128, 0x23291993 +0, 11440128, 11440128, 602112, 128, 0x23291993 +0, 12042240, 12042240, 602112, 128, 0x23291993 +0, 12644352, 12644352, 602112, 128, 0x23291993 +0, 13246464, 13246464, 602112, 128, 0x23291993 +0, 13848576, 13848576, 602112, 128, 0x23291993 +0, 14450688, 14450688, 602112, 128, 0x23291993 +0, 15052800, 15052800, 602112, 128, 0x23291993 +0, 15654912, 15654912, 602112, 128, 0x23291993 +0, 16257024, 16257024, 602112, 128