[FFmpeg-devel] avcodec/aac_ac3_parser: account for data already in the parsing buffer

Submitted by Hendrik Leppkes on April 11, 2018, 11:40 a.m.

Details

Message ID 20180411114040.1596-1-h.leppkes@gmail.com
State New
Headers show

Commit Message

Hendrik Leppkes April 11, 2018, 11:40 a.m.
If a frame starts very close to a packet boundary, the start code may
already have been added to the parsing buffer, indicated by a small
negative value of "i", while the header is still being tracked in the
"state" variable.

Reduce the remaining size accordingly, otherwise trying to find the next
frame could skip over the frame header and lump two frames together as
one.
---
 libavcodec/aac_ac3_parser.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Carl Eugen Hoyos April 11, 2018, 6:07 p.m.
2018-04-11 13:40 GMT+02:00, Hendrik Leppkes <h.leppkes@gmail.com>:
> If a frame starts very close to a packet boundary, the start code may
> already have been added to the parsing buffer, indicated by a small
> negative value of "i", while the header is still being tracked in the
> "state" variable.

Do you have a sample?

Carl Eugen
Hendrik Leppkes April 11, 2018, 11:31 p.m.
On Wed, Apr 11, 2018 at 8:07 PM, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> 2018-04-11 13:40 GMT+02:00, Hendrik Leppkes <h.leppkes@gmail.com>:
>> If a frame starts very close to a packet boundary, the start code may
>> already have been added to the parsing buffer, indicated by a small
>> negative value of "i", while the header is still being tracked in the
>> "state" variable.
>
> Do you have a sample?
>

I do, but unless you can tell me how you can get ffmpeg to split
packets from a DVD VOB stream on a fixed arbitrary packet size (2048
bytes, iirc) to cause the issue to appear, I wouldn't know how to
reproduce it from CLI.

- Hendrik
Hendrik Leppkes April 13, 2018, 9:50 a.m.
On Wed, Apr 11, 2018 at 1:40 PM, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
> If a frame starts very close to a packet boundary, the start code may
> already have been added to the parsing buffer, indicated by a small
> negative value of "i", while the header is still being tracked in the
> "state" variable.
>
> Reduce the remaining size accordingly, otherwise trying to find the next
> frame could skip over the frame header and lump two frames together as
> one.
> ---
>  libavcodec/aac_ac3_parser.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/libavcodec/aac_ac3_parser.c b/libavcodec/aac_ac3_parser.c
> index 019074b0dd..7f20626285 100644
> --- a/libavcodec/aac_ac3_parser.c
> +++ b/libavcodec/aac_ac3_parser.c
> @@ -60,6 +60,8 @@ get_next:
>                      s->remaining_size += i;
>                      goto get_next;
>                  }
> +                else if (i < 0)
> +                    s->remaining_size += i;
>              }
>          }
>      }
> --
> 2.16.1.windows.4
>

Ping.
Note that this is somewhat of a regression in AC3 parsing since the
recent AC3+EAC3 combined stream support (the bug existed before, but
that change exposed it).

- Hendrik
Paul B Mahol April 13, 2018, 9:54 a.m.
On 4/13/18, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
> On Wed, Apr 11, 2018 at 1:40 PM, Hendrik Leppkes <h.leppkes@gmail.com>
> wrote:
>> If a frame starts very close to a packet boundary, the start code may
>> already have been added to the parsing buffer, indicated by a small
>> negative value of "i", while the header is still being tracked in the
>> "state" variable.
>>
>> Reduce the remaining size accordingly, otherwise trying to find the next
>> frame could skip over the frame header and lump two frames together as
>> one.
>> ---
>>  libavcodec/aac_ac3_parser.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/libavcodec/aac_ac3_parser.c b/libavcodec/aac_ac3_parser.c
>> index 019074b0dd..7f20626285 100644
>> --- a/libavcodec/aac_ac3_parser.c
>> +++ b/libavcodec/aac_ac3_parser.c
>> @@ -60,6 +60,8 @@ get_next:
>>                      s->remaining_size += i;
>>                      goto get_next;
>>                  }
>> +                else if (i < 0)
>> +                    s->remaining_size += i;
>>              }
>>          }
>>      }
>> --
>> 2.16.1.windows.4
>>
>
> Ping.
> Note that this is somewhat of a regression in AC3 parsing since the
> recent AC3+EAC3 combined stream support (the bug existed before, but
> that change exposed it).

Please add { } brackets around new code.

Patch LGTM otherwise.
Hendrik Leppkes April 15, 2018, 8:31 a.m.
On Fri, Apr 13, 2018 at 11:54 AM, Paul B Mahol <onemda@gmail.com> wrote:
> On 4/13/18, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>> On Wed, Apr 11, 2018 at 1:40 PM, Hendrik Leppkes <h.leppkes@gmail.com>
>> wrote:
>>> If a frame starts very close to a packet boundary, the start code may
>>> already have been added to the parsing buffer, indicated by a small
>>> negative value of "i", while the header is still being tracked in the
>>> "state" variable.
>>>
>>> Reduce the remaining size accordingly, otherwise trying to find the next
>>> frame could skip over the frame header and lump two frames together as
>>> one.
>>> ---
>>>  libavcodec/aac_ac3_parser.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/libavcodec/aac_ac3_parser.c b/libavcodec/aac_ac3_parser.c
>>> index 019074b0dd..7f20626285 100644
>>> --- a/libavcodec/aac_ac3_parser.c
>>> +++ b/libavcodec/aac_ac3_parser.c
>>> @@ -60,6 +60,8 @@ get_next:
>>>                      s->remaining_size += i;
>>>                      goto get_next;
>>>                  }
>>> +                else if (i < 0)
>>> +                    s->remaining_size += i;
>>>              }
>>>          }
>>>      }
>>> --
>>> 2.16.1.windows.4
>>>
>>
>> Ping.
>> Note that this is somewhat of a regression in AC3 parsing since the
>> recent AC3+EAC3 combined stream support (the bug existed before, but
>> that change exposed it).
>
> Please add { } brackets around new code.
>

Applied with that changed.

- Hendrik

Patch hide | download patch | download mbox

diff --git a/libavcodec/aac_ac3_parser.c b/libavcodec/aac_ac3_parser.c
index 019074b0dd..7f20626285 100644
--- a/libavcodec/aac_ac3_parser.c
+++ b/libavcodec/aac_ac3_parser.c
@@ -60,6 +60,8 @@  get_next:
                     s->remaining_size += i;
                     goto get_next;
                 }
+                else if (i < 0)
+                    s->remaining_size += i;
             }
         }
     }