search

chromeos / cros-codecs

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

Improve JVT-FR-EXT test scores #27

Closed dwlsalmeida closed 1 year ago

dwlsalmeida commented 1 year ago

This PR improves the score to 42/61 on Intel.

Other issues include:

a) the apparent lack of support for HIGH10 on the Intel driver. b) https://github.com/intel/media-driver/issues/1683, but for H.264, which makes vaEndPicture return VA_INVALID_PARAMETER on the Intel driver.

None of these things can be worked from cros-codecs itself.

dwlsalmeida commented 1 year ago

@Gnurou what do you think about h264: parser: add support to High10 ?

We should add the parser fix, for sure, but I am unsure whether we should map that to VAProfileHigh. I can't find VA_RT_FORMAT_420_10 for that profile in the Intel driver, for any of their SKUs. The situation in Mesa is of course worse.

Maybe we should just reject that here: https://github.com/chromeos/cros-codecs/blob/main/src/decoder/stateless/h264/vaapi.rs#L45

OTOH, https://github.com/chromeos/cros-codecs/blob/main/src/decoder/stateless/h264/vaapi.rs#L66 is another instance of this problem, yet apparently that works for some hardware out there.

My point being, maybe we should merge this patch and let it crash in vaCreateConfig if the rt_format is not supported by the driver? The error message (VA_UNSUPPORTED_RT_FORMAT) is quite clear...

dwlsalmeida commented 1 year ago

Fixes #26 to the extent possible.

Gnurou commented 1 year ago

@Gnurou what do you think about h264: parser: add support to High10 ?

I agree that the profile should appear in the parser. Regarding the mapping in the VAAPI backend, I am less sure - if we have evidence that this works on some hardware, I'm all for it (that's why I did https://github.com/chromeos/cros-codecs/blob/main/src/decoder/stateless/h264/vaapi.rs#L66) but here it doesn't look like we have that here? OTOH I don't see a better VAAPI profile to map to, so maybe that profile is just unsupported in VAAPI at the moment?

I'm not quite sure but we are going to get an error anyway, so let's merge this for now. :+1: