Ted (Chromium) Meyer (Gerrit)

unread,

Dec 5, 2025, 4:29:11 PMDec 5

to chromium...@chromium.org, blink-...@chromium.org, erickun...@chromium.org, feature-me...@chromium.org, jophba...@chromium.org, kinuko...@chromium.org, mfoltz+wa...@chromium.org

Attention needed from Frank Liberato

Ted (Chromium) Meyer voted and added 1 comment

Votes added by Ted (Chromium) Meyer

Commit-Queue

+1

1 comment

Patchset-level comments

File-level comment, Patchset 7 (Latest):

Ted (Chromium) Meyer . resolved

This was my alternative option to plumbing the "SetTrackState" callback into HlsManifestDemuxerEngine. I just didn't think it made a lot of sense to have so many callbacks all stored in the HMDE directly.

Open in Gerrit

Related details

Attention is currently required from:

Frank Liberato

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

Ted (Chromium) Meyer (Gerrit)

unread,

Dec 18, 2025, 7:52:14 PM (2 days ago) Dec 18

to Chromium LUCI CQ, chromium...@chromium.org, blink-...@chromium.org, erickun...@chromium.org, feature-me...@chromium.org, jophba...@chromium.org, kinuko...@chromium.org, mfoltz+wa...@chromium.org

Attention needed from Frank Liberato

Ted (Chromium) Meyer voted and added 1 comment

Votes added by Ted (Chromium) Meyer

Commit-Queue

+1

1 comment

Patchset-level comments

File-level comment, Patchset 9 (Latest):

Ted (Chromium) Meyer . resolved

@Frank, PTAL. Vast majority of changes to demuxer_manager.cc are just moving methods around to share #if DEFINED(...) blocks.

Open in Gerrit

Related details

Attention is currently required from:

Frank Liberato

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

Frank Liberato (Gerrit)

unread,

Dec 19, 2025, 11:44:23 AM (17 hours ago) Dec 19

to Ted (Chromium) Meyer, Chromium LUCI CQ, chromium...@chromium.org, blink-...@chromium.org, erickun...@chromium.org, feature-me...@chromium.org, jophba...@chromium.org, kinuko...@chromium.org, mfoltz+wa...@chromium.org

Attention needed from Ted (Chromium) Meyer

Frank Liberato voted and added 4 comments

Votes added by Frank Liberato

Code-Review

+1

4 comments

Patchset-level comments

File-level comment, Patchset 9 (Latest):

Frank Liberato . resolved

seems reasonable. i don't know if it can be untangled further but it's an improvement for sure.

-fl

Commit Message

Line 11, Patchset 7:+-------------------------+ +--------------------------+

Frank Liberato . resolved

let me guess.. beamer.

File media/base/media_track.h

Line 128, Patchset 7:

// don't line up, with destructors posted across thread hops, so prior to this
// implementation, most of the calls were wrapped in repeating callbacks with

Frank Liberato . unresolved

nit: in ten years when somebody fiddles with this code next, they won't care what it was like before this interface.

File third_party/blink/renderer/platform/media/web_media_player_impl.h

Line 447, Patchset 7: std::unique_ptr<media::TrackManager> GetTrackManager() const override;

Frank Liberato . unresolved

this could use a comment that the resulting track manager can be called on any thread.

Open in Gerrit

Related details

Attention is currently required from:

Ted (Chromium) Meyer

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

Ted (Chromium) Meyer (Gerrit)

unread,

Dec 19, 2025, 2:06:45 PM (15 hours ago) Dec 19

to Chromium LUCI CQ, chromium...@chromium.org, blink-...@chromium.org, erickun...@chromium.org, feature-me...@chromium.org, jophba...@chromium.org, kinuko...@chromium.org, mfoltz+wa...@chromium.org

Ted (Chromium) Meyer added 4 comments

Patchset-level comments

File-level comment, Patchset 9 (Latest):

Frank Liberato . resolved

seems reasonable. i don't know if it can be untangled further but it's an improvement for sure.
-fl

Ted (Chromium) Meyer

You might want to re-review patch set 9, some stuff changed (including commit message)

Commit Message

Line 11, Patchset 7:+-------------------------+ +--------------------------+

Frank Liberato . resolved

let me guess.. beamer.

Ted (Chromium) Meyer

lol. no just ascii art. you should see the next CL :)

File media/base/media_track.h

Line 128, Patchset 7:

// don't line up, with destructors posted across thread hops, so prior to this
// implementation, most of the calls were wrapped in repeating callbacks with

Frank Liberato . resolved

nit: in ten years when somebody fiddles with this code next, they won't care what it was like before this interface.

Ted (Chromium) Meyer

I think you're remarking on the non-latest patch?

File third_party/blink/renderer/platform/media/web_media_player_impl.h

Line 447, Patchset 7: std::unique_ptr<media::TrackManager> GetTrackManager() const override;

Frank Liberato . resolved

this could use a comment that the resulting track manager can be called on any thread.

Ted (Chromium) Meyer

Same thing. turns out there was some serious weirdness with allowing ffmpeg demuxer to post-task the track additions, and it was breaking very many things, hence the new patch set.

Open in Gerrit

Related details

Attention set is empty

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

open

diffy

Ted (Chromium) Meyer (Gerrit)

unread,

Dec 19, 2025, 4:57:22 PM (12 hours ago) Dec 19

to Chromium LUCI CQ, chromium...@chromium.org, blink-...@chromium.org, erickun...@chromium.org, feature-me...@chromium.org, jophba...@chromium.org, kinuko...@chromium.org, mfoltz+wa...@chromium.org

Ted (Chromium) Meyer voted Commit-Queue+2

Commit-Queue

+2

Open in Gerrit

Related details

Attention set is empty

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

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Dec 19, 2025, 5:00:51 PM (12 hours ago) Dec 19

to Ted (Chromium) Meyer, chromium...@chromium.org, blink-...@chromium.org, erickun...@chromium.org, feature-me...@chromium.org, jophba...@chromium.org, kinuko...@chromium.org, mfoltz+wa...@chromium.org

Chromium LUCI CQ submitted the change

Change information

Commit message:

[HLS] Replace track op functions with helper class

As a result of the crazy media object lifetimes, the HLS player
implementation always talks back to DemuxerManager via weakly bound
repeating callbacks.

ManifestDemuxer used
 - DemuxerManager::DemuxerRequestsSeek

HlsManifestDemuxerEngine used
 - DemuxerManager::AddTrack
 - DemuxerManager::RemoveTrack

It was slightly annoying to have to create a new bound callback for
every new method that we needed to add (in this case, SetTrackState),
and the class member list was getting polluted with a bunch of same-type
repeating callbacks.

This CL accomplishes two things -
 1. Clean up the method declarations for
    AddTrack/RemoveTrack/SetTrackState. Identical methods
    were originally declared in both DemuxerManager::Client
    and MediaPlayerClient. These can be shared in a super-interface
    for both, which is now called "TrackManager".
 2. Package up all the track management callbacks into a helper
    object which can be passed as a single parameter and class
    member to HlsManifestDemuxerEngine. This also makes it easier
    to unittest.

Bug: 41393620

Change-Id: I58914609071ce0a3fa77f835e1987cbb606dfc33

Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7228978

Reviewed-by: Frank Liberato <libe...@chromium.org>

Commit-Queue: Ted (Chromium) Meyer <tmath...@chromium.org>

Cr-Commit-Position: refs/heads/main@{#1561362}

Files:

M media/base/media_track.cc
M media/base/media_track.h
M media/filters/demuxer_manager.cc
M media/filters/demuxer_manager.h
M media/filters/hls_manifest_demuxer_engine.cc
M media/filters/hls_manifest_demuxer_engine.h
M media/filters/hls_manifest_demuxer_engine_unittest.cc
M media/test/pipeline_integration_test_base.cc
M third_party/blink/renderer/platform/media/media_player_client.h
M third_party/blink/renderer/platform/media/web_media_player_impl.cc
M third_party/blink/renderer/platform/media/web_media_player_impl.h

Change size: L

Delta: 11 files changed, 152 insertions(+), 114 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Frank Liberato

Open in Gerrit

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

Gerrit

open

diffy

satisfied_requirement

Reply all

Reply to author

Forward

[HLS] Replace track op functions with helper class [chromium/src : main]

Ted (Chromium) Meyer (Gerrit)

Ted (Chromium) Meyer voted and added 1 comment

Votes added by Ted (Chromium) Meyer

1 comment

Related details

Ted (Chromium) Meyer (Gerrit)

Ted (Chromium) Meyer voted and added 1 comment

Votes added by Ted (Chromium) Meyer

1 comment

Related details

Frank Liberato (Gerrit)

Frank Liberato voted and added 4 comments

Votes added by Frank Liberato

4 comments

Related details

Ted (Chromium) Meyer (Gerrit)

Ted (Chromium) Meyer added 4 comments

Related details

Ted (Chromium) Meyer (Gerrit)

Ted (Chromium) Meyer voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information