[vlc-devel] [PATCH 2/2] Add VideoToolbox based decoder

[Felix Paul Kühne]

> On 03 Feb 2015, at 16:53, Rémi Denis-Courmont <remi at remlab.net> wrote:
> 
> Le 2015-02-03 18:40, Felix Paul Kühne a écrit :
>>> On 03 Feb 2015, at 16:38, Rémi Denis-Courmont <remi at remlab.net> wrote:
>>> 
>>> Le 2015-02-03 13:46, Felix Paul Kühne a écrit :
>>>> Supports H264, mp4v, mp1v, mp2v, H263 and DV depending on Darwin
>>>> flavor and version
>>> 
>>> Same concerns as previous version…
>> 
>> Well, it was switched to I420 now and your other concerns were addressed.
> 
> None of the three other comments were addressed. Not that I care about MacOS but if you are going to ignore reviews, I would really prefer that you commit directly so that I don't waste my time.

In fact, I enjoy your reviews and your critique because it leads to better overall code quality. That’s why I send stuff like this for review and I’m happy that you are reviewing it despite it is out of your usual scope.

However, I should have answered to your comments instead of just pushing a modified patch.


>> +            msg_Err(p_dec, "trying to decode MPEG-4 Part 10: profile %zu,
>> level %zu", i_profile, i_level); +
>> +            if (deviceSupports10BitH264()) {
>> +                if (i_profile > 110)
>> +                    return VLC_EGENERIC;
> 
> Unwarranted assumptions about not yet specified profiles AFAICT.

Maybe true, but Apple claims support for all profiles up to 110 on the A7/A8 CPUs included with the latest and the previous generation of iPhone and iPad. This is not supported on OS X or previous iOS devices, where 100 is the limit.

> 
>> +            } else {
>> +                if (i_profile > 100)
>> +                    return VLC_EGENERIC;
>> +            }
> 
> The component depth is specified by the chroma IDC, not the profile.

The original naming was wrong. This check is about profiles, not about chroma.

For chroma checks, we intend to parse the SPS as suggested by you in the other thread. Regarding the further implementation of the module, we don’t care about the input chroma at all as everything is handled by the DSP without letting us know.

> 
>> +
>> +            /* we don't do decoding beyond 4K */
>> +            if (i_level > 51)
>> +                return VLC_EGENERIC;
> 
> This is a sufficient but not necessary condition, AFAIU.

Regrettably, it is necessary because higher levels will lead to decoder errors at a point where we can’t fallback on libavcodec anymore.

Cheers,

Felix