Hey Emil,
On 9 March 2017 at 22:57, Emil Velikov <emil.l.veli...@gmail.com> wrote:
> So was feeling a bit bored and decided to remind myself about all the
> crazy formats out there :-)
>
> I believe you've got a couple of mistakes, plus there's a couple of
> questions/ideas below.
> Note that handful of those are inspired by now format info is stored in DRM.
Great, thanks for taking a look!
> On 3 March 2017 at 23:05, Daniel Stone <dani...@collabora.com> wrote:
>> --- /dev/null
>> +++ b/libweston/pixel-formats.c
>
>
> +#include <drm/drm_fourcc.h>
> Err... please don't.
> This will include the kernel copy of the header while we want to use
> the libdrm one -> <drm_fourcc.h>
Good point indeed.
>> +static const struct pixel_format_info pixel_format_table[] = {
>> + {
>> + .format = DRM_FORMAT_VYUY,
>> + .sampler_type = EGL_TEXTURE_Y_XUXV_WL,
>> + .num_planes = 1,
>> + .luma_chroma_order = ORDER_CHROMA_LUMA,
>> + .chroma_order = ORDER_VU,
> All of these four should all have hsub = 2, afaict. The only
> difference between them is the order in which the components are
> stored.
Right you are, I think.
>> + },
>> + {
>> + /* our one format with an alpha channel and no opaque equiv;
>> + * also cannot be trivially converted to RGB without writing
>> + * a new shader in gl-renderer */
>> + .format = DRM_FORMAT_AYUV,
>> + .sampler_type = EGL_TEXTURE_Y_XUXV_WL,
>> + .num_planes = 1,
>> + },
> According to WL_bind_wayland_display and the i965/i915 drivers
> (afaict) above five should have two planes. Then again, the FOURCC and
> DRM formats are pretty clear that we're talking about a packed format
> in a single plain... Did you test these, do we have a spec bug, is the
> i915/i965 iimplementation working/been tested ?
No, they're all right, in their own way. ;) In terms of storage, the
buffers are stored purely in one plane, containing (bytewise)
YUYV/etc, as encoded in the FourCC. However, to use it from GL, you
need to bind the one plane to two samplers: one for luma and one for
chroma. Given that the luma and chroma have different subsampling
values, you can't bind them to the same sampler. So I think this is
correct here.
> Seems like DRM_FORMAT_AYUV is going to act strange with the
> pixel_format_is_opaque() helper. As-in latter will return true,
> despite the format has alpha.
Indeed. I'd be tempted to just delete AYUV tbh.
>> + {
>> + .format = DRM_FORMAT_NV16,
>> + .sampler_type = EGL_TEXTURE_Y_UV_WL,
>> + .num_planes = 2,
>> + .hsub = 2,
>> + .vsub = 1,
>> + },
>> + {
>> + .format = DRM_FORMAT_NV61,
>> + .sampler_type = EGL_TEXTURE_Y_UV_WL,
>> + .num_planes = 2,
>> + .chroma_order = ORDER_VU,
>> + .hsub = 2,
>> + .vsub = 1,
>> + },
> Might be the only person, who finds the mixed use of 0 and 1 as
> [hv]sub sampling confusing.
> Strictly peaking 1x1 is equivalent no subsampling, hence why we have
> 0's below (NV24/42). At the same time, for the single plain packed
> formats we only specify the hsub, even though we should also mention
> vsub [=1].
>
> For simplicity (?) the kernel provides 1, when there's no
> {v,h}sub-sampling. Worth using same the approach here ?
Yeah, consistency would probably be good here. I'll see what I can do.
>> +struct pixel_format_info {
>
>> + /** How the format should be sampled, expressed in terms of tokens
>> + * from the EGL_WL_bind_wayland_display extension. If not set,
>> + * assumed to be either RGB or RGBA, depending on whether or not
>> + * the format contains an alpha channel. */
>> + uint32_t sampler_type;
>> +
> If we have special field say "is_rgb_format" one can correctly deduct
> the sampler_type. I will also help with the "DRM_FORMAT_AYUV does not
> have alpha?" question above.
> - RGB formats - has alpha - RGBA, RGB otherwise
Yeah, that's good to have indeed.
> - YUV formats - 1/2/3 num_planes ->
> EGL_TEXTURE_Y_XUXV_WL/EGL_TEXTURE_Y_XUXV_WL/EGL_TEXTURE_Y_XUXV_WL
> respectively
Well, you still need the chroma order to figure out if it's XUXV or
XVXU; and did you mean to paste the same sampler type three times? ;)
I get the point though. The idea was for the combination of
ORDER_{LUMA_CHROMA,CHROMA_LUMA} and ORDER_{UV,VU}, as well as
num_planes, to uniquely identify the sampler type.
> Considering DRM_IOCTL_MODE_ADDFB2 was introduced with kernel 3.3
> (2012-03-18) do we really want to have the depth/bpp scruft ?
> DRM_IOCTL_MODE_ADDFB supported only 5 formats from this table.
Perhaps not, but I'd like to bin that in a follow-up patch to all
these; doing it now would require removing features from the DRM
backend which people may somehow still be using. Stranger things have
happened. I'd be much more comfortable if I could do it standalone.
I'll send a respin of this next week.
Cheers,
Daniel
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
