search

chromeos / cros-codecs

BSD 3-Clause "New" or "Revised" License
30 stars 11 forks source link

Add film grain support in VA-API #61

Open dwlsalmeida opened 11 months ago

dwlsalmeida commented 11 months ago

This PR adds film grain support to the VA-API code. It does so by first adding a new type of handle, and then making use of that handle in the AV1 VA-API decoder.

With this PR, all our film grain tests go from crashing to failing on Intel Gen12. This is a limitation of their implementation, which is not spec compliant and will also fail when using GStreamer.

I hear it should be good on Intel 13th gen onwards, but I do not have hardware to confirm. Meanwhile, users with unsupported hw (i.e. gen <=12) will get Intel's own implementation, which should be indistinguishable from the spec version for a human eye anyways.

This is built on top of #59, since it touches some bits that were changed in that PR as well, so it has the same dependencies as that PR.

@Gnurou Please test this on your end as well!

A late merry Christmas and a happy new year :)

dwlsalmeida commented 11 months ago

@Gnurou I noticed that you're probably in the middle of rebasing #59, so I will leave this bit of a mess for now. Let me know when you're done and I can rebase on top of your work.

Gnurou commented 9 months ago

Note to self: all CLs but the last two ones have been merged. The remaining CLs will need a rebase due to many conflicts (thankfully they are not too big) but we may also want to reconsider the workflow as I'm not too keen about turning the VAAPI handle into an enum just for this special case.

dwlsalmeida commented 9 months ago

@Gnurou having an extra surface is also important for other contexts, like using the preprocessor, which is why GStreamer has the same design

@ndufresne correct me if I am wrong

ndufresne commented 9 months ago

Correct, filmgrain is a postprocessing that must not be applied to reference frames. Though VA itself have limited inline postprocessing capabilities (which would require this). Vulkan Video fixes this issue.

Encoders on the other end, always needs an extra set of surfaces, even when no preprocessing is taking place. This is to store the reconstruction frames.

Gnurou commented 9 months ago

Yeah I am not against the extra surface which is indeed a necessity, just wondering if we could avoid the enum on the handle. I need to look at this more closely, but the general principle is of course good (and needed for compliance anyway :))