[FFmpeg-devel,2/2] avformat/movenc: support Opus packets with more than 60ms of audio when writing the Sample Group Description Box

Submitted by James Almer on Aug. 23, 2018, 10:17 p.m.

Details

Message ID 20180823221742.8236-2-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer Aug. 23, 2018, 10:17 p.m.
Fixes assertion failures when trying to mux such streams.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavformat/movenc.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

Carl Eugen Hoyos Aug. 24, 2018, 10:19 a.m.
2018-08-24 0:17 GMT+02:00, James Almer <jamrial@gmail.com>:
> Fixes assertion failures when trying to mux such streams.

Shouldn't this be 1/2?

And does this assert now for libavformat users that use
new libopus (but not libavcodec) or do I misunderstand?

Carl Eugen
James Almer Aug. 24, 2018, 3:31 p.m.
On 8/24/2018 7:19 AM, Carl Eugen Hoyos wrote:
> 2018-08-24 0:17 GMT+02:00, James Almer <jamrial@gmail.com>:
>> Fixes assertion failures when trying to mux such streams.
> 
> Shouldn't this be 1/2?
> 
> And does this assert now for libavformat users that use
> new libopus (but not libavcodec) or do I misunderstand?

This asserts for any stream with >= 80ms packets. It doesn't need to be
a direct encode from the libopus wrapper, since it can also happen
during be a remux. So the order these two patches are committed doesn't
really matter.

But if you prefer, i can swap them before pushing.

> 
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Carl Eugen Hoyos Aug. 24, 2018, 3:33 p.m.
2018-08-24 17:31 GMT+02:00, James Almer <jamrial@gmail.com>:
> On 8/24/2018 7:19 AM, Carl Eugen Hoyos wrote:
>> 2018-08-24 0:17 GMT+02:00, James Almer <jamrial@gmail.com>:
>>> Fixes assertion failures when trying to mux such streams.
>>
>> Shouldn't this be 1/2?
>>
>> And does this assert now for libavformat users that use
>> new libopus (but not libavcodec) or do I misunderstand?
>
> This asserts for any stream with >= 80ms packets. It doesn't need to be
> a direct encode from the libopus wrapper, since it can also happen
> during be a remux.

Doesn't this indicate that the assert is wrong?
(That invalid input can trigger the assert)

Sorry if I misunderstand, Carl Eugen
James Almer Aug. 24, 2018, 3:41 p.m.
On 8/24/2018 12:33 PM, Carl Eugen Hoyos wrote:
> 2018-08-24 17:31 GMT+02:00, James Almer <jamrial@gmail.com>:
>> On 8/24/2018 7:19 AM, Carl Eugen Hoyos wrote:
>>> 2018-08-24 0:17 GMT+02:00, James Almer <jamrial@gmail.com>:
>>>> Fixes assertion failures when trying to mux such streams.
>>>
>>> Shouldn't this be 1/2?
>>>
>>> And does this assert now for libavformat users that use
>>> new libopus (but not libavcodec) or do I misunderstand?
>>
>> This asserts for any stream with >= 80ms packets. It doesn't need to be
>> a direct encode from the libopus wrapper, since it can also happen
>> during be a remux.
> 
> Doesn't this indicate that the assert is wrong?
> (That invalid input can trigger the assert)

Invalid input (say, a packet reporting a frame size of the equivalent of
1ms) would assert before and after this patch. Do you consider an assert
that triggers on invalid input to be wrong? I thought that was the
entire point of one...

If you prefer i can remove the entire assert altogether. Or maybe
replace it with a normal check that returns an error instead. Otherwise
I'm not sure what's your concern.
Carl Eugen Hoyos Aug. 24, 2018, 3:47 p.m.
2018-08-24 17:41 GMT+02:00, James Almer <jamrial@gmail.com>:
> On 8/24/2018 12:33 PM, Carl Eugen Hoyos wrote:
>> 2018-08-24 17:31 GMT+02:00, James Almer <jamrial@gmail.com>:
>>> On 8/24/2018 7:19 AM, Carl Eugen Hoyos wrote:
>>>> 2018-08-24 0:17 GMT+02:00, James Almer <jamrial@gmail.com>:
>>>>> Fixes assertion failures when trying to mux such streams.
>>>>
>>>> Shouldn't this be 1/2?
>>>>
>>>> And does this assert now for libavformat users that use
>>>> new libopus (but not libavcodec) or do I misunderstand?
>>>
>>> This asserts for any stream with >= 80ms packets. It doesn't need to be
>>> a direct encode from the libopus wrapper, since it can also happen
>>> during be a remux.
>>
>> Doesn't this indicate that the assert is wrong?
>> (That invalid input can trigger the assert)
>
> Invalid input (say, a packet reporting a frame size of the equivalent of
> 1ms) would assert before and after this patch. Do you consider an assert
> that triggers on invalid input to be wrong?

I wanted to write "definitely", I am a little puzzled now that you seem to
disagree.

Yes, I think so: It probably depends on the definition of "invalid" but
I mean invalid data with valid api usage.

Carl Eugen
James Almer Aug. 24, 2018, 3:53 p.m.
On 8/24/2018 12:47 PM, Carl Eugen Hoyos wrote:
> 2018-08-24 17:41 GMT+02:00, James Almer <jamrial@gmail.com>:
>> On 8/24/2018 12:33 PM, Carl Eugen Hoyos wrote:
>>> 2018-08-24 17:31 GMT+02:00, James Almer <jamrial@gmail.com>:
>>>> On 8/24/2018 7:19 AM, Carl Eugen Hoyos wrote:
>>>>> 2018-08-24 0:17 GMT+02:00, James Almer <jamrial@gmail.com>:
>>>>>> Fixes assertion failures when trying to mux such streams.
>>>>>
>>>>> Shouldn't this be 1/2?
>>>>>
>>>>> And does this assert now for libavformat users that use
>>>>> new libopus (but not libavcodec) or do I misunderstand?
>>>>
>>>> This asserts for any stream with >= 80ms packets. It doesn't need to be
>>>> a direct encode from the libopus wrapper, since it can also happen
>>>> during be a remux.
>>>
>>> Doesn't this indicate that the assert is wrong?
>>> (That invalid input can trigger the assert)
>>
>> Invalid input (say, a packet reporting a frame size of the equivalent of
>> 1ms) would assert before and after this patch. Do you consider an assert
>> that triggers on invalid input to be wrong?
> 
> I wanted to write "definitely", I am a little puzzled now that you seem to
> disagree.
> 
> Yes, I think so: It probably depends on the definition of "invalid" but
> I mean invalid data with valid api usage.
> 
> Carl Eugen

I am completely lost in this discussion since i don't understand your
concern about this at all, so please tell me what you want me to do so
we can move on: Do i commit the patch as is, do i remove the assert line
altogether, or do i replace it with a normal check?
Carl Eugen Hoyos Aug. 24, 2018, 3:56 p.m.
2018-08-24 17:53 GMT+02:00, James Almer <jamrial@gmail.com>:
> On 8/24/2018 12:47 PM, Carl Eugen Hoyos wrote:
>> 2018-08-24 17:41 GMT+02:00, James Almer <jamrial@gmail.com>:
>>> On 8/24/2018 12:33 PM, Carl Eugen Hoyos wrote:
>>>> 2018-08-24 17:31 GMT+02:00, James Almer <jamrial@gmail.com>:
>>>>> On 8/24/2018 7:19 AM, Carl Eugen Hoyos wrote:
>>>>>> 2018-08-24 0:17 GMT+02:00, James Almer <jamrial@gmail.com>:
>>>>>>> Fixes assertion failures when trying to mux such streams.
>>>>>>
>>>>>> Shouldn't this be 1/2?
>>>>>>
>>>>>> And does this assert now for libavformat users that use
>>>>>> new libopus (but not libavcodec) or do I misunderstand?
>>>>>
>>>>> This asserts for any stream with >= 80ms packets. It doesn't need to be
>>>>> a direct encode from the libopus wrapper, since it can also happen
>>>>> during be a remux.
>>>>
>>>> Doesn't this indicate that the assert is wrong?
>>>> (That invalid input can trigger the assert)
>>>
>>> Invalid input (say, a packet reporting a frame size of the equivalent of
>>> 1ms) would assert before and after this patch. Do you consider an assert
>>> that triggers on invalid input to be wrong?
>>
>> I wanted to write "definitely", I am a little puzzled now that you seem to
>> disagree.
>>
>> Yes, I think so: It probably depends on the definition of "invalid" but
>> I mean invalid data with valid api usage.

> I am completely lost in this discussion since i don't understand your
> concern about this at all, so please tell me what you want me to do so
> we can move on: Do i commit the patch as is, do i remove the assert line
> altogether, or do i replace it with a normal check?

I believe the assert should be replaced with a normal check and
return (and/or error message) but I am not sure if I understand
the situation correctly since you seem to disagree.
(For me this appears as a trivial technical issue, a mistake in the
current code, that does not need substantial discussion.)

Carl Eugen
Hendrik Leppkes Aug. 24, 2018, 3:57 p.m.
On Fri, Aug 24, 2018 at 5:53 PM James Almer <jamrial@gmail.com> wrote:
>
> On 8/24/2018 12:47 PM, Carl Eugen Hoyos wrote:
> > 2018-08-24 17:41 GMT+02:00, James Almer <jamrial@gmail.com>:
> >> On 8/24/2018 12:33 PM, Carl Eugen Hoyos wrote:
> >>> 2018-08-24 17:31 GMT+02:00, James Almer <jamrial@gmail.com>:
> >>>> On 8/24/2018 7:19 AM, Carl Eugen Hoyos wrote:
> >>>>> 2018-08-24 0:17 GMT+02:00, James Almer <jamrial@gmail.com>:
> >>>>>> Fixes assertion failures when trying to mux such streams.
> >>>>>
> >>>>> Shouldn't this be 1/2?
> >>>>>
> >>>>> And does this assert now for libavformat users that use
> >>>>> new libopus (but not libavcodec) or do I misunderstand?
> >>>>
> >>>> This asserts for any stream with >= 80ms packets. It doesn't need to be
> >>>> a direct encode from the libopus wrapper, since it can also happen
> >>>> during be a remux.
> >>>
> >>> Doesn't this indicate that the assert is wrong?
> >>> (That invalid input can trigger the assert)
> >>
> >> Invalid input (say, a packet reporting a frame size of the equivalent of
> >> 1ms) would assert before and after this patch. Do you consider an assert
> >> that triggers on invalid input to be wrong?
> >
> > I wanted to write "definitely", I am a little puzzled now that you seem to
> > disagree.
> >
> > Yes, I think so: It probably depends on the definition of "invalid" but
> > I mean invalid data with valid api usage.
> >
> > Carl Eugen
>
> I am completely lost in this discussion since i don't understand your
> concern about this at all, so please tell me what you want me to do so
> we can move on: Do i commit the patch as is, do i remove the assert line
> altogether, or do i replace it with a normal check?

asserts should guard against errors made by developers, invalid
internal API use, verifying assumptions made about how an API is being
used, and things like that.
File data should never be checked with an assert, because you can then
abort the process with a crafted file.

So I would suggest to replace it with a normal if check.

- Hendrik
James Almer Aug. 24, 2018, 4:23 p.m.
On 8/24/2018 12:56 PM, Carl Eugen Hoyos wrote:
> 2018-08-24 17:53 GMT+02:00, James Almer <jamrial@gmail.com>:
>> On 8/24/2018 12:47 PM, Carl Eugen Hoyos wrote:
>>> 2018-08-24 17:41 GMT+02:00, James Almer <jamrial@gmail.com>:
>>>> On 8/24/2018 12:33 PM, Carl Eugen Hoyos wrote:
>>>>> 2018-08-24 17:31 GMT+02:00, James Almer <jamrial@gmail.com>:
>>>>>> On 8/24/2018 7:19 AM, Carl Eugen Hoyos wrote:
>>>>>>> 2018-08-24 0:17 GMT+02:00, James Almer <jamrial@gmail.com>:
>>>>>>>> Fixes assertion failures when trying to mux such streams.
>>>>>>>
>>>>>>> Shouldn't this be 1/2?
>>>>>>>
>>>>>>> And does this assert now for libavformat users that use
>>>>>>> new libopus (but not libavcodec) or do I misunderstand?
>>>>>>
>>>>>> This asserts for any stream with >= 80ms packets. It doesn't need to be
>>>>>> a direct encode from the libopus wrapper, since it can also happen
>>>>>> during be a remux.
>>>>>
>>>>> Doesn't this indicate that the assert is wrong?
>>>>> (That invalid input can trigger the assert)
>>>>
>>>> Invalid input (say, a packet reporting a frame size of the equivalent of
>>>> 1ms) would assert before and after this patch. Do you consider an assert
>>>> that triggers on invalid input to be wrong?
>>>
>>> I wanted to write "definitely", I am a little puzzled now that you seem to
>>> disagree.
>>>
>>> Yes, I think so: It probably depends on the definition of "invalid" but
>>> I mean invalid data with valid api usage.
> 
>> I am completely lost in this discussion since i don't understand your
>> concern about this at all, so please tell me what you want me to do so
>> we can move on: Do i commit the patch as is, do i remove the assert line
>> altogether, or do i replace it with a normal check?
> 
> I believe the assert should be replaced with a normal check and
> return (and/or error message) but I am not sure if I understand
> the situation correctly since you seem to disagree.
> (For me this appears as a trivial technical issue, a mistake in the
> current code, that does not need substantial discussion.)

Changed and pushed. Thanks.
Nicolas George Aug. 24, 2018, 4:44 p.m.
James Almer (2018-08-24):
> Invalid input (say, a packet reporting a frame size of the equivalent of
> 1ms) would assert before and after this patch. Do you consider an assert
> that triggers on invalid input to be wrong? I thought that was the
> entire point of one...

Assert failures cause an immediate exit and core dump. If a crafted
input can trigger a failure and crash the application, that is a serious
problem. Asserts are there to catch bugs.

> If you prefer i can remove the entire assert altogether. Or maybe
> replace it with a normal check that returns an error instead.

If you just remove the assert, then the invalid value is passed to the
rest of the code. You know better than me how it will react to it.

Regards,

Patch hide | download patch | download mbox

diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index 8ad7026741..1b1d765f7d 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -2365,9 +2365,8 @@  static int mov_preroll_write_stbl_atoms(AVIOContext *pb, MOVTrack *track)
                decoded. */
             if (roll_samples_remaining > 0)
                 distance = 0;
-            /* Verify distance is a minimum of 2 (60ms) packets and a maximum of
-               32 (2.5ms) packets. */
-            av_assert0(distance == 0 || (distance >= 2 && distance <= 32));
+            /* Verify distance is a maximum of 32 (2.5ms) packets. */
+            av_assert0(distance <= 32);
             if (i && distance == sgpd_entries[entries].roll_distance) {
                 sgpd_entries[entries].count++;
             } else {