[vlc-devel] [PATCH 2/4] direct3d11: handle output to a SMPTE ST 2084 display

[Filip Roséen]

Hi Steve,

On 2017-03-07 08:31, Steve Lhomme wrote:

> On Mon, Mar 6, 2017 at 4:24 PM, Filip Roséen <filip at atch.se> wrote:
> > On 2017-03-06 16:09, Steve Lhomme wrote:
> >  +                    /* Linear to ST2084 */
> >  +                    psz_src_transform =
> >  +                           "const float m1 = 2610.0 / (4096.0 * 4);\
> >  +                            const float m2 = (2523.0 / 4096.0) * 128.0;\
> >  +                            const float c1 = 3424.0 / 4096.0;\
> >  +                            const float c2 = (2413.0 / 4096.0) * 32.0;\
> >  +                            const float c3 = (2392.0 / 4096.0) * 32.0;\
> >  +                            \
> >
> > Given that the above constants are also declared in patch 1/4, would it not
> > make sense to introduce a common helper for both cases?
> 
> Yeah, it will make the things a little less readable though.

Naming the constants `ST2084_$x` where `$x` is each entity in `{ m1,
m2, c1, c2, c3 }` would, at least to me, make the code more readable
as it would make the magic constants disappear from the calculations,
while also making it easier to maintain in the future.

Or even better, introduce two arrays `float const ST2084_m[2]` and
`float const ST2084_c[3]`, either on their own or wrapped in a
suitably named struct (with anonymous type).

But currently I guess it does not matter much, I just thought I'd
share my immediate reaction when reading the patch.

> >  +                            rgb = pow(rgb, m1);\
> >  +                            rgb = (c1 + c2 * rgb) / (1 + c3 * rgb);\
> >  +                            rgb = pow(rgb, m3);\
> >
> > NITPICK: Is it by intention that pow, together with double-literals are
> > used, while the variables in question are float? If floats are really
> > desired, powf and .0f seems more suitable to declare such intent.
> 
> This is not C or C++ code, it's HLSL which doesn't have double nor powf().
> https://msdn.microsoft.com/en-us/library/windows/desktop/bb509636(v=vs.85).aspx

Oh yeah right, I kind of assumed that the symmetry would have both
functions; but now that you said it I realize my mistake.

Thank you, and I will read the linked specification!

Best Regards,\
Filip
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170307/9b0f851b/attachment.html>