| Message ID | 20170904024736.4932-1-jamrial@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
On Sun, 3 Sep 2017, James Almer wrote: > Skip the invalid data in an attempt to find one instead, and continue decoding from there. > > Fixes ticket #6634 If you skipped some broken ADTS frames, which were previously reported errnous, then can you signal that somehow? (E.g. by setting AVFrame->decode_error_flags on the next frame, or adding AV_FRAME_FLAG_CORRUPT flag to the next returned frame?) Thanks, Marton
On 9/5/2017 3:52 PM, Marton Balint wrote: > > On Sun, 3 Sep 2017, James Almer wrote: > >> Skip the invalid data in an attempt to find one instead, and continue >> decoding from there. >> >> Fixes ticket #6634 > > If you skipped some broken ADTS frames, which were previously reported > errnous, then can you signal that somehow? (E.g. by setting > AVFrame->decode_error_flags on the next frame, or adding > AV_FRAME_FLAG_CORRUPT flag to the next returned frame?) The entire point of the custom packet reading function instead of using the raw one was to avoid propagating any kind of data that's not an ADTS frame. This could for example be an id3v1 or APE tag at the end. fate-adts-id3v1-demux tests this (The demuxer skips both garbage at the beginning of the file and the id3v1 tag at the end). If a file has an incomplete ADTS frame but with an intact header we afaics have no way to know it from the demuxer side. We read the amount of bytes the ADTS header reports and propagate it inside a packet. By the time the demuxer tries to read the next frame, if it finds out there's no ADTS header right after the reported end of the previous frame then the only thing it can do is bail out, or skip data until EOF or an ADTS header shows up. Before this patch, the former behavior is what happens. After this patch the demuxer will skip data until either EOF, an arbitrary amount of bytes were read, or an ADTS header shows up.
On Tue, 5 Sep 2017, James Almer wrote: > On 9/5/2017 3:52 PM, Marton Balint wrote: >> >> On Sun, 3 Sep 2017, James Almer wrote: >> >>> Skip the invalid data in an attempt to find one instead, and continue >>> decoding from there. >>> >>> Fixes ticket #6634 >> >> If you skipped some broken ADTS frames, which were previously reported >> errnous, then can you signal that somehow? (E.g. by setting >> AVFrame->decode_error_flags on the next frame, or adding >> AV_FRAME_FLAG_CORRUPT flag to the next returned frame?) > > The entire point of the custom packet reading function instead of using > the raw one was to avoid propagating any kind of data that's not an ADTS > frame. This could for example be an id3v1 or APE tag at the end. > fate-adts-id3v1-demux tests this (The demuxer skips both garbage at the > beginning of the file and the id3v1 tag at the end). > > If a file has an incomplete ADTS frame but with an intact header we > afaics have no way to know it from the demuxer side. We read the amount > of bytes the ADTS header reports and propagate it inside a packet. > By the time the demuxer tries to read the next frame, if it finds out > there's no ADTS header right after the reported end of the previous > frame then the only thing it can do is bail out, or skip data until EOF > or an ADTS header shows up. > Before this patch, the former behavior is what happens. After this patch > the demuxer will skip data until either EOF, an arbitrary amount of > bytes were read, or an ADTS header shows up. I kind of confused this with the aac decoder, sorry. Thanks for explaining, I guess it is OK then. Regards, Marton
On Mon, Sep 4, 2017 at 4:47 AM, James Almer <jamrial@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@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? - Hendrik
On 9/5/2017 5:30 PM, Hendrik Leppkes wrote: > On Mon, Sep 4, 2017 at 4:47 AM, James Almer <jamrial@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@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. > > - Hendrik > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Tue, Sep 5, 2017 at 11:20 PM, James Almer <jamrial@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@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@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. - Hendrik
On 9/5/2017 7:12 PM, Hendrik Leppkes wrote: > On Tue, Sep 5, 2017 at 11:20 PM, James Almer <jamrial@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@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@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. > > - Hendrik > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Wed, Sep 6, 2017 at 12:32 AM, James Almer <jamrial@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@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@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@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
On 9/5/2017 8:47 PM, Hendrik Leppkes wrote: > On Wed, Sep 6, 2017 at 12:32 AM, James Almer <jamrial@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@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@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@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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Wed, Sep 6, 2017 at 2:27 AM, James Almer <jamrial@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@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@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@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@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. This still allows you to skip garbage in the middle and resume decoding afterwards. - Hendrik
On 9/6/2017 6:09 AM, Hendrik Leppkes wrote: > On Wed, Sep 6, 2017 at 2:27 AM, James Almer <jamrial@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@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@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@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@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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel >
On Wed, Sep 6, 2017 at 3:51 PM, James Almer <jamrial@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@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@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@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@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@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
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; + } + + if ((state >> 4) != 0xFFF) + return avio_feof(s->pb) ? AVERROR_EOF : AVERROR_INVALIDDATA; + + return 0; +} + static int adts_aac_read_header(AVFormatContext *s) { AVStream *st; - uint16_t state; + int ret; st = avformat_new_stream(s, NULL); if (!st) @@ -99,16 +118,9 @@ static int adts_aac_read_header(AVFormatContext *s) } // skip data until the first ADTS frame is found - 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; - } - if ((state >> 4) != 0xFFF) - return AVERROR_INVALIDDATA; + ret = resync(s); + if (ret < 0) + return ret; // LCM of all possible ADTS sample rates avpriv_set_pts_info(st, 64, 1, 28224000); @@ -129,8 +141,13 @@ static int adts_aac_read_packet(AVFormatContext *s, AVPacket *pkt) } if ((AV_RB16(pkt->data) >> 4) != 0xfff) { + // try resyncing until we find an ADTS frame. av_packet_unref(pkt); - return AVERROR_INVALIDDATA; + avio_seek(s->pb, -ret, SEEK_CUR); + ret = resync(s); + if (ret < 0) + return ret; + return adts_aac_read_packet(s, pkt); } fsize = (AV_RB32(pkt->data + 3) >> 13) & 0x1FFF;
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@gmail.com> --- libavformat/aacdec.c | 41 +++++++++++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 12 deletions(-)