Weidong Liu (Gerrit)

unread,

Nov 7, 2025, 4:06:28 AM (6 days ago) Nov 7

to Dale Curtis, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, fuzzin...@chromium.org, feature-me...@chromium.org

Attention needed from Dale Curtis

Weidong Liu added 1 comment

Patchset-level comments

File-level comment, Patchset 3 (Latest):

Weidong Liu . resolved

Hi, Dale. PTAL.

Open in Gerrit

Related details

Attention is currently required from:

Dale Curtis

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

Thomas Guilbert (Gerrit)

unread,

Nov 10, 2025, 3:40:45 PM (3 days ago) Nov 10

to Weidong Liu, Thomas Guilbert, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, fuzzin...@chromium.org, feature-me...@chromium.org

Attention needed from Weidong Liu

Thomas Guilbert added 5 comments

File media/formats/mp2t/es_parser.cc

Line 35, Patchset 3 (Latest): DCHECK(!buf.empty());

Thomas Guilbert . unresolved

Normally, when there's a CHECK, we don't handle it below. CHECKs are for "This never happens" invariants.

I would remove this check since we handle it below.

File media/formats/mp2t/es_parser_adts.cc

Line 14, Patchset 3 (Latest):#include "base/memory/raw_ptr.h"

Thomas Guilbert . unresolved

Can `raw_ptr.h` be removed?

Line 83, Patchset 3 (Latest): size_t remaining_size = es_size - offset;

Thomas Guilbert . unresolved

Same as `cur_buf.size()`?

File media/formats/mp2t/es_parser_h264.cc

Line 50, Patchset 3 (Latest): const uint32_t value = base::U32FromNativeEndian(buffer.first<4u>());

Thomas Guilbert . unresolved

I don't think that native endianness should be used here. There would be a different result based on the platform.

I think you want big-endian here. An easy way to confirm would be to try little endian and see if things are broken.

Line 86, Patchset 3 (Latest):

                base::checked_cast<size_t>(search_pos),
                base::checked_cast<size_t>(end_pos - search_pos))))) {

Thomas Guilbert . unresolved

NIT: Any way to update some data type to avoid `checked_cast`s here?

Open in Gerrit

Related details

Attention is currently required from:

Weidong Liu

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

Weidong Liu (Gerrit)

unread,

Nov 10, 2025, 9:29:29 PM (2 days ago) Nov 10

to Thomas Guilbert, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, fuzzin...@chromium.org, feature-me...@chromium.org

Attention needed from Thomas Guilbert

Weidong Liu voted and added 5 comments

Votes added by Weidong Liu

Commit-Queue

+1

5 comments

File media/formats/mp2t/es_parser.cc

Line 35, Patchset 3: DCHECK(!buf.empty());

Thomas Guilbert . resolved

Normally, when there's a CHECK, we don't handle it below. CHECKs are for "This never happens" invariants.
I would remove this check since we handle it below.

Weidong Liu

Done

File media/formats/mp2t/es_parser_adts.cc

Line 14, Patchset 3:#include "base/memory/raw_ptr.h"

Thomas Guilbert . resolved

Can `raw_ptr.h` be removed?

Weidong Liu

Done

Line 83, Patchset 3: size_t remaining_size = es_size - offset;

Thomas Guilbert . resolved

Same as `cur_buf.size()`?

Weidong Liu

Done

File media/formats/mp2t/es_parser_h264.cc

Line 50, Patchset 3: const uint32_t value = base::U32FromNativeEndian(buffer.first<4u>());

Thomas Guilbert . resolved

I don't think that native endianness should be used here. There would be a different result based on the platform.
I think you want big-endian here. An easy way to confirm would be to try little endian and see if things are broken.

Weidong Liu

Done. Thank you for your correction.

Line 86, Patchset 3: base::checked_cast<size_t>(search_pos),


                base::checked_cast<size_t>(end_pos - search_pos))))) {

Thomas Guilbert . resolved

NIT: Any way to update some data type to avoid `checked_cast`s here?

Weidong Liu

This requires converting the type of `protected_blocks_`. I anticipate this will involve a wider range of modifications. For now, let's leave it as is.

Open in Gerrit

Related details

Attention is currently required from:

Thomas Guilbert

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

Thomas Guilbert (Gerrit)

unread,

Nov 12, 2025, 2:00:26 PM (16 hours ago) Nov 12

to Weidong Liu, Thomas Guilbert, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, fuzzin...@chromium.org, feature-me...@chromium.org

Attention needed from Weidong Liu

Thomas Guilbert added 4 comments

File media/formats/mp2t/es_parser_adts.cc

Line 29, Patchset 4 (Latest):

static int ExtractAdtsFrameSize(base::span<const uint8_t> adts_header) {
  return ((static_cast<int>(adts_header[5]) >> 5) |
          (static_cast<int>(adts_header[4]) << 3) |
          ((static_cast<int>(adts_header[3]) & 0x3) << 11));

Thomas Guilbert . unresolved

NIT: Use `size_t` and get rid of `checked_cast<size_t>` below

Line 62, Patchset 4 (Latest): size_t es_size = es_queue_->Data().size();

Thomas Guilbert . unresolved

NIT: If we remove `es_size` below, using `es.size()` directly would be clearer, since there would be only two uses.

Line 74, Patchset 4 (Latest): size_t frame_size =

Thomas Guilbert . unresolved

NIT: Const

Line 96, Patchset 4 (Latest): es_size = es_queue_->Data().size();

Thomas Guilbert . unresolved

No longer needed?

Open in Gerrit

Related details

Attention is currently required from:

Weidong Liu

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

Weidong Liu (Gerrit)

unread,

Nov 12, 2025, 9:27:54 PM (8 hours ago) Nov 12

to Thomas Guilbert, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, fuzzin...@chromium.org, feature-me...@chromium.org

Attention needed from Thomas Guilbert

Weidong Liu voted and added 4 comments

Votes added by Weidong Liu

Commit-Queue

+1

4 comments

File media/formats/mp2t/es_parser_adts.cc

Line 29, Patchset 4:static int ExtractAdtsFrameSize(base::span<const uint8_t> adts_header) {


  return ((static_cast<int>(adts_header[5]) >> 5) |
          (static_cast<int>(adts_header[4]) << 3) |
          ((static_cast<int>(adts_header[3]) & 0x3) << 11));

Thomas Guilbert . resolved

NIT: Use `size_t` and get rid of `checked_cast<size_t>` below

Weidong Liu

Done

Line 62, Patchset 4: size_t es_size = es_queue_->Data().size();

Thomas Guilbert . resolved

NIT: If we remove `es_size` below, using `es.size()` directly would be clearer, since there would be only two uses.

Weidong Liu

Done

Line 74, Patchset 4: size_t frame_size =

Thomas Guilbert . resolved

NIT: Const

Weidong Liu

Done

Line 96, Patchset 4: es_size = es_queue_->Data().size();

Thomas Guilbert . resolved

No longer needed?

Weidong Liu

Done

Open in Gerrit

Related details

Attention is currently required from:

Thomas Guilbert

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

Thomas Guilbert (Gerrit)

unread,

12:26 AM (5 hours ago) 12:26 AM

to Weidong Liu, Thomas Guilbert, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, fuzzin...@chromium.org, feature-me...@chromium.org

Attention needed from 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 5 (Latest):

Thomas Guilbert . resolved

Thank you for the cleanup!

File media/formats/mp2t/es_parser_adts.cc

Line 14, Patchset 5 (Latest):#include "base/numerics/safe_conversions.h"

Thomas Guilbert . unresolved

NIT: Probably no longer needed.

Open in Gerrit

Related details

Attention is currently required from:

Weidong Liu

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

Weidong Liu (Gerrit)

unread,

1:49 AM (4 hours ago) 1:49 AM

to Thomas Guilbert, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, fuzzin...@chromium.org, feature-me...@chromium.org

Weidong Liu voted and added 2 comments

Votes added by Weidong Liu

Commit-Queue

+2

2 comments

Patchset-level comments

File-level comment, Patchset 5 (Latest):

Weidong Liu . resolved

Thanks.

File media/formats/mp2t/es_parser_adts.cc

Line 14, Patchset 5 (Latest):#include "base/numerics/safe_conversions.h"

Thomas Guilbert . resolved

NIT: Probably no longer needed.

Weidong Liu

Done

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

Weidong Liu (Gerrit)

unread,

1:49 AM (4 hours ago) 1:49 AM

to Thomas Guilbert, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, fuzzin...@chromium.org, feature-me...@chromium.org

Weidong Liu voted Commit-Queue+0

Commit-Queue

+0

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

Weidong Liu (Gerrit)

unread,

1:51 AM (4 hours ago) 1:51 AM

to Thomas Guilbert, AyeAye, Chromium LUCI CQ, chromium...@chromium.org, fuzzin...@chromium.org, feature-me...@chromium.org

Weidong Liu 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,

3:14 AM (2 hours ago) 3:14 AM

to Weidong Liu, Thomas Guilbert, AyeAye, chromium...@chromium.org, fuzzin...@chromium.org, feature-me...@chromium.org

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

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

```
The name of the file: media/formats/mp2t/es_parser_adts.cc
Insertions: 0, Deletions: 1.

@@ -12,7 +12,6 @@
 #include "base/containers/span.h"
 #include "base/logging.h"
 #include "base/numerics/safe_conversions.h"
-#include "base/strings/string_number_conversions.h"
 #include "media/base/audio_timestamp_helper.h"
 #include "media/base/bit_reader.h"
 #include "media/base/channel_layout.h"
```

Change information

Commit message:

spanification: spanify some parts of EsParser.

Bug: 40284755

Cq-Include-Trybots: luci.chromium.try:linux-cast-x64-rel

Change-Id: I9c1d316a862ebcacb05bc4d402a4dd0dac061d4a

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

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

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

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

Files:

M media/formats/mp2t/es_adapter_video_unittest.cc
M media/formats/mp2t/es_parser.cc
M media/formats/mp2t/es_parser.h
M media/formats/mp2t/es_parser_adts.cc
M media/formats/mp2t/es_parser_adts.h
M media/formats/mp2t/es_parser_adts_fuzzer.cc
M media/formats/mp2t/es_parser_h264.cc
M media/formats/mp2t/es_parser_h264_fuzzer.cc
M media/formats/mp2t/es_parser_mpeg1audio_fuzzer.cc
M media/formats/mp2t/es_parser_test_base.cc
M media/formats/mp2t/ts_section_pes.cc
M media/formats/mp2t/ts_section_pes.h

Change size: M

Delta: 12 files changed, 99 insertions(+), 124 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +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

spanification: spanify some parts of EsParser. [chromium/src : main]

Weidong Liu (Gerrit)

Weidong Liu added 1 comment

Related details

Thomas Guilbert (Gerrit)

Thomas Guilbert added 5 comments

Related details

Weidong Liu (Gerrit)

Weidong Liu voted and added 5 comments

Votes added by Weidong Liu

5 comments

Related details

Thomas Guilbert (Gerrit)

Thomas Guilbert added 4 comments

Related details

Weidong Liu (Gerrit)

Weidong Liu voted and added 4 comments

Votes added by Weidong Liu

4 comments

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 2 comments

Votes added by Weidong Liu

2 comments

Related details

Weidong Liu (Gerrit)

Weidong Liu voted Commit-Queue+0

Related details

Weidong Liu (Gerrit)

Weidong Liu voted Commit-Queue+2

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change with unreviewed changes

Unreviewed changes

Change information