| Message ID | 20180823221742.8236-2-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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 >
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
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.
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
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?
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
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
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.
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,
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 {
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(-)