[Ozone/Linux] Support VA-API on Linux Ozone/Wayland [chromium/src : main]

1,022 views
Skip to first unread message

Jianhui J Dai (Gerrit)

unread,
Jul 26, 2022, 10:41:15 PM7/26/22
to feature-me...@chromium.org, media-cro...@chromium.org, poscia...@chromium.org, vaapi-...@chromium.org, ozone-...@chromium.org, Robert Kroeger, Chromium LUCI CQ, Ted (Chromium) Meyer, Kramer Ge, Maksim Sisov, Zhenyao Mo, Andres Calderon Jaramillo, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge, Maksim Sisov, Robert Kroeger.

View Change

2 comments:

  • Patchset:

    • Patch Set #14:

      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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
Gerrit-Change-Number: 3646633
Gerrit-PatchSet: 14
Gerrit-Owner: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Kramer Ge <fang...@chromium.org>
Gerrit-Reviewer: Maksim Sisov <msi...@igalia.com>
Gerrit-Reviewer: Robert Kroeger <rjkr...@chromium.org>
Gerrit-CC: Ted (Chromium) Meyer <tmath...@chromium.org>
Gerrit-CC: Zhenyao Mo <z...@chromium.org>
Gerrit-Attention: Maksim Sisov <msi...@igalia.com>
Gerrit-Attention: Kramer Ge <fang...@chromium.org>
Gerrit-Attention: Robert Kroeger <rjkr...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Comment-Date: Wed, 27 Jul 2022 02:41:06 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Robert Kroeger <rjkr...@chromium.org>
Comment-In-Reply-To: Jianhui J Dai <jianhu...@intel.com>
Gerrit-MessageType: comment

Robert Kroeger (Gerrit)

unread,
Jul 27, 2022, 2:25:36 PM7/27/22
to Jianhui J Dai, feature-me...@chromium.org, media-cro...@chromium.org, poscia...@chromium.org, vaapi-...@chromium.org, ozone-...@chromium.org, Chromium LUCI CQ, Ted (Chromium) Meyer, Kramer Ge, Maksim Sisov, Zhenyao Mo, Andres Calderon Jaramillo, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo, Jianhui J Dai, Kramer Ge, Maksim Sisov.

Patch set 14:Code-Review +1

View Change

2 comments:

  • Patchset:

  • 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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
Gerrit-Change-Number: 3646633
Gerrit-PatchSet: 14
Gerrit-Owner: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Kramer Ge <fang...@chromium.org>
Gerrit-Reviewer: Maksim Sisov <msi...@igalia.com>
Gerrit-Reviewer: Robert Kroeger <rjkr...@chromium.org>
Gerrit-CC: Ted (Chromium) Meyer <tmath...@chromium.org>
Gerrit-CC: Zhenyao Mo <z...@chromium.org>
Gerrit-Attention: Maksim Sisov <msi...@igalia.com>
Gerrit-Attention: Kramer Ge <fang...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Attention: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Comment-Date: Wed, 27 Jul 2022 18:25:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Jianhui J Dai (Gerrit)

unread,
Jul 28, 2022, 9:31:48 PM7/28/22
to feature-me...@chromium.org, media-cro...@chromium.org, poscia...@chromium.org, vaapi-...@chromium.org, ozone-...@chromium.org, Robert Kroeger, Chromium LUCI CQ, Ted (Chromium) Meyer, Kramer Ge, Maksim Sisov, Zhenyao Mo, Andres Calderon Jaramillo, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge, Maksim Sisov, Robert Kroeger.

View Change

1 comment:

  • File media/gpu/vaapi/vaapi_picture_factory.cc:

    • 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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
Gerrit-Change-Number: 3646633
Gerrit-PatchSet: 14
Gerrit-Owner: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Kramer Ge <fang...@chromium.org>
Gerrit-Reviewer: Maksim Sisov <msi...@igalia.com>
Gerrit-Reviewer: Robert Kroeger <rjkr...@chromium.org>
Gerrit-CC: Ted (Chromium) Meyer <tmath...@chromium.org>
Gerrit-CC: Zhenyao Mo <z...@chromium.org>
Gerrit-Attention: Maksim Sisov <msi...@igalia.com>
Gerrit-Attention: Kramer Ge <fang...@chromium.org>
Gerrit-Attention: Robert Kroeger <rjkr...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Comment-Date: Fri, 29 Jul 2022 01:31:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Robert Kroeger <rjkr...@chromium.org>
Gerrit-MessageType: comment

Robert Kroeger (Gerrit)

unread,
Jul 29, 2022, 2:23:06 PM7/29/22
to Jianhui J Dai, feature-me...@chromium.org, media-cro...@chromium.org, poscia...@chromium.org, vaapi-...@chromium.org, ozone-...@chromium.org, Chromium LUCI CQ, Ted (Chromium) Meyer, Kramer Ge, Maksim Sisov, Zhenyao Mo, Andres Calderon Jaramillo, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo, Jianhui J Dai, Kramer Ge, Maksim Sisov.

View Change

1 comment:

  • File media/gpu/vaapi/vaapi_picture_factory.cc:

    • 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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
Gerrit-Change-Number: 3646633
Gerrit-PatchSet: 14
Gerrit-Owner: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Kramer Ge <fang...@chromium.org>
Gerrit-Reviewer: Maksim Sisov <msi...@igalia.com>
Gerrit-Reviewer: Robert Kroeger <rjkr...@chromium.org>
Gerrit-CC: Ted (Chromium) Meyer <tmath...@chromium.org>
Gerrit-CC: Zhenyao Mo <z...@chromium.org>
Gerrit-Attention: Maksim Sisov <msi...@igalia.com>
Gerrit-Attention: Kramer Ge <fang...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Attention: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Comment-Date: Fri, 29 Jul 2022 18:22:56 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Robert Kroeger <rjkr...@chromium.org>

Jianhui J Dai (Gerrit)

unread,
Aug 7, 2022, 8:33:32 PM8/7/22
to feature-me...@chromium.org, media-cro...@chromium.org, poscia...@chromium.org, vaapi-...@chromium.org, ozone-...@chromium.org, Robert Kroeger, Chromium LUCI CQ, Ted (Chromium) Meyer, Kramer Ge, Maksim Sisov, Zhenyao Mo, Andres Calderon Jaramillo, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge, Maksim Sisov.

View Change

1 comment:

  • Patchset:

    • Patch Set #14:

      @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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
Gerrit-Change-Number: 3646633
Gerrit-PatchSet: 14
Gerrit-Owner: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Kramer Ge <fang...@chromium.org>
Gerrit-Reviewer: Maksim Sisov <msi...@igalia.com>
Gerrit-Reviewer: Robert Kroeger <rjkr...@chromium.org>
Gerrit-CC: Ted (Chromium) Meyer <tmath...@chromium.org>
Gerrit-CC: Zhenyao Mo <z...@chromium.org>
Gerrit-Attention: Maksim Sisov <msi...@igalia.com>
Gerrit-Attention: Kramer Ge <fang...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Comment-Date: Mon, 08 Aug 2022 00:33:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Andres Calderon Jaramillo (Gerrit)

unread,
Aug 8, 2022, 8:48:38 PM8/8/22
to Jianhui J Dai, feature-me...@chromium.org, media-cro...@chromium.org, poscia...@chromium.org, vaapi-...@chromium.org, ozone-...@chromium.org, Robert Kroeger, Chromium LUCI CQ, Ted (Chromium) Meyer, Kramer Ge, Maksim Sisov, Zhenyao Mo, chromium...@chromium.org

Attention is currently required from: Jianhui J Dai, Kramer Ge, Maksim Sisov.

View Change

3 comments:

  • Patchset:

  • 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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
Gerrit-Change-Number: 3646633
Gerrit-PatchSet: 14
Gerrit-Owner: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Kramer Ge <fang...@chromium.org>
Gerrit-Reviewer: Maksim Sisov <msi...@igalia.com>
Gerrit-Reviewer: Robert Kroeger <rjkr...@chromium.org>
Gerrit-CC: Ted (Chromium) Meyer <tmath...@chromium.org>
Gerrit-CC: Zhenyao Mo <z...@chromium.org>
Gerrit-Attention: Maksim Sisov <msi...@igalia.com>
Gerrit-Attention: Kramer Ge <fang...@chromium.org>
Gerrit-Attention: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Comment-Date: Tue, 09 Aug 2022 00:48:29 +0000

Maksim Sisov (Gerrit)

unread,
Aug 9, 2022, 1:47:44 AM8/9/22
to Jianhui J Dai, feature-me...@chromium.org, media-cro...@chromium.org, poscia...@chromium.org, vaapi-...@chromium.org, ozone-...@chromium.org, Robert Kroeger, Chromium LUCI CQ, Ted (Chromium) Meyer, Kramer Ge, Zhenyao Mo, Andres Calderon Jaramillo, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo, Jianhui J Dai, Kramer Ge.

Patch set 14:Code-Review +1

View Change

1 comment:

    • 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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
Gerrit-Change-Number: 3646633
Gerrit-PatchSet: 14
Gerrit-Owner: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Kramer Ge <fang...@chromium.org>
Gerrit-Reviewer: Maksim Sisov <msi...@igalia.com>
Gerrit-Reviewer: Robert Kroeger <rjkr...@chromium.org>
Gerrit-CC: Ted (Chromium) Meyer <tmath...@chromium.org>
Gerrit-CC: Zhenyao Mo <z...@chromium.org>
Gerrit-Attention: Kramer Ge <fang...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Attention: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Comment-Date: Tue, 09 Aug 2022 05:47:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-MessageType: comment

Jianhui J Dai (Gerrit)

unread,
Aug 9, 2022, 3:29:10 AM8/9/22
to feature-me...@chromium.org, media-cro...@chromium.org, poscia...@chromium.org, vaapi-...@chromium.org, ozone-...@chromium.org, Robert Kroeger, Chromium LUCI CQ, Ted (Chromium) Meyer, Kramer Ge, Maksim Sisov, Zhenyao Mo, Andres Calderon Jaramillo, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge, Maksim Sisov.

View Change

1 comment:

  • File media/gpu/vaapi/vaapi_wrapper.cc:

    • 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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
Gerrit-Change-Number: 3646633
Gerrit-PatchSet: 14
Gerrit-Owner: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Kramer Ge <fang...@chromium.org>
Gerrit-Reviewer: Maksim Sisov <msi...@igalia.com>
Gerrit-Reviewer: Robert Kroeger <rjkr...@chromium.org>
Gerrit-CC: Ted (Chromium) Meyer <tmath...@chromium.org>
Gerrit-CC: Zhenyao Mo <z...@chromium.org>
Gerrit-Attention: Maksim Sisov <msi...@igalia.com>
Gerrit-Attention: Kramer Ge <fang...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Comment-Date: Tue, 09 Aug 2022 07:28:58 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Maksim Sisov <msi...@igalia.com>

Jianhui J Dai (Gerrit)

unread,
Aug 9, 2022, 7:01:51 AM8/9/22
to feature-me...@chromium.org, media-cro...@chromium.org, poscia...@chromium.org, vaapi-...@chromium.org, ozone-...@chromium.org, Robert Kroeger, Chromium LUCI CQ, Ted (Chromium) Meyer, Kramer Ge, Maksim Sisov, Zhenyao Mo, Andres Calderon Jaramillo, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge, Maksim Sisov, Robert Kroeger.

View Change

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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
Gerrit-Change-Number: 3646633
Gerrit-PatchSet: 16
Gerrit-Owner: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Kramer Ge <fang...@chromium.org>
Gerrit-Reviewer: Maksim Sisov <msi...@igalia.com>
Gerrit-Reviewer: Robert Kroeger <rjkr...@chromium.org>
Gerrit-CC: Ted (Chromium) Meyer <tmath...@chromium.org>
Gerrit-CC: Zhenyao Mo <z...@chromium.org>
Gerrit-Attention: Maksim Sisov <msi...@igalia.com>
Gerrit-Attention: Kramer Ge <fang...@chromium.org>
Gerrit-Attention: Robert Kroeger <rjkr...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Comment-Date: Tue, 09 Aug 2022 11:01:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No

Robert Kroeger (Gerrit)

unread,
Aug 18, 2022, 10:37:34 AM8/18/22
to Jianhui J Dai, feature-me...@chromium.org, media-cro...@chromium.org, poscia...@chromium.org, vaapi-...@chromium.org, ozone-...@chromium.org, Chromium LUCI CQ, Ted (Chromium) Meyer, Kramer Ge, Maksim Sisov, Zhenyao Mo, Andres Calderon Jaramillo, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo, Jianhui J Dai, Kramer Ge.

View Change

1 comment:

  • Patchset:

    • Patch Set #16:

      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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
Gerrit-Change-Number: 3646633
Gerrit-PatchSet: 16
Gerrit-Owner: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Kramer Ge <fang...@chromium.org>
Gerrit-Reviewer: Maksim Sisov <msi...@igalia.com>
Gerrit-Reviewer: Robert Kroeger <rjkr...@chromium.org>
Gerrit-CC: Ted (Chromium) Meyer <tmath...@chromium.org>
Gerrit-CC: Zhenyao Mo <z...@chromium.org>
Gerrit-Attention: Kramer Ge <fang...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Attention: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Comment-Date: Thu, 18 Aug 2022 14:37:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Jianhui J Dai (Gerrit)

unread,
Aug 18, 2022, 8:58:01 PM8/18/22
to feature-me...@chromium.org, media-cro...@chromium.org, poscia...@chromium.org, vaapi-...@chromium.org, ozone-...@chromium.org, Robert Kroeger, Chromium LUCI CQ, Ted (Chromium) Meyer, Kramer Ge, Maksim Sisov, Zhenyao Mo, Andres Calderon Jaramillo, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge, Robert Kroeger.

View Change

1 comment:

  • Patchset:

    • Patch Set #16:

      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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
Gerrit-Change-Number: 3646633
Gerrit-PatchSet: 17
Gerrit-Owner: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Kramer Ge <fang...@chromium.org>
Gerrit-Reviewer: Maksim Sisov <msi...@igalia.com>
Gerrit-Reviewer: Robert Kroeger <rjkr...@chromium.org>
Gerrit-CC: Ted (Chromium) Meyer <tmath...@chromium.org>
Gerrit-CC: Zhenyao Mo <z...@chromium.org>
Gerrit-Attention: Kramer Ge <fang...@chromium.org>
Gerrit-Attention: Robert Kroeger <rjkr...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Comment-Date: Fri, 19 Aug 2022 00:57:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No

Robert Kroeger (Gerrit)

unread,
Aug 19, 2022, 12:57:55 PM8/19/22
to Jianhui J Dai, feature-me...@chromium.org, media-cro...@chromium.org, poscia...@chromium.org, vaapi-...@chromium.org, ozone-...@chromium.org, Chromium LUCI CQ, Ted (Chromium) Meyer, Kramer Ge, Maksim Sisov, Zhenyao Mo, Andres Calderon Jaramillo, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo, Jianhui J Dai, Kramer Ge.

Patch set 17:Code-Review +1

View Change

1 comment:

To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
Gerrit-Change-Number: 3646633
Gerrit-PatchSet: 17
Gerrit-Owner: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Kramer Ge <fang...@chromium.org>
Gerrit-Reviewer: Maksim Sisov <msi...@igalia.com>
Gerrit-Reviewer: Robert Kroeger <rjkr...@chromium.org>
Gerrit-CC: Ted (Chromium) Meyer <tmath...@chromium.org>
Gerrit-CC: Zhenyao Mo <z...@chromium.org>
Gerrit-Attention: Kramer Ge <fang...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Attention: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Comment-Date: Fri, 19 Aug 2022 16:57:40 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Jianhui J Dai (Gerrit)

unread,
Aug 24, 2022, 2:12:38 AM8/24/22
to feature-me...@chromium.org, media-cro...@chromium.org, poscia...@chromium.org, vaapi-...@chromium.org, ozone-...@chromium.org, Robert Kroeger, Chromium LUCI CQ, Ted (Chromium) Meyer, Kramer Ge, Maksim Sisov, Zhenyao Mo, Andres Calderon Jaramillo, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge, Maksim Sisov.

View Change

4 comments:

  • File media/gpu/vaapi/vaapi_picture_factory.cc:

    • 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:

    • Done

      @andres, fixed the naming.
      Still need your review on VA-API change to move forward. : )

    • 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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
Gerrit-Change-Number: 3646633
Gerrit-PatchSet: 17
Gerrit-Owner: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Kramer Ge <fang...@chromium.org>
Gerrit-Reviewer: Maksim Sisov <msi...@igalia.com>
Gerrit-Reviewer: Robert Kroeger <rjkr...@chromium.org>
Gerrit-CC: Ted (Chromium) Meyer <tmath...@chromium.org>
Gerrit-CC: Zhenyao Mo <z...@chromium.org>
Gerrit-Attention: Maksim Sisov <msi...@igalia.com>
Gerrit-Attention: Kramer Ge <fang...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Comment-Date: Wed, 24 Aug 2022 06:12:25 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Maksim Sisov <msi...@igalia.com>
Comment-In-Reply-To: Robert Kroeger <rjkr...@chromium.org>
Comment-In-Reply-To: Andres Calderon Jaramillo <andr...@chromium.org>

Jianhui J Dai (Gerrit)

unread,
Sep 8, 2022, 1:57:55 AM9/8/22
to feature-me...@chromium.org, media-cro...@chromium.org, poscia...@chromium.org, vaapi-...@chromium.org, ozone-...@chromium.org, Maksim Sisov, Robert Kroeger, Chromium LUCI CQ, Ted (Chromium) Meyer, Kramer Ge, Zhenyao Mo, Andres Calderon Jaramillo, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge.

View Change

1 comment:

  • File media/gpu/vaapi/vaapi_wrapper.cc:

    • @andres, fixed the naming. […]

      @andres, a friendly ping :)

To view, visit change 3646633. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
Gerrit-Change-Number: 3646633
Gerrit-PatchSet: 17
Gerrit-Owner: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Kramer Ge <fang...@chromium.org>
Gerrit-Reviewer: Robert Kroeger <rjkr...@chromium.org>
Gerrit-CC: Maksim Sisov <msi...@igalia.com>
Gerrit-CC: Ted (Chromium) Meyer <tmath...@chromium.org>
Gerrit-CC: Zhenyao Mo <z...@chromium.org>
Gerrit-Attention: Kramer Ge <fang...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Comment-Date: Thu, 08 Sep 2022 05:57:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No

Maksim Sisov (Gerrit)

unread,
Sep 13, 2022, 7:19:17 AM9/13/22
to Jianhui J Dai, feature-me...@chromium.org, media-cro...@chromium.org, poscia...@chromium.org, vaapi-...@chromium.org, ozone-...@chromium.org, Robert Kroeger, Chromium LUCI CQ, Ted (Chromium) Meyer, Kramer Ge, Zhenyao Mo, Andres Calderon Jaramillo, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo, Jianhui J Dai, Kramer Ge.

View Change

1 comment:

  • File media/gpu/vaapi/vaapi_wrapper.cc:

    • @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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
Gerrit-Change-Number: 3646633
Gerrit-PatchSet: 17
Gerrit-Owner: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Kramer Ge <fang...@chromium.org>
Gerrit-Reviewer: Robert Kroeger <rjkr...@chromium.org>
Gerrit-CC: Maksim Sisov <msi...@igalia.com>
Gerrit-CC: Ted (Chromium) Meyer <tmath...@chromium.org>
Gerrit-CC: Zhenyao Mo <z...@chromium.org>
Gerrit-Attention: Kramer Ge <fang...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Attention: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Comment-Date: Tue, 13 Sep 2022 11:18:54 +0000

Andres Calderon Jaramillo (Gerrit)

unread,
Sep 14, 2022, 2:58:26 AM9/14/22
to Jianhui J Dai, feature-me...@chromium.org, media-cro...@chromium.org, poscia...@chromium.org, vaapi-...@chromium.org, ozone-...@chromium.org, Maksim Sisov, Robert Kroeger, Chromium LUCI CQ, Ted (Chromium) Meyer, Kramer Ge, Zhenyao Mo, chromium...@chromium.org

Attention is currently required from: Jianhui J Dai, Kramer Ge.

Patch set 17:Code-Review +1

View Change

15 comments:

  • Patchset:

    • Patch Set #17:

      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:

    • Patch Set #17, Line 267:


      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.

    • Patch Set #17, Line 54:

      ui::OzonePlatform::IsInitialized() && ui::OzonePlatform::GetInstance()
      ->GetPlatformProperties()
      .supports_vaapi_x11

      Assuming 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()) {
      ...
      }
      ```
    • Patch Set #17, Line 66:

      gl::kGLImplementationEGLANGLE,
      vaapi_implement_for_egl_angle

      Have you had a chance to test this on Wayland?

    • Patch Set #17, Line 109:

      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;
      ```
    • Patch Set #17, Line 120:

      #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;
      ```
    • Patch Set #17, Line 143:

      DCHECK(ui::OzonePlatform::GetInstance()
      ->GetPlatformProperties()
      .supports_vaapi_x11);

      `DCHECK(VaapiWrapper::MayUseVaapiOverX11().value_or(false));`

    • Patch Set #17, Line 152:

      DCHECK(ui::OzonePlatform::GetInstance()
      ->GetPlatformProperties()
      .supports_vaapi_x11);

      `DCHECK(VaapiWrapper::MayUseVaapiOverX11().value_or(false));`

  • File media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc:

    • Patch Set #17, Line 23:

      #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.

    • Patch Set #17, Line 433:

      ui::OzonePlatform::IsInitialized() && ui::OzonePlatform::GetInstance()
      ->GetPlatformProperties()
      .supports_vaapi_x11

      Assuming 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:

    • Patch Set #17, Line 753:

        //
      // 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.

    • Patch Set #17, Line 756:

      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 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:

    • Patch Set #17, Line 152:

      // 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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
Gerrit-Change-Number: 3646633
Gerrit-PatchSet: 17
Gerrit-Owner: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Kramer Ge <fang...@chromium.org>
Gerrit-Reviewer: Robert Kroeger <rjkr...@chromium.org>
Gerrit-CC: Maksim Sisov <msi...@igalia.com>
Gerrit-CC: Ted (Chromium) Meyer <tmath...@chromium.org>
Gerrit-CC: Zhenyao Mo <z...@chromium.org>
Gerrit-Attention: Kramer Ge <fang...@chromium.org>
Gerrit-Attention: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Comment-Date: Wed, 14 Sep 2022 06:58:13 +0000

Jianhui J Dai (Gerrit)

unread,
Sep 15, 2022, 3:15:26 AM9/15/22
to feature-me...@chromium.org, media-cro...@chromium.org, poscia...@chromium.org, vaapi-...@chromium.org, ozone-...@chromium.org, Andres Calderon Jaramillo, Maksim Sisov, Robert Kroeger, Chromium LUCI CQ, Ted (Chromium) Meyer, Kramer Ge, Zhenyao Mo, chromium...@chromium.org

Attention is currently required from: Kramer Ge.

View Change

1 comment:

  • Patchset:

    • Patch Set #17:

      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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
Gerrit-Change-Number: 3646633
Gerrit-PatchSet: 17
Gerrit-Owner: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Kramer Ge <fang...@chromium.org>
Gerrit-Reviewer: Robert Kroeger <rjkr...@chromium.org>
Gerrit-CC: Maksim Sisov <msi...@igalia.com>
Gerrit-CC: Ted (Chromium) Meyer <tmath...@chromium.org>
Gerrit-CC: Zhenyao Mo <z...@chromium.org>
Gerrit-Attention: Kramer Ge <fang...@chromium.org>
Gerrit-Comment-Date: Thu, 15 Sep 2022 07:15:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-MessageType: comment

Maksim Sisov (Gerrit)

unread,
Sep 15, 2022, 6:12:01 AM9/15/22
to Jianhui J Dai, feature-me...@chromium.org, media-cro...@chromium.org, poscia...@chromium.org, vaapi-...@chromium.org, ozone-...@chromium.org, Andres Calderon Jaramillo, Robert Kroeger, Chromium LUCI CQ, Ted (Chromium) Meyer, Kramer Ge, Zhenyao Mo, chromium...@chromium.org

Attention is currently required from: Jianhui J Dai, Kramer Ge.

View Change

1 comment:

  • Patchset:

    • Patch Set #17:

      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 VaapiVideoDecoder

      Debugging the code, it seems like it stops at [1]

      [1] https://source.chromium.org/chromium/chromium/src/+/main:media/mojo/services/gpu_mojo_media_client_cros.cc;l=80?q=media%2Fmojo%2Fservices%2Fgpu_mojo_media_client_cros.cc&ss=chromium%2Fchromium%2Fsrc

      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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
Gerrit-Change-Number: 3646633
Gerrit-PatchSet: 17
Gerrit-Owner: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Kramer Ge <fang...@chromium.org>
Gerrit-Reviewer: Robert Kroeger <rjkr...@chromium.org>
Gerrit-CC: Maksim Sisov <msi...@igalia.com>
Gerrit-CC: Ted (Chromium) Meyer <tmath...@chromium.org>
Gerrit-CC: Zhenyao Mo <z...@chromium.org>
Gerrit-Attention: Kramer Ge <fang...@chromium.org>
Gerrit-Attention: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Comment-Date: Thu, 15 Sep 2022 10:11:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Jianhui J Dai (Gerrit)

unread,
Sep 15, 2022, 10:20:32 AM9/15/22
to feature-me...@chromium.org, media-cro...@chromium.org, poscia...@chromium.org, vaapi-...@chromium.org, ozone-...@chromium.org, Andres Calderon Jaramillo, Maksim Sisov, Robert Kroeger, Chromium LUCI CQ, Ted (Chromium) Meyer, Kramer Ge, Zhenyao Mo, chromium...@chromium.org

Attention is currently required from: Kramer Ge, Maksim Sisov.

View Change

1 comment:

  • Patchset:

    • Patch Set #17:

      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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
Gerrit-Change-Number: 3646633
Gerrit-PatchSet: 17
Gerrit-Owner: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Kramer Ge <fang...@chromium.org>
Gerrit-Reviewer: Robert Kroeger <rjkr...@chromium.org>
Gerrit-CC: Maksim Sisov <msi...@igalia.com>
Gerrit-CC: Ted (Chromium) Meyer <tmath...@chromium.org>
Gerrit-CC: Zhenyao Mo <z...@chromium.org>
Gerrit-Attention: Maksim Sisov <msi...@igalia.com>
Gerrit-Attention: Kramer Ge <fang...@chromium.org>
Gerrit-Comment-Date: Thu, 15 Sep 2022 14:20:13 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Maksim Sisov <msi...@igalia.com>
Gerrit-MessageType: comment

Jianhui J Dai (Gerrit)

unread,
Sep 15, 2022, 10:28:05 AM9/15/22
to feature-me...@chromium.org, media-cro...@chromium.org, poscia...@chromium.org, vaapi-...@chromium.org, ozone-...@chromium.org, Andres Calderon Jaramillo, Maksim Sisov, Robert Kroeger, Chromium LUCI CQ, Ted (Chromium) Meyer, Kramer Ge, Zhenyao Mo, chromium...@chromium.org

Attention is currently required from: Kramer Ge, Maksim Sisov.

View Change

1 comment:

  • Patchset:

    • Patch Set #17:

      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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
Gerrit-Change-Number: 3646633
Gerrit-PatchSet: 17
Gerrit-Owner: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Kramer Ge <fang...@chromium.org>
Gerrit-Reviewer: Robert Kroeger <rjkr...@chromium.org>
Gerrit-CC: Maksim Sisov <msi...@igalia.com>
Gerrit-CC: Ted (Chromium) Meyer <tmath...@chromium.org>
Gerrit-CC: Zhenyao Mo <z...@chromium.org>
Gerrit-Attention: Maksim Sisov <msi...@igalia.com>
Gerrit-Attention: Kramer Ge <fang...@chromium.org>
Gerrit-Comment-Date: Thu, 15 Sep 2022 14:27:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Maksim Sisov <msi...@igalia.com>

Jianhui J Dai (Gerrit)

unread,
Sep 16, 2022, 3:23:51 AM9/16/22
to feature-me...@chromium.org, media-cro...@chromium.org, poscia...@chromium.org, vaapi-...@chromium.org, ozone-...@chromium.org, Andres Calderon Jaramillo, Maksim Sisov, Robert Kroeger, Chromium LUCI CQ, Ted (Chromium) Meyer, Kramer Ge, Zhenyao Mo, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo, Kramer Ge, Maksim Sisov.

View Change

15 comments:

  • Patchset:

    • Patch Set #17:

      To enable VaapiVideoDecoder, there are other future works: […]

      Update patchset-19 to make VDA work on system minigbm.

  • File media/gpu/vaapi/BUILD.gn:

    • Patch Set #17, Line 267:


      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

    • Patch Set #17, Line 54:

      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)) {
      ...
      }
      ```
    • Patch Set #17, Line 66:

      gl::kGLImplementationEGLANGLE,
      vaapi_implement_for_egl_angle

      Have you had a chance to test this on Wayland?

    • Patch Set #17, Line 109:

      if (ui::OzonePlatform::IsInitialized() && ui::OzonePlatform::GetInstance()
      ->GetPlatformProperties()
      .supports_vaapi_x11) {
      return GL_TEXTURE_2D;
      }

    • Here too: […]

      Done

    • Patch Set #17, Line 120:

      #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(VaapiWrapper::MayUseVaapiOverX11(). […]

      Done

    • `DCHECK(VaapiWrapper::MayUseVaapiOverX11(). […]

      Done

  • File media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc:

    • Patch Set #17, Line 23:

      #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

    • Patch Set #17, Line 433:

      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:

    • Patch Set #17, Line 753:

        //
      // 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

    • Patch Set #17, Line 756:

      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?

    • Patch Set #17, Line 152:

      // 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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
Gerrit-Change-Number: 3646633
Gerrit-PatchSet: 19
Gerrit-Owner: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Kramer Ge <fang...@chromium.org>
Gerrit-Reviewer: Robert Kroeger <rjkr...@chromium.org>
Gerrit-CC: Maksim Sisov <msi...@igalia.com>
Gerrit-CC: Ted (Chromium) Meyer <tmath...@chromium.org>
Gerrit-CC: Zhenyao Mo <z...@chromium.org>
Gerrit-Attention: Maksim Sisov <msi...@igalia.com>
Gerrit-Attention: Kramer Ge <fang...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Comment-Date: Fri, 16 Sep 2022 07:23:34 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Maksim Sisov <msi...@igalia.com>
Comment-In-Reply-To: Andres Calderon Jaramillo <andr...@chromium.org>

Maksim Sisov (Gerrit)

unread,
Sep 21, 2022, 2:25:52 AM9/21/22
to Jianhui J Dai, feature-me...@chromium.org, media-cro...@chromium.org, poscia...@chromium.org, vaapi-...@chromium.org, ozone-...@chromium.org, Andres Calderon Jaramillo, Robert Kroeger, Chromium LUCI CQ, Ted (Chromium) Meyer, Kramer Ge, Zhenyao Mo, chromium...@chromium.org

Attention is currently required from: Andres Calderon Jaramillo, Jianhui J Dai, Kramer Ge.

View Change

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.

Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I6d6bf781833a7752d23dafb2b63112c2fc81b17a
Gerrit-Change-Number: 3646633
Gerrit-PatchSet: 19
Gerrit-Owner: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Reviewer: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Reviewer: Kramer Ge <fang...@chromium.org>
Gerrit-Reviewer: Robert Kroeger <rjkr...@chromium.org>
Gerrit-CC: Maksim Sisov <msi...@igalia.com>
Gerrit-CC: Ted (Chromium) Meyer <tmath...@chromium.org>
Gerrit-CC: Zhenyao Mo <z...@chromium.org>
Gerrit-Attention: Kramer Ge <fang...@chromium.org>
Gerrit-Attention: Andres Calderon Jaramillo <andr...@chromium.org>
Gerrit-Attention: Jianhui J Dai <jianhu...@intel.com>
Gerrit-Comment-Date: Wed, 21 Sep 2022 06:25:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Andres Calderon Jaramillo (Gerrit)

unread,
Sep 21, 2022, 2:27:25 AM9/21/22
to Jianhui J Dai, feature-me...@chromium.org, media-cro...@chromium.org, poscia...@chromium.org, vaapi-...@chromium.org, ozone-...@chromium.org, Maksim Sisov, Robert Kroeger, Chromium LUCI CQ, Ted (Chromium) Meyer, Kramer Ge, Zhenyao Mo, chromium...@chromium.org

Attention is currently required from: Jianhui J Dai, Kramer Ge.

Patch set 19:Code-Review +1

View Change

5 comments:

    • 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.

    • 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 solution

      For 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