[FFmpeg-devel] [PATCH] alsdec: channel sorting

[Paul B Mahol]

On 12/21/12, Thilo Borgmann <thilo.borgmann at googlemail.com> wrote:
> Am 21.12.12 19:16, schrieb Paul B Mahol:
>> On 12/21/12, Thilo Borgmann <thilo.borgmann at googlemail.com> wrote:
>>> Am 21.12.12 17:52, schrieb Paul B Mahol:
>>>> On 12/21/12, Paul B Mahol <onemda at gmail.com> wrote:
>>>>> On 12/21/12, Thilo Borgmann <thilo.borgmann at googlemail.com> wrote:
>>>>>> Am 21.12.12 15:05, schrieb Paul B Mahol:
>>>>>>> On 12/21/12, Thilo Borgmann <thilo.borgmann at googlemail.com> wrote:
>>>>>>>> Am 21.12.12 14:21, schrieb Paul B Mahol:
>>>>>>>>> Signed-off-by: Paul B Mahol <onemda at gmail.com>
>>>>>>>>> ---
>>>>>>>>>  libavcodec/alsdec.c | 13 ++++++++++---
>>>>>>>>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>>>>>>>
>>>>>>>> Probably ok. Have you tested and veryfied using the reference
>>>>>>>> encoder/decoder?
>>>>>>>>
>>>>>>>
>>>>>>> Yes, without this patch ffmpeg reports CRC error.
>>>>>>>
>>>>>>
>>>>>>> Though instead of aborting i will just ignore channel sorting if
>>>>>>> invalid
>>>>>>> values
>>>>>>> are encountered.
>>>>>>
>>>
>>>>>> This sounds like a "will do in another patch" ? Currently your patch
>>>>>> returns
>>>>>> INVALIDDATA...?
>>>
>>> see below...
>>>
>>>>>>> [...]
>>>>>>> also reference decoder gives broken output for some 6ch files i
>>>>>>> found
>>>>>>> on
>>>>>>> their
>>>>>>> ftp server.
>>>>>>
>>>>>> Please provide me a link to these files segfaulting the reference
>>>>>> decoder
>>>>>> (which
>>>>>> version do you use btw?)
>>>>>
>>>>> It is not really file which cause segv.
>>>>> How to segfault, latest 23 release:
>>>>> /tmp/mp4alsRM23/bin/{arch}/mp4alsRM23 -m2,1,3,4,7,6 /tmp/out.wav
>>>>> /tmp/out.mp4
>>>>
>>>> The point is that input wav have 6 channels.
>>>
>>> Ok it segfaults for wrong channel numbers then like you already told.
>>>
>>> I think ignoring reordering in this case is reasonable but your patch
>>> does
>>> not
>>> do that, does it?
>>
>> I already comitted one that ignore it.
>
> "Probably ok." combined with more comments does hardly mean to commit this
> patch
> in my eyes. However I ignored that. That you commit something different
> does
> make it worse - let's just say I'm not amused.
>
> I'm still "probably ok" with the patch but I think it should be done
> differently. Please see Log for what I think should be done differently...

Its not obvious for me what should be done differently and what Log
actaully means.

So next time when you say "probably ok" I will ignore that and wil
leave you to commit patches.