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

[James Almer]

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?

> 
> This still allows you to skip garbage in the middle  and resume
> decoding afterwards.
> 
> - Hendrik
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>