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

[James Almer]

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.

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.

I'll send an updated patch to do a couple extra checks.

> 
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>