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

[James Almer]

On 8/24/2018 12:56 PM, Carl Eugen Hoyos wrote:
> 2018-08-24 17:53 GMT+02:00, James Almer <jamrial at gmail.com>:
>> On 8/24/2018 12:47 PM, Carl Eugen Hoyos wrote:
>>> 2018-08-24 17:41 GMT+02:00, James Almer <jamrial at gmail.com>:
>>>> On 8/24/2018 12:33 PM, Carl Eugen Hoyos wrote:
>>>>> 2018-08-24 17:31 GMT+02:00, James Almer <jamrial at gmail.com>:
>>>>>> On 8/24/2018 7:19 AM, Carl Eugen Hoyos wrote:
>>>>>>> 2018-08-24 0:17 GMT+02:00, James Almer <jamrial at 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.