Weidong Liu (Gerrit)

unread,

Jun 30, 2025, 12:13:05 AM6/30/25

to Markus Handell, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, Rijubrata Bhaumik, oshima...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org

Attention needed from Markus Handell

Weidong Liu added 1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Weidong Liu . resolved

Hi, Markus. Can you take a look?

Open in Gerrit

Related details

Attention is currently required from:

Markus Handell

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Weidong Liu (Gerrit)

unread,

Jun 30, 2025, 12:15:52 AM6/30/25

to Markus Handell, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, Rijubrata Bhaumik, oshima...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org

Attention needed from Frank Liberato and Markus Handell

Weidong Liu added 1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Weidong Liu . resolved

Hi, Frank. Can you review //media/formats/mp4?

Open in Gerrit

Related details

Attention is currently required from:

Frank Liberato
Markus Handell

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Weidong Liu (Gerrit)

unread,

Jun 30, 2025, 12:17:10 AM6/30/25

to Markus Handell, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, Rijubrata Bhaumik, oshima...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org

Attention needed from Frank Liberato and Markus Handell

Weidong Liu added 1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Weidong Liu . resolved

Hi, Frank. Can you review //media/formats/mp4?

Weidong Liu

also //media/filters.

satisfied_requirement

unsatisfied_requirement

open

diffy

Markus Handell (Gerrit)

unread,

Jun 30, 2025, 5:00:38 AM6/30/25

to Weidong Liu, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, Rijubrata Bhaumik, oshima...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org

Attention needed from Frank Liberato and Weidong Liu

Markus Handell voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Frank Liberato
Weidong Liu

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Weidong Liu (Gerrit)

unread,

Jul 3, 2025, 11:18:25 PM7/3/25

to Thomas Guilbert, Markus Handell, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, Rijubrata Bhaumik, oshima...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org

Attention needed from Thomas Guilbert

Weidong Liu voted and added 1 comment

Votes added by Weidong Liu

Commit-Queue

+1

1 comment

Patchset-level comments

File-level comment, Patchset 4 (Latest):

Weidong Liu . resolved

frank is ooo. Thomas, PTAL.

Open in Gerrit

Related details

Attention is currently required from:

Thomas Guilbert

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Thomas Guilbert (Gerrit)

unread,

Jul 7, 2025, 4:07:48 PM7/7/25

to Weidong Liu, Thomas Guilbert, Markus Handell, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, Rijubrata Bhaumik, oshima...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org

Attention needed from Markus Handell and Weidong Liu

Thomas Guilbert added 7 comments

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Thomas Guilbert . resolved

Looks good in general. TY for the cleanups!

File media/filters/hls_data_source_provider.h

Line 108, Patchset 5 (Latest): const uint8_t* raw_data() const { return buffer_.data(); }

Thomas Guilbert . unresolved

I think there are relatively few uses of this field, all of which are used to create a span. Could you switch the `raw_data()` usage to `data()` too? This would be fine as a small follow up too.

File media/filters/hls_manifest_demuxer_engine.cc

Line 975, Patchset 5 (Latest): base::span(stream->raw_data(), stream->buffer_size());

Thomas Guilbert . unresolved

Use `stream->data()` directly.

File media/formats/mp4/box_reader_unittest.cc

Line 102, Patchset 5 (Latest): base::span(buf).copy_from_nonoverlapping(data);

Thomas Guilbert . unresolved

You can also use `buf.assign(data.begin(), data.end())` here.

File media/formats/mp4/mp4_box_reader_fuzzer.cc

Line 12, Patchset 5 (Latest):#include "base/compiler_specific.h"

Thomas Guilbert . unresolved

Is this include necessary?

File media/muxers/webm_muxer.cc

Line 94, Patchset 5 (Latest): writer.WriteU32NativeEndian(params.sample_rate());

Thomas Guilbert . unresolved

[Per Spec](https://wiki.xiph.org/OggOpus#ID_Header), I think these should all be little endian.

Line 99, Patchset 5 (Latest): writer.WriteU8NativeEndian(0);

Thomas Guilbert . unresolved

NIT: add `// Set channel mapping family (Mono or L/R Stereo).` comment above.

Open in Gerrit

Related details

Attention is currently required from:

Markus Handell
Weidong Liu

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review
No-Unresolved-Comments

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Weidong Liu (Gerrit)

unread,

Jul 8, 2025, 12:24:19 AM7/8/25

to Xiaoqian Dai, Thomas Guilbert, Markus Handell, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, Rijubrata Bhaumik, oshima...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org

Attention needed from Markus Handell, Thomas Guilbert and Xiaoqian Dai

Weidong Liu voted and added 7 comments

Votes added by Weidong Liu

Commit-Queue

+1

7 comments

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Weidong Liu . resolved

Hi, xdai@. PTAL.

File media/filters/hls_data_source_provider.h

Line 108, Patchset 5: const uint8_t* raw_data() const { return buffer_.data(); }

Thomas Guilbert . resolved

I think there are relatively few uses of this field, all of which are used to create a span. Could you switch the `raw_data()` usage to `data()` too? This would be fine as a small follow up too.

Weidong Liu

Done

File media/filters/hls_manifest_demuxer_engine.cc

Line 975, Patchset 5: base::span(stream->raw_data(), stream->buffer_size());

Thomas Guilbert . resolved

Use `stream->data()` directly.

Weidong Liu

Done

File media/formats/mp4/box_reader_unittest.cc

Line 102, Patchset 5: base::span(buf).copy_from_nonoverlapping(data);

Thomas Guilbert . resolved

You can also use `buf.assign(data.begin(), data.end())` here.

Weidong Liu

Done

File media/formats/mp4/mp4_box_reader_fuzzer.cc

Line 12, Patchset 5:#include "base/compiler_specific.h"

Thomas Guilbert . resolved

Is this include necessary?

Weidong Liu

`UNSAFE_TODO` is defined in this file. According to IWYU, this should be added.

File media/muxers/webm_muxer.cc

Line 94, Patchset 5: writer.WriteU32NativeEndian(params.sample_rate());

Thomas Guilbert . resolved

[Per Spec](https://wiki.xiph.org/OggOpus#ID_Header), I think these should all be little endian.

Weidong Liu

Done

Line 99, Patchset 5: writer.WriteU8NativeEndian(0);

Thomas Guilbert . resolved

NIT: add `// Set channel mapping family (Mono or L/R Stereo).` comment above.

Weidong Liu

Done

Open in Gerrit

Related details

Attention is currently required from:

Markus Handell
Thomas Guilbert
Xiaoqian Dai

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Xiaoqian Dai (Gerrit)

unread,

Jul 8, 2025, 12:52:27 PM7/8/25

to Weidong Liu, Xiaoqian Dai, Thomas Guilbert, Markus Handell, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, Rijubrata Bhaumik, oshima...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org

Attention needed from Markus Handell, Thomas Guilbert and Weidong Liu

Xiaoqian Dai voted and added 1 comment

Votes added by Xiaoqian Dai

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Xiaoqian Dai . resolved

chromeos/ash/services/recording/webm_encoder_muxer.cc LGTM

Open in Gerrit

Related details

Attention is currently required from:

Markus Handell
Thomas Guilbert
Weidong Liu

Submit Requirements:

Code-Coverage
Code-Owners

Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Thomas Guilbert (Gerrit)

unread,

Jul 8, 2025, 5:51:52 PM7/8/25

to Weidong Liu, Thomas Guilbert, Xiaoqian Dai, Markus Handell, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, Rijubrata Bhaumik, oshima...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org

Attention needed from Markus Handell and Weidong Liu

Thomas Guilbert voted and added 2 comments

Votes added by Thomas Guilbert

Code-Review

+1

2 comments

Patchset-level comments

File-level comment, Patchset 6 (Latest):

Thomas Guilbert . resolved

LGTM, thanks for the cleanups!

File media/formats/mp4/mp4_box_reader_fuzzer.cc

Line 12, Patchset 5:#include "base/compiler_specific.h"

Thomas Guilbert . resolved

Is this include necessary?

Weidong Liu

`UNSAFE_TODO` is defined in this file. According to IWYU, this should be added.

Thomas Guilbert

Ack.

Open in Gerrit

Related details

Attention is currently required from:

Markus Handell
Weidong Liu

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

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

Gerrit

satisfied_requirement

open

diffy

Weidong Liu (Gerrit)

unread,

Jul 8, 2025, 9:55:09 PM7/8/25

to Thomas Guilbert, Xiaoqian Dai, Markus Handell, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, Rijubrata Bhaumik, oshima...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org

Attention needed from Markus Handell

Weidong Liu voted and added 1 comment

Votes added by Weidong Liu

Commit-Queue

+1

1 comment

Patchset-level comments

File-level comment, Patchset 7 (Latest):

Weidong Liu . resolved

Rebase because of conflicts.

Thanks for your review. Commit it now.

Open in Gerrit

Related details

Attention is currently required from:

Markus Handell

Submit Requirements:

Code-Coverage
Code-Owners
Code-Review

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

Gerrit

satisfied_requirement

open

diffy

Weidong Liu (Gerrit)

unread,

Jul 9, 2025, 12:18:20 AM7/9/25

to Thomas Guilbert, Xiaoqian Dai, Markus Handell, Chromium LUCI CQ, AyeAye, chromium...@chromium.org, Rijubrata Bhaumik, oshima...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org

Attention needed from Markus Handell

Weidong Liu voted Commit-Queue+2

Commit-Queue

+2

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Jul 9, 2025, 1:00:05 AM7/9/25

to Weidong Liu, Thomas Guilbert, Xiaoqian Dai, Markus Handell, AyeAye, chromium...@chromium.org, Rijubrata Bhaumik, oshima...@chromium.org, chfreme...@chromium.org, feature-me...@chromium.org, fuzzin...@chromium.org

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

6 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: media/muxers/memory_webm_muxer_delegate.cc
Insertions: 7, Deletions: 2.

The diff is too large to show. Please review the diff.
```

Change information

Commit message:

Eliminate allow_unsafe_buffers in //media/muxers.

The purpose of this CL is to eliminate the unsafe buffer calls in
//media/muxers. Because it involves some functions in other folders.
For this purpose, the ReadTopLevelBox/StartTopLevelBox methods of
BoxReader and their callers are modified.

Bug: 40284755

Change-Id: I5098d3b183d37b736cb1ff126c6dc0a28d57b7c3

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

Reviewed-by: Xiaoqian Dai <xd...@chromium.org>

Reviewed-by: Thomas Guilbert <tgui...@chromium.org>

Commit-Queue: Weidong Liu <weido...@chromium.org>

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

Files:

M chromeos/ash/services/recording/webm_encoder_muxer.cc
M media/filters/hls_data_source_provider.h
M media/filters/hls_manifest_demuxer_engine.cc
M media/filters/hls_rendition_impl.cc
M media/formats/mp4/box_reader.cc
M media/formats/mp4/box_reader.h
M media/formats/mp4/box_reader_unittest.cc
M media/formats/mp4/mp4_box_reader_fuzzer.cc
M media/formats/mp4/mp4_stream_parser.cc
M media/muxers/file_webm_muxer_delegate.cc
M media/muxers/file_webm_muxer_delegate.h
M media/muxers/live_webm_muxer_delegate.cc
M media/muxers/live_webm_muxer_delegate.h
M media/muxers/memory_webm_muxer_delegate.cc
M media/muxers/memory_webm_muxer_delegate.h
M media/muxers/mp4_movie_box_writer.cc
M media/muxers/mp4_muxer_box_writer_unittest.cc
M media/muxers/mp4_muxer_delegate_fragment.cc
M media/muxers/mp4_muxer_delegate_unittest.cc
M media/muxers/output_position_tracker_unittest.cc
M media/muxers/webm_muxer.cc
M media/muxers/webm_muxer.h
M media/muxers/webm_muxer_fuzzertest.cc

Change size: L

Delta: 23 files changed, 148 insertions(+), 235 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Xiaoqian Dai, +1 by Thomas Guilbert

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

Eliminate allow_unsafe_buffers in //media/muxers. [chromium/src : main]

Weidong Liu (Gerrit)

Weidong Liu added 1 comment

Related details

Weidong Liu (Gerrit)

Weidong Liu added 1 comment

Related details

Weidong Liu (Gerrit)

Weidong Liu added 1 comment

Markus Handell (Gerrit)

Markus Handell voted Code-Review+1

Related details

Weidong Liu (Gerrit)

Weidong Liu voted and added 1 comment

Votes added by Weidong Liu

1 comment

Related details

Thomas Guilbert (Gerrit)

Thomas Guilbert added 7 comments

Related details

Weidong Liu (Gerrit)

Weidong Liu voted and added 7 comments

Votes added by Weidong Liu

7 comments

Related details

Xiaoqian Dai (Gerrit)

Xiaoqian Dai voted and added 1 comment

Votes added by Xiaoqian Dai

1 comment

Related details

Thomas Guilbert (Gerrit)

Thomas Guilbert voted and added 2 comments

Votes added by Thomas Guilbert

2 comments

Related details

Weidong Liu (Gerrit)

Weidong Liu voted and added 1 comment

Votes added by Weidong Liu

1 comment

Related details

Weidong Liu (Gerrit)

Weidong Liu voted Commit-Queue+2

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

Change information