[FFmpeg-devel] [PATCH] avformat/aacdec: don't immediately abort if an ADTS frame is not found

[Hendrik Leppkes]

On Wed, Sep 6, 2017 at 3:51 PM, James Almer <jamrial at gmail.com> wrote:
> On 9/6/2017 6:09 AM, Hendrik Leppkes wrote:
>> On Wed, Sep 6, 2017 at 2:27 AM, James Almer <jamrial at gmail.com> wrote:
>>> On 9/5/2017 8:47 PM, Hendrik Leppkes wrote:
>>>> On Wed, Sep 6, 2017 at 12:32 AM, James Almer <jamrial at gmail.com> wrote:
>>>>> On 9/5/2017 7:12 PM, Hendrik Leppkes wrote:
>>>>>> On Tue, Sep 5, 2017 at 11:20 PM, James Almer <jamrial at gmail.com> wrote:
>>>>>>> On 9/5/2017 5:30 PM, Hendrik Leppkes wrote:
>>>>>>>> On Mon, Sep 4, 2017 at 4:47 AM, James Almer <jamrial at gmail.com> wrote:
>>>>>>>>> Skip the invalid data in an attempt to find one instead, and continue decoding from there.
>>>>>>>>>
>>>>>>>>> Fixes ticket #6634
>>>>>>>>>
>>>>>>>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>>>>>>>> ---
>>>>>>>>>  libavformat/aacdec.c | 41 +++++++++++++++++++++++++++++------------
>>>>>>>>>  1 file changed, 29 insertions(+), 12 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/libavformat/aacdec.c b/libavformat/aacdec.c
>>>>>>>>> index 364b33404f..7aacc88560 100644
>>>>>>>>> --- a/libavformat/aacdec.c
>>>>>>>>> +++ b/libavformat/aacdec.c
>>>>>>>>> @@ -77,10 +77,29 @@ static int adts_aac_probe(AVProbeData *p)
>>>>>>>>>          return 0;
>>>>>>>>>  }
>>>>>>>>>
>>>>>>>>> +static int resync(AVFormatContext *s)
>>>>>>>>> +{
>>>>>>>>> +    uint16_t state;
>>>>>>>>> +
>>>>>>>>> +    state = avio_r8(s->pb);
>>>>>>>>> +    while (!avio_feof(s->pb) && avio_tell(s->pb) < s->probesize) {
>>>>>>>>> +        state = (state << 8) | avio_r8(s->pb);
>>>>>>>>> +        if ((state >> 4) != 0xFFF)
>>>>>>>>> +            continue;
>>>>>>>>> +        avio_seek(s->pb, -2, SEEK_CUR);
>>>>>>>>> +        break;
>>>>>>>>> +    }
>>>>>>>>> +
>>>>>>>>
>>>>>>>> The ADTS sync code isn't that much of a sync code, maybe it might be
>>>>>>>> more resilient if you try to read the frame size and check if after
>>>>>>>> that the next frame also starts with a valid code?
>>>>>>>
>>>>>>> That will only let me know if at the end of the frame is another frame,
>>>>>>> and not if the frame I'm reading is complete or not.
>>>>>>> Maybe it's complete and right after it there's an id3v1 tag. Maybe it's
>>>>>>> complete and right after it there's garbage. Maybe it's incomplete
>>>>>>> because another ADTS frame started suddenly in the middle of the one i
>>>>>>> tried to read because the source is some weird stream (sample in the
>>>>>>> ticket this fixes), and after reading the reported size of the intended
>>>>>>> frame the demuxer will find itself in the middle of the new one but
>>>>>>> unable to know that's the case.
>>>>>>>
>>>>>>> Really, at the demuxer level i can't do much more than read an ADTS
>>>>>>> header, make sure it's at least seven bytes long and that the reported
>>>>>>> size of the entire frame is bigger than the header size, then make a
>>>>>>> packet out of that. The decoder/parser will handle things from there,
>>>>>>> knowing that at very least for the first few bytes what reaches them is
>>>>>>> an ADTS frame.
>>>>>>>
>>>>>>
>>>>>> We're not talking about validating the ADTS frame, but just making
>>>>>> sure you really "resync" to the start of a frame, and not some
>>>>>> arbitrary random position that just happens to be 0xFFF, because that
>>>>>> code isn't very long or very special.
>>>>>
>>>>> Again, what if there's no new ADTS frame after the supposed end of the
>>>>> one we're reading? How do we interpret that? That the one we read was
>>>>> wrong or that it was right and there simply is no new ADTS frame right
>>>>> after it? How does that help us decide if we propagate it or not?
>>>>>
>>>>> If anything, i could maybe use avpriv_aac_parse_header(). Barely more
>>>>> resilient than just looking for a sync code and the size field being >
>>>>> 7, but it's something i guess.
>>>>
>>>> If there is no two consecutive ADTS frames to be found, just stop sending data.
>>>
>>> We're doing exactly that right now. The point of this patch is to not do
>>> it any longer.
>>>
>>>> I don't see the problem.
>>>
>>> This patch loses its meaning. I have the feeling you're not getting what
>>> I'm trying to do here.
>>> The sample this fixes has several ADTS frames cut at the middle where
>>> another was injected (Seems to be a raw stream dump). At the reported
>>> end of the cut frame there isn't a new sync header, there's data from
>>> the middle of the new unexpected frame. By stopping there, we're barely
>>> decoding 2 seconds of audio, when there's about 19 that can get decoded
>>> if we instead move forward trying to find the start of a new frame.
>>>
>>> Before the commit that put us in our current situation the demuxer would
>>> just send everything and let the parser combine data into hopefully
>>> valid frames. This resulted in things that weren't audio frames being
>>> propagated, like id3v1 or APE tags, or just garbage.
>>
>> I don't see how checking for two consecutive frames would really stop
>> this from decoding further audio?
>> We lost sync, we try to find the next ADTS sync code, then we validate
>> it by reading the frame size and checking if after this frame another
>> one starts - ie. if we're really "synced" to frame starts again.
>
> What if after the frame i'm checking there's an id3v1 or APE tag? Or
> some other kind of file footer? Do i skip what is most likely the last
> valid frame of the file just because another didn't start right after it?

I would rather favor finding a certain frame sync and drop one more
frame in special cases, then potentially sending invalid data because
the ADTS sync code isn't long or very unique, and easily aliased.
I've even had issues with aliasing the full 32-bit DTS sync codes in
DTS audio, so I imagine this happening for ADTS so much more often.

I could turn that around as well, what happens if you find some random
0xFFF data, and the (wrong) length has you reading until in the middle
of the next valid ADTS frame, in which case you also lost that one.

Ultimately its up to you, but outside of special cases I do believe
checking for two consecutive frames like other re-sync code in other
demuxers is the more resilient solution for such a short magic number.

- Hendrik