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

[Hendrik Leppkes]

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.
I don't see the problem. We use this kind of resync in all sorts of
demuxers, because a 12-bit magic number that happens to be all 1's
isn't all that safe from being aliased in other data.

- Hendrik