Mirroring: Wire EncodeCommandBufferId to VEA and fix V4L2 VEA threading [chromium/src : main]

0 views

Skip to first unread message

Vikas Soni (Gerrit)

unread,

Jan 16, 2026, 2:48:40 PM (2 days ago) Jan 16

to Mark Foltz, Nathan Hebert, Fritz Koenig, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, media-cro...@chromium.org, chromeos-gfx-...@google.com, feature-me...@chromium.org, jasonrobe...@google.com, jophba...@chromium.org, mfoltz+wa...@chromium.org

Attention needed from Mark Foltz

Vikas Soni added 4 comments

Patchset-level comments

File-level comment, Patchset 15 (Latest):

Vikas Soni . resolved

+Mark for reviews.

File media/gpu/v4l2/v4l2_video_encode_accelerator.cc

Line 1195, Patchset 13: // FIX: Move thread-bound resources to the Child Thread for safe destruction.

Nathan Hebert . resolved

I don't see similar usage of "FIX: " prefixes in the media code tree. Maybe:
```suggestion
  // Moves thread-bound resources to the Child Thread for safe destruction.
```

Vikas Soni

Done

Line 1197, Patchset 13: client_ptr_factory_.reset();

Nathan Hebert . resolved

Does this code branch ever get hit? Above, we DCHECK (not checked in prod) that this is called on the `encoder_sequence_checker_.` If not, can we remove it and always post to the `child_task_runner_`?

Vikas Soni

Done

Line 1744, Patchset 13://

Nathan Hebert . resolved

Nit: This function comment is already in the .h file where it is defined. Can you remove it here?

Vikas Soni

Done

Open in Gerrit

Related details

Attention is currently required from:

Mark Foltz

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Mark Foltz (Gerrit)

unread,

Jan 16, 2026, 7:02:29 PM (2 days ago) Jan 16

to Vikas Soni, Jordan Bayles, Mark Foltz, Nathan Hebert, Fritz Koenig, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, media-cro...@chromium.org, chromeos-gfx-...@google.com, feature-me...@chromium.org, jasonrobe...@google.com, jophba...@chromium.org, mfoltz+wa...@chromium.org

Attention needed from Jordan Bayles and Vikas Soni

Mark Foltz added 7 comments

Patchset-level comments

File-level comment, Patchset 15 (Latest):

Mark Foltz . resolved

I'm not super familar with the V4L2 code, but did have some comments on the mirroring code.

File components/mirroring/service/mirroring_gpu_factories_factory.h

Line 30, Patchset 15 (Latest):// to be in an invalid state.

Mark Foltz . unresolved

Please update the class documentation to explain context_configured_cb.

File components/mirroring/service/mirroring_gpu_factories_factory.cc

Line 31, Patchset 15 (Latest): context_configured_cb_(std::move(context_configured_cb)) {}

Mark Foltz . unresolved

nit: can you

`#include <utility>`

Line 105, Patchset 15 (Latest): command_buffer_proxy->GetGpuChannel().GetChannelToken(base::BindOnce(

Mark Foltz . unresolved

Can you add a comment explaining why `base::Unretained(this)` is safe here? In particular what if `OnContextLost()` is called before `OnChannelTokenReady()`?

File components/mirroring/service/openscreen_session_host.cc

Line 689, Patchset 15 (Latest): route_id_ = route_id;

Mark Foltz . unresolved

Is route_id_ guaranteed to be non-zero?

Line 980, Patchset 15 (Latest):void OpenscreenSessionHost::OnGpuFactoryContextLost(

Mark Foltz . unresolved

Should this do something with any queued callbacks in `pending_vea_requests_`?

Line 993, Patchset 15 (Latest): route_id_ = 0;

Mark Foltz . unresolved

If this is called before OnGpuFactoriesConfigured(), will we get an invalid route id/token set returned to callers?

Open in Gerrit

Related details

Attention is currently required from:

Jordan Bayles
Vikas Soni

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

No-Unresolved-Comments
Review-Enforcement

Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings.

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Reply all

Reply to author

Forward

0 new messages