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

2018-04-11 Thread Michael Niedermayer 
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

2018-04-11 Thread 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] libavformat/aac: Parse all ID3 tags present between ADTS frames

2018-04-09 Thread Mattias Amnefelt 

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

2018-04-04 Thread Mattias Amnefelt 

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,

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

2018-04-04 Thread Mattias Amnefelt 

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

2018-04-03 Thread James Almer 
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-03 Thread Carl Eugen Hoyos 
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

2018-04-03 Thread 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.
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 Thread Carl Eugen Hoyos 
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

2018-04-03 Thread Michael Niedermayer 
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

2018-04-02 Thread 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.


/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,

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

2018-04-02 Thread 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

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

2018-04-02 Thread Mattias Amnefelt 
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,