[Ozone/Linux] Support VA-API on Linux Ozone/Wayland [chromium/src : main]
Jianhui J Dai (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge, Maksim Sisov, Robert Kroeger.
2 comments:
Patchset:
Update new patch set to use ozone property for VA-API, also the description in commit message.
File media/gpu/vaapi/vaapi_wrapper.cc:
Patch Set #13, Line 758:
if (ui::GetOzonePlatformId() == ui::kPlatformX11)Thanks. I will look into the ozone property solution for it.
Done
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Robert Kroeger (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Jianhui J Dai, Kramer Ge, Maksim Sisov.
Patch set 14:Code-Review +1
2 comments:
Patchset:
Great. Thanks.
File media/gpu/vaapi/vaapi_picture_factory.cc:
Patch Set #14, Line 53:
#if BUILDFLAG(USE_VAAPI_X11)I note in passing that (afaik) the vaapi code is only on Linux-like platforms and they're all ozone users so the #ifs are probably unnecessary.
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Jianhui J Dai (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge, Maksim Sisov, Robert Kroeger.
1 comment:
File media/gpu/vaapi/vaapi_picture_factory.cc:
Patch Set #14, Line 53:
#if BUILDFLAG(USE_VAAPI_X11)
I note in passing that (afaik) the vaapi code is only on Linux-like platforms and they're all ozone […]
I think ChromeOS don't need the X11 code, for less binary size.
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Robert Kroeger (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Jianhui J Dai, Kramer Ge, Maksim Sisov.
1 comment:
File media/gpu/vaapi/vaapi_picture_factory.cc:
Patch Set #14, Line 53:
#if BUILDFLAG(USE_VAAPI_X11)
I think ChromeOS don't need the X11 code, for less binary size.
My understanding is that CrOS no longer defines LINUX. But you should feel free to leave this as is.
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Jianhui J Dai (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge, Maksim Sisov.
1 comment:
Patchset:
@andres friendly ping if review comment from VA-API perspective.
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Andres Calderon Jaramillo (Gerrit)
Attention is currently required from: Jianhui J Dai, Kramer Ge, Maksim Sisov.
3 comments:
Patchset:
I'm mostly fine with this CL :)
File media/gpu/vaapi/vaapi_wrapper.cc:
Patch Set #14, Line 746:
#if BUILDFLAG(USE_VAAPI_X11)nit: with this CL, `USE_VAAPI_X11` may no longer be a good name because setting that to true doesn't mean we'll use VA-API/X11 (or even try to). Please change [this](https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/args.gni;l=9-12;drc=cf0720f7ee1734aa91405756c9782b64187a2c02) to:
```
# Build Chrome support for using VA-API over X11. Note that setting this to true is
# not a guarantee that Chrome will use (or even try to use) VA-API over X11. In
# particular, it is possible to build Chrome with support for VA-API over X11 but
# pick Wayland as the Ozone backend at runtime. In this case, Chrome will try to
# use VA-API over DRM.
support_vaapi_over_x11 = is_linux && ozone_platform_x11 &&
(target_cpu == "x86" || target_cpu == "x64") && !is_castos
```Patch Set #14, Line 813:
case gl::kGLImplementationEGLANGLE:Question: did you test with ANGLE on Ozone/Wayland?
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Maksim Sisov (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Jianhui J Dai, Kramer Ge.
Patch set 14:Code-Review +1
1 comment:
File media/gpu/vaapi/vaapi_wrapper.cc:
Patch Set #14, Line 813:
case gl::kGLImplementationEGLANGLE:
Question: did you test with ANGLE on Ozone/Wayland?
It's not supported yet. Landed, reverted, landed, reverted. There are some issues we are addressing now.
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Jianhui J Dai (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge, Maksim Sisov.
1 comment:
File media/gpu/vaapi/vaapi_wrapper.cc:
Patch Set #14, Line 813:
case gl::kGLImplementationEGLANGLE:
It's not supported yet. Landed, reverted, landed, reverted. […]
Yes. ANGLE is not supported by Wayland yet.
I could test after it is enabled. VA-API uses drm for Wayland/ANGLE in current code.
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Jianhui J Dai (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge, Maksim Sisov, Robert Kroeger.
1 comment:
File media/gpu/vaapi/vaapi_wrapper.cc:
Patch Set #14, Line 746:
#if BUILDFLAG(USE_VAAPI_X11)nit: with this CL, `USE_VAAPI_X11` may no longer be a good name because setting that to true doesn't […]
Done
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Robert Kroeger (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Jianhui J Dai, Kramer Ge.
1 comment:
Patchset:
The CQ failure looks relevant.
Also, note the +1's are no longer sticky.
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Jianhui J Dai (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge, Robert Kroeger.
1 comment:
Patchset:
The CQ failure looks relevant. […]
Pass the CQ dry run. And friendly ping reviewers.
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Robert Kroeger (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Jianhui J Dai, Kramer Ge.
Patch set 17:Code-Review +1
1 comment:
Patchset:
ui/ozone lgtm
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Jianhui J Dai (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge, Maksim Sisov.
4 comments:
File media/gpu/vaapi/vaapi_picture_factory.cc:
Patch Set #14, Line 53:
#if BUILDFLAG(USE_VAAPI_X11)
My understanding is that CrOS no longer defines LINUX. But you should feel free to leave this as is.
Done
File media/gpu/vaapi/vaapi_wrapper.cc:
Patch Set #14, Line 746:
#if BUILDFLAG(USE_VAAPI_X11)
Done
@andres, fixed the naming.
Still need your review on VA-API change to move forward. : )
Patch Set #14, Line 813:
case gl::kGLImplementationEGLANGLE:
Yes. ANGLE is not supported by Wayland yet. […]
Done
File ui/ozone/BUILD.gn:
Patch Set #13, Line 196:
"platform_selection.h",You're opening the door to making a leakier abstraction and I would prefer that we not do that.
Done
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Jianhui J Dai (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge.
1 comment:
File media/gpu/vaapi/vaapi_wrapper.cc:
Patch Set #14, Line 746:
#if BUILDFLAG(USE_VAAPI_X11)
@andres, fixed the naming. […]
@andres, a friendly ping :)
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Maksim Sisov (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Jianhui J Dai, Kramer Ge.
1 comment:
File media/gpu/vaapi/vaapi_wrapper.cc:
Patch Set #14, Line 746:
#if BUILDFLAG(USE_VAAPI_X11)
@andres, a friendly ping :)
@andres, do you think we can already land this?
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Andres Calderon Jaramillo (Gerrit)
Attention is currently required from: Jianhui J Dai, Kramer Ge.
Patch set 17:Code-Review +1
15 comments:
Patchset:
Sorry for the delay -- LGTM mostly, but the main thing is applying the changes in CL:3894976 (see my comments in vaapi_wrapper.cc first). After you make those changes, I can also test it on my setup and send it to the CQ.
File media/gpu/vaapi/BUILD.gn:
if (support_vaapi_over_x11) {
deps += [ "//ui/ozone" ]
}nit: You can remove this assuming you make the changes in my other comments.
File media/gpu/vaapi/vaapi_picture_factory.cc:
Patch Set #17, Line 18:
#include "ui/ozone/public/ozone_platform.h"nit: You can remove this assuming you make the changes in my other comments.
ui::OzonePlatform::IsInitialized() && ui::OzonePlatform::GetInstance()
->GetPlatformProperties()
.supports_vaapi_x11Assuming that you apply the changes in CL:3894976, change this to:
```
absl::optional<bool> may_use_vaapi_over_x11 = VaapiWrapper::MayUseVaapiOverX11();
CHECK(may_use_vaapi_over_x11.has_value());
if (may_use_vaapi_over_x11.value()) {
...
}
```gl::kGLImplementationEGLANGLE,
vaapi_implement_for_egl_angleHave you had a chance to test this on Wayland?
if (ui::OzonePlatform::IsInitialized() && ui::OzonePlatform::GetInstance()
->GetPlatformProperties()
.supports_vaapi_x11) {
return GL_TEXTURE_2D;
}Here too:
```
absl::optional<bool> may_use_vaapi_over_x11 = VaapiWrapper::MayUseVaapiOverX11();
CHECK(may_use_vaapi_over_x11.has_value());
if (may_use_vaapi_over_x11.value())
return GL_TEXTURE_2D;
```#if BUILDFLAG(SUPPORT_VAAPI_OVER_X11)
if (ui::OzonePlatform::IsInitialized() && ui::OzonePlatform::GetInstance()
->GetPlatformProperties()
.supports_vaapi_x11) {
return gfx::BufferFormat::RGBX_8888;
}
#endif // BUILDFLAG(SUPPORT_VAAPI_OVER_X11)Here too:
```
absl::optional<bool> may_use_vaapi_over_x11 = VaapiWrapper::MayUseVaapiOverX11();
CHECK(may_use_vaapi_over_x11.has_value());
if (may_use_vaapi_over_x11.value())
return gfx::BufferFormat::RGBX_8888;
```DCHECK(ui::OzonePlatform::GetInstance()
->GetPlatformProperties()
.supports_vaapi_x11);`DCHECK(VaapiWrapper::MayUseVaapiOverX11().value_or(false));`
DCHECK(ui::OzonePlatform::GetInstance()
->GetPlatformProperties()
.supports_vaapi_x11);`DCHECK(VaapiWrapper::MayUseVaapiOverX11().value_or(false));`
File media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc:
#if BUILDFLAG(SUPPORT_VAAPI_OVER_X11)
// GN doesn't understand conditional includes, so we need nogncheck here.
// See crbug.com/1125897.
#include "ui/ozone/public/ozone_platform.h" // nogncheck
#endif // BUILDFLAG(SUPPORT_VAAPI_OVER_X11)nit: You can remove this assuming you make the changes in my comments.
ui::OzonePlatform::IsInitialized() && ui::OzonePlatform::GetInstance()
->GetPlatformProperties()
.supports_vaapi_x11Assuming that you apply the changes in CL:3894976, change this to:
```
absl::optional<bool> may_use_vaapi_over_x11 = VaapiWrapper::MayUseVaapiOverX11();
CHECK(may_use_vaapi_over_x11.has_value());
if (may_use_vaapi_over_x11.value()) {
...
}
```
File media/gpu/vaapi/vaapi_wrapper.cc:
//
// Use DRM display backend if Ozone platform is not initialized. It only
// happens in VA-API unit tests on Linux.nit: Delete this since you've landed CL:3724915.
ui::OzonePlatform::IsInitialized() && ui::OzonePlatform::GetInstance()
->GetPlatformProperties()
.supports_vaapi_x11I'm not too comfortable with this being here and in a bunch of other places -- I'd much rather you compute this very early once (among other reasons, I want to get rid of this Ozone check in the medium term and always use the VA-API DRM backend -- this will make it easier for me to find the places where we rely on this check). There are a few small changes to do this, but instead of putting them in this comment, I've uploaded them as CL:3894976. Can you apply those changes here?
Patch Set #17, Line 814:
switch (gl::GetGLImplementation()) {If you have only tested one GL implementation, can we reject the other ones until we know they work?
File ui/ozone/public/ozone_platform.h:
// TODO(crbug.com/1116701, crbug.com/1326754): add VA-API support for other
// Ozone platforms on Linux. VA-API supports DRM display backend for all
// Ozone platforms, and supports X11 display backend only for Linux
// Ozone/X11. As a temporary solution, `supports_vaapi_x11` is added to
// implicitly indicate the runtime platform is Ozone/X11. It means VA-API is
// supported on all Linux Ozone platforms through DRM display
// backend by default; if `supports_vaapi_x11` is set, VA-API is supported
// through X11 display backend.I'm not sure I understand this TODO. After this CL, the statements `add VA-API support for other Ozone platforms on Linux` and `It means VA-API is supported on all Linux Ozone platforms through DRM display backend by default` are somewhat contradictory.
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Jianhui J Dai (Gerrit)
Attention is currently required from: Kramer Ge.
1 comment:
Patchset:
Sorry for the delay -- LGTM mostly, but the main thing is applying the changes in CL:3894976 (see my […]
Thank you Andres. Let me apply that patch and solve comments.
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Maksim Sisov (Gerrit)
Attention is currently required from: Jianhui J Dai, Kramer Ge.
1 comment:
Patchset:
it seems like it doesn't work for me -
[30926:15:0915/130158.713529:ERROR:decoder_selector.cc(334)] GetAndInitializeNextDecoder: initializing Unknown Video Decoder
[30926:15:0915/130158.713739:ERROR:decoder_selector.cc(347)] OnDecoderInitializeDone: VaapiVideoDecoder success=202
[30926:15:0915/130158.713812:ERROR:decoder_selector.cc(354)] Failed to initialize VaapiVideoDecoderDebugging the code, it seems like it stops at [1]
Here is my vainfo
libva info: VA-API version 1.14.0
libva info: Trying to open /usr/lib/x86_64-linux-gnu/dri/radeonsi_drv_video.so
libva info: Found init function __vaDriverInit_1_14
libva info: va_openDriver() returns 0
vainfo: VA-API version: 1.14 (libva 2.12.0)
vainfo: Driver version: Mesa Gallium driver 22.0.5 for AMD Radeon RX 460 Graphics (polaris11, LLVM 13.0.1, DRM 3.42, 5.15.0-47-generic)
vainfo: Supported profile and entrypoints
VAProfileMPEG2Simple : VAEntrypointVLD
VAProfileMPEG2Main : VAEntrypointVLD
VAProfileVC1Simple : VAEntrypointVLD
VAProfileVC1Main : VAEntrypointVLD
VAProfileVC1Advanced : VAEntrypointVLD
VAProfileH264ConstrainedBaseline: VAEntrypointVLD
VAProfileH264ConstrainedBaseline: VAEntrypointEncSlice
VAProfileH264Main : VAEntrypointVLD
VAProfileH264Main : VAEntrypointEncSlice
VAProfileH264High : VAEntrypointVLD
VAProfileH264High : VAEntrypointEncSlice
VAProfileHEVCMain : VAEntrypointVLD
VAProfileHEVCMain : VAEntrypointEncSlice
VAProfileHEVCMain10 : VAEntrypointVLD
VAProfileJPEGBaseline : VAEntrypointVLD
VAProfileNone : VAEntrypointVideoProc
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Jianhui J Dai (Gerrit)
Attention is currently required from: Kramer Ge, Maksim Sisov.
1 comment:
Patchset:
it seems like it doesn't work for me - […]
Just realized current patchset17 is built with chromium minigbm, with VDA only.
Build args:
```
use_system_minigbm = false
use_intel_minigbm = true
use_amdgpu_minigbm = true
```Running flags:
```
--use-gl=egl \
--ignore-gpu-blocklist \
--disable-gpu-driver-bug-workaround \
--enable-features=VaapiVideoDecoder \
--disable-features=UseChromeOSDirectVideoDecoder
--enable-hardware-overlays="" \
--ozone-platform=wayland
```In order to run on mesa minigbm, we have to use RGBX_8888, will fix in next patchset.
```
uint32_t VaapiPictureFactory::GetGLTextureTarget() {
#if BUILDFLAG(SUPPORT_VAAPI_OVER_X11)
return GL_TEXTURE_2D;
#else
return GL_TEXTURE_EXTERNAL_OES;
#endif // BUILDFLAG(SUPPORT_VAAPI_OVER_X11)
}gfx::BufferFormat VaapiPictureFactory::GetBufferFormat() {
#if BUILDFLAG(SUPPORT_VAAPI_OVER_X11)
return gfx::BufferFormat::RGBX_8888;
#else
return gfx::BufferFormat::YUV_420_BIPLANAR;
#endif // BUILDFLAG(SUPPORT_VAAPI_OVER_X11)
}
```
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Jianhui J Dai (Gerrit)
Patchset:
Just realized current patchset17 is built with chromium minigbm, with VDA only. […]
To enable VaapiVideoDecoder, there are other future works:
- NV12 buffer issue in system minigbm
- Fix DCHECK in ozone/wayland overlay.
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Jianhui J Dai (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge, Maksim Sisov.
15 comments:
Patchset:
To enable VaapiVideoDecoder, there are other future works: […]
Update patchset-19 to make VDA work on system minigbm.
File media/gpu/vaapi/BUILD.gn:
if (support_vaapi_over_x11) {
deps += [ "//ui/ozone" ]
}nit: You can remove this assuming you make the changes in my other comments.
Done
File media/gpu/vaapi/vaapi_picture_factory.cc:
Patch Set #17, Line 18:
#include "ui/ozone/public/ozone_platform.h"nit: You can remove this assuming you make the changes in my other comments.
Done
ui::OzonePlatform::IsInitialized() && ui::OzonePlatform::GetInstance()
->GetPlatformProperties()
.supports_vaapi_x11
Assuming that you apply the changes in CL:3894976, change this to: […]
Empty `may_use_vaapi_over_x11_` is rejected by`VADisplayState::Initialize()`.
```
bool VADisplayState::Initialize() {
base::AutoLock auto_lock(va_lock_);#if BUILDFLAG(SUPPORT_VAAPI_OVER_X11)
if (!may_use_vaapi_over_x11_.has_value())
return false;
#endif
```We might skip CHECK in later code, use `value_or` instead.
```
if (VaapiWrapper::MayUseVaapiOverX11().value_or(false)) {
...
}
```
gl::kGLImplementationEGLANGLE,
vaapi_implement_for_egl_angleHave you had a chance to test this on Wayland?
These piece of code is never reachable for Ozone/Wayland.
If using `gl::kGLImplementationEGLANGLE`, it will failed in Ozone/Wayland `CreateViewGLSurface`[1], before actually create mojo video decoder.
And we will also reject all GL implementations except native EGL/GLES in `GetVADisplayState` [2].
[1] wayland_surface_factory.cc - Chromium Code Search
https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/wayland/gpu/wayland_surface_factory.cc;l=104;drc=f616c54d73c8eea9db5f7e567611711897651b66[2] vaapi_wrapper.cc - Chromium Code Search
https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/vaapi/vaapi_wrapper.cc;l=803?q=GetSupportedScalabilityModes&ss=chromium%2Fchromium%2Fsrc
if (ui::OzonePlatform::IsInitialized() && ui::OzonePlatform::GetInstance()
->GetPlatformProperties()
.supports_vaapi_x11) {
return GL_TEXTURE_2D;
}
Here too: […]
Done
#if BUILDFLAG(SUPPORT_VAAPI_OVER_X11)
if (ui::OzonePlatform::IsInitialized() && ui::OzonePlatform::GetInstance()
->GetPlatformProperties()
.supports_vaapi_x11) {
return gfx::BufferFormat::RGBX_8888;
}
#endif // BUILDFLAG(SUPPORT_VAAPI_OVER_X11)
Here too: […]
Done
DCHECK(ui::OzonePlatform::GetInstance()
->GetPlatformProperties()
.supports_vaapi_x11);
`DCHECK(VaapiWrapper::MayUseVaapiOverX11(). […]
Done
DCHECK(ui::OzonePlatform::GetInstance()
->GetPlatformProperties()
.supports_vaapi_x11);
`DCHECK(VaapiWrapper::MayUseVaapiOverX11(). […]
Done
File media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc:
#if BUILDFLAG(SUPPORT_VAAPI_OVER_X11)
// GN doesn't understand conditional includes, so we need nogncheck here.
// See crbug.com/1125897.
#include "ui/ozone/public/ozone_platform.h" // nogncheck
#endif // BUILDFLAG(SUPPORT_VAAPI_OVER_X11)nit: You can remove this assuming you make the changes in my comments.
Done
ui::OzonePlatform::IsInitialized() && ui::OzonePlatform::GetInstance()
->GetPlatformProperties()
.supports_vaapi_x11
Assuming that you apply the changes in CL:3894976, change this to: […]
Done
File media/gpu/vaapi/vaapi_wrapper.cc:
//
// Use DRM display backend if Ozone platform is not initialized. It only
// happens in VA-API unit tests on Linux.nit: Delete this since you've landed CL:3724915.
Done
ui::OzonePlatform::IsInitialized() && ui::OzonePlatform::GetInstance()
->GetPlatformProperties()
.supports_vaapi_x11
I'm not too comfortable with this being here and in a bunch of other places -- I'd much rather you c […]
Done
Patch Set #17, Line 814:
switch (gl::GetGLImplementation()) {If you have only tested one GL implementation, can we reject the other ones until we know they work?
Ozone/Wayland only supports mojo video decoder on native EGL/GLES.
If using `gl::kGLImplementationEGLANGLE`, it will failed in Ozone/Wayland `CreateViewGLSurface`[1], before actually create mojo video decoder.
[1] wayland_surface_factory.cc - Chromium Code Search
https://source.chromium.org/chromium/chromium/src/+/main:ui/ozone/platform/wayland/gpu/wayland_surface_factory.cc;l=104;drc=f616c54d73c8eea9db5f7e567611711897651b66
File ui/ozone/public/ozone_platform.h:
// TODO(crbug.com/1116701, crbug.com/1326754): add VA-API support for other
// Ozone platforms on Linux. VA-API supports DRM display backend for all
// Ozone platforms, and supports X11 display backend only for Linux
// Ozone/X11. As a temporary solution, `supports_vaapi_x11` is added to
// implicitly indicate the runtime platform is Ozone/X11. It means VA-API is
// supported on all Linux Ozone platforms through DRM display
// backend by default; if `supports_vaapi_x11` is set, VA-API is supported
// through X11 display backend.
I'm not sure I understand this TODO. […]
Ack
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Maksim Sisov (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Jianhui J Dai, Kramer Ge.
2 comments:
File media/gpu/vaapi/vaapi_picture_factory.cc:
Patch Set #19, Line 106:
return GL_TEXTURE_2D;This should also be used on Linux/Wayland. Probably, guarding it under OS_LINUX should be enough.
Patch Set #19, Line 114:
return gfx::BufferFormat::RGBX_8888;Same here. Otherwise, Ozone/Wayland will fail to create a buffer. YUV formats are typically not available on Linux. So, it should be RGBX for Wayland/Linux
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Andres Calderon Jaramillo (Gerrit)
Attention is currently required from: Jianhui J Dai, Kramer Ge.
Patch set 19:Code-Review +1
5 comments:
File media/gpu/vaapi/vaapi_picture_factory.cc:
ui::OzonePlatform::IsInitialized() && ui::OzonePlatform::GetInstance()
->GetPlatformProperties()
.supports_vaapi_x11
Empty `may_use_vaapi_over_x11_` is rejected by`VADisplayState::Initialize()`. […]
I'm not sure if I understood your reply: it's correct that `VADisplayState::Initialize()` should reject an empty `may_use_vaapi_over_x11_` so that we shouldn't even be trying to construct a `VaapiPictureFactory` in that case. However, using `value_or()` suggests to a casual reader that reaching this point with an empty `may_use_vaapi_over_x11_` is a legitimate possibility, when in fact, it's not. That's why I'd prefer to use a CHECK (here, and in the other suggested places): it serves the double purpose of documenting that this is an invariant that cannot be violated and enforcing it.
gl::kGLImplementationEGLANGLE,
vaapi_implement_for_egl_angle
These piece of code is never reachable for Ozone/Wayland. […]
Ok, that makes sense, at least for now -- Maksim can chime in, but I'd guess that at some point Ozone/Wayland + ANGLE will need to work.
For now, then, let's be explicit about that restriction (see my comment in vaapi_wrapper.cc), and here:
1) If `VaapiWrapper::MayUseVaapiOverX11()` returns true, we can add the (`kGLImplementationEGLANGLE`, `kVaapiImplementationAngle`) combination.
2) If `VaapiWrapper::MayUseVaapiOverX11()` returns false **and** we're not on Linux, then add the (`kGLImplementationEGLANGLE`, `kVaapiImplementationDrm`) combination. Otherwise (we're on Linux), don't add anything for `kGLImplementationEGLANGLE`.
3) In `VaapiPictureFactory::DeterminePictureCreationAndDownloadingMechanism()`, guard the `kVaapiImplementationNone` case with `#if defined(USE_OZONE) && !BUILDFLAG(IS_LINUX)`.
File media/gpu/vaapi/vaapi_picture_factory.cc:
#if BUILDFLAG(SUPPORT_VAAPI_OVER_X11)
return GL_TEXTURE_2D;
#else
return GL_TEXTURE_EXTERNAL_OES;
#endif // BUILDFLAG(SUPPORT_VAAPI_OVER_X11)
nit: Rewrite as
```
#if BUILDFLAG(IS_CHROMEOS)
return GL_TEXTURE_EXTERNAL_OES;
#else
return GL_TEXTURE_2D;
#endif
```Similar for `GetBufferFormat()`.
The rationale is that there's more of a clear relationship between ChromeOS and `YUV_420_BIPLANAR`.
File media/gpu/vaapi/vaapi_wrapper.cc:
Patch Set #19, Line 825:
!BUILDFLAG(SUPPORT_VAAPI_OVER_X11)In this particular instance, it'd make more semantic sense to do `#if BUILDFLAG(IS_CHROMEOS)` (i.e., these two paths only make sense on ChromeOS for now) and the comment on the next line can say:
```
// GetVADisplayState() should not get called on Linux with Ozone/X11
// (GetVADisplayStateX11() should get called instead), and we haven't tried VA-API
// decoding on Linux with Ozone/Wayland and anything other than native EGL/GLES2.
```
File ui/ozone/public/ozone_platform.h:
Patch Set #19, Line 153:
As a temporary solutionFor my understanding: `As a temporary solution` means you have something in mind for a more "permanent solution." What is that? Supporting only the VA-API DRM backend?
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Jianhui J Dai (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge, Maksim Sisov.
8 comments:
Patchset:
Thanks for the patient review. Fixed most of comments.
For patchset 20, it failed on some Linux tests using `MockVaapiWrapper` without initialize Ozone, because `VaapiWrapper::MayUseVaapiOverX11()` return empty value [1].
It is not easy to MOCK the static method `VaapiWrapper::MayUseVaapiOverX11()`.
It takes more times to figure out solution on such test failures.
[1] vaapi_video_decode_accelerator_unittest.cc - Chromium Code Search
https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc;l=73
File media/gpu/vaapi/vaapi_picture_factory.cc:
ui::OzonePlatform::IsInitialized() && ui::OzonePlatform::GetInstance()
->GetPlatformProperties()
.supports_vaapi_x11
I'm not sure if I understood your reply: it's correct that `VADisplayState::Initialize()` should rej […]
Done
gl::kGLImplementationEGLANGLE,
vaapi_implement_for_egl_angle
Ok, that makes sense, at least for now -- Maksim can chime in, but I'd guess that at some point Ozon […]
1) and 2) are done.
For 3), `kVaapiImplementationNone` is valid for Ozone/X11 [1], so add DCHECK for it.
[1] vaapi_wrapper.cc - Chromium Code Search
https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/vaapi/vaapi_wrapper.cc;l=780?q=vaapi_wrapper.c&ss=chromium%2Fchromium%2Fsrc
File media/gpu/vaapi/vaapi_picture_factory.cc:
Patch Set #19, Line 106:
return GL_TEXTURE_2D;
This should also be used on Linux/Wayland. Probably, guarding it under OS_LINUX should be enough.
Done
#if BUILDFLAG(SUPPORT_VAAPI_OVER_X11)
return GL_TEXTURE_2D;
#else
return GL_TEXTURE_EXTERNAL_OES;
#endif // BUILDFLAG(SUPPORT_VAAPI_OVER_X11)
nit: Rewrite as […]
Done
Patch Set #19, Line 114:
return gfx::BufferFormat::RGBX_8888;
Same here. Otherwise, Ozone/Wayland will fail to create a buffer. […]
Done
File media/gpu/vaapi/vaapi_wrapper.cc:
Patch Set #19, Line 825:
!BUILDFLAG(SUPPORT_VAAPI_OVER_X11)
In this particular instance, it'd make more semantic sense to do `#if BUILDFLAG(IS_CHROMEOS)` (i.e. […]
Done
File ui/ozone/public/ozone_platform.h:
Patch Set #19, Line 153:
As a temporary solution
For my understanding: `As a temporary solution` means you have something in mind for a more "permane […]
Ack
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Jianhui J Dai (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge, Robert Kroeger.
1 comment:
Patchset:
Fixed CQ DRY RUN:
1. Pass `may_use_vaapi_over_x11` to `VaapiPictureFactory` as construction parameter, in order to avoid empty value in `MockVaapiPictureFactory` (MockVaapiWrapper).
2. In `VADisplayState::PreSandboxInitialization`, set `may_use_vaapi_over_x11_` before drm check to pass X11 EGL ANGLE test case (no valid drm file).
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Robert Kroeger (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Jianhui J Dai, Kramer Ge.
1 comment:
File ui/ozone/public/ozone_platform.h:
Patch Set #23, Line 152:
// VA-API supports DRM display backend for all Ozone platforms, and supportsThis is unclear to me. You are saying that vaapi only works with ozone/{drm is available} and ozone/x11?
In particular: it does not work with ozone/wayland without DRM?
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Jianhui J Dai (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Jianhui J Dai, Kramer Ge.
Jianhui J Dai uploaded patch set #25 to this change.
[Ozone/Linux] Support VA-API on Linux Ozone/Wayland
VA-API supports different display backends [1]. VA/DRM and VA/X11 are used by Chromium at the moment. All Ozone platforms support VA/DRM by default. VA/X11 is supported only on Ozone/X11.
This CL renames 'supports_vaapi' to 'supports_vaapi_x11'; it indicates if VA/X11 supported. `VaapiPictureFactory` takes `may_use_vaapi_over_x11` flag to determine picture creation and downloading mechanism for Linux Ozone/X11 and Ozone/Wayland. Linux Ozone/Wayland supports VA-API through VA/DRM.
[1] https://github.com/intel/libva/blob/master/va/va_backend.h
Test: VDA video playback on Linux X11 and Linux Wayland
Bug: 1326754,1116701
Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
---
M media/gpu/BUILD.gn
M media/gpu/args.gni
M media/gpu/vaapi/BUILD.gn
M media/gpu/vaapi/va_stub_header.fragment
M media/gpu/vaapi/vaapi_picture_factory.cc
M media/gpu/vaapi/vaapi_picture_factory.h
M media/gpu/vaapi/vaapi_video_decode_accelerator.cc
M media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc
M media/gpu/vaapi/vaapi_wrapper.cc
M media/gpu/vaapi/vaapi_wrapper.h
M ui/ozone/platform/x11/ozone_platform_x11.cc
M ui/ozone/public/ozone_platform.h
12 files changed, 203 insertions(+), 84 deletions(-)
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Jianhui J Dai (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge, Robert Kroeger.
1 comment:
File ui/ozone/public/ozone_platform.h:
Patch Set #23, Line 152:
// VA-API supports DRM display backend for all Ozone platforms, and supports
This is unclear to me. […]
Update the description. Ozone/Wayland supports VA-API through VA/DRM. It does not work if no VA/DRM.
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Robert Kroeger (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Jianhui J Dai, Kramer Ge.
Patch set 26:Code-Review +1
Jianhui J Dai (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge.
1 comment:
Patchset:
@andres, do you think we can ship this CL?
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Kramer Ge (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Jianhui J Dai.
Patch set 26:Code-Review +1
1 comment:
Patchset:
My +1 dropped
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Kramer Ge (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Jianhui J Dai.
1 comment:
Patchset:
My +1 dropped
Not that I own anything
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Jianhui J Dai (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge, Robert Kroeger.
1 comment:
Patchset:
Rebase to latest to fix merge conflict.
And a friendly ping reviewers.
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Robert Kroeger (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Jianhui J Dai, Kramer Ge.
Patch set 27:Code-Review +1
1 comment:
Patchset:
ozone still lgtm
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Jianhui J Dai (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge.
1 comment:
Patchset:
@andres, could you take a look after code rebase and fixing test failure? Thanks.
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Nick Yamane (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Jianhui J Dai.
1 comment:
Patchset:
Hi jianhui@, Could you provide a status update/plan for this? Is there something blocking this which we could help with? thanks
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Jianhui J Dai (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Nick Yamane.
1 comment:
Patchset:
Hi jianhui@, Could you provide a status update/plan for this? Is there something blocking this which […]
@Yamane, thanks.
I will check if we can go forward with this CL.
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Wasser Mai (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Jianhui J Dai, Nick Yamane.
1 comment:
Patchset:
@Yamane, thanks. […]
the patch (modified Patchset 27) works very well with chromium-111, but unfortunately no longer with chromium-112.0.5615.49.
@Jianhui, it would be really great if you can continue to work on your patchset for chromium-112.Thanks a lot for your great work!
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.
Richard May (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Jianhui J Dai, Nick Yamane.
Jianhui J Dai has uploaded this change for review.
[Ozone/Linux] Support VA-API on Linux Ozone/Wayland
VA-API supports different display backends [1]. VA/DRM and VA/X11 are
used by Chromium at the moment. All Ozone platforms support VA/DRM by
default. VA/X11 is supported only on Ozone/X11.
This CL renames 'supports_vaapi' to 'supports_vaapi_x11'; it indicates
if VA/X11 supported. Linux Ozone/X11 supports both VA/X11 and VA/DRM;
Linux Ozone/Wayland supports only VA/DRM. `VaapiPictureFactory` also
takes this flag to determine picture creation and downloading mechanism
on Linux.
[1] https://github.com/intel/libva/blob/master/va/va_backend.h
Test: VDA video playback on Linux X11 and Linux Wayland
Bug: 1326754,1116701
Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
---
M media/gpu/BUILD.gn
M media/gpu/args.gni
M media/gpu/vaapi/BUILD.gn
M media/gpu/vaapi/va_stub_header.fragment
M media/gpu/vaapi/vaapi_picture_factory.cc
M media/gpu/vaapi/vaapi_picture_factory.h
M media/gpu/vaapi/vaapi_picture_native_pixmap_ozone.cc
M media/gpu/vaapi/vaapi_video_decode_accelerator.cc
M media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc
M media/gpu/vaapi/vaapi_wrapper.cc
M media/gpu/vaapi/vaapi_wrapper.h
M ui/ozone/platform/x11/ozone_platform_x11.cc
M ui/ozone/public/ozone_platform.h
13 files changed, 187 insertions(+), 85 deletions(-)
Jianhui J Dai (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo, Nick Yamane, Robert Kroeger.
1 comment:
Patchset:
Rebase after VDA was removed.
To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.