Refactor for media/webrtc/audio_processor [chromium/src : main]

0 views
Skip to first unread message

Sergio Solano (Gerrit)

unread,
Nov 7, 2025, 5:31:54 PM (5 days ago) Nov 7
to Colin Blundell, Eugene Zemtsov, Chromium LUCI CQ, Ale Bzk, chromium...@chromium.org, Per Åhgren, Sam Zackrisson, feature-me...@chromium.org
Attention needed from Colin Blundell and Eugene Zemtsov

Sergio Solano voted and added 1 comment

Votes added by Sergio Solano

Auto-Submit+1
Commit-Queue+2

1 comment

Patchset-level comments
File-level comment, Patchset 1 (Latest):
Sergio Solano . resolved

Hi, could you help me with a Code Review?

Open in Gerrit

Related details

Attention is currently required from:
  • Colin Blundell
  • Eugene Zemtsov
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Owners
  • requirement is not satisfiedCode-Review
  • requirement is not satisfiedReview-Enforcement
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: chromium/src
Gerrit-Branch: main
Gerrit-Change-Id: I94b908c53e31b80455bba766e9418cb883597dbb
Gerrit-Change-Number: 7132522
Gerrit-PatchSet: 1
Gerrit-Owner: Sergio Solano <sergio...@google.com>
Gerrit-Reviewer: Colin Blundell <blun...@chromium.org>
Gerrit-Reviewer: Eugene Zemtsov <eug...@chromium.org>
Gerrit-Reviewer: Sergio Solano <sergio...@google.com>
Gerrit-CC: Ale Bzk <ales...@chromium.org>
Gerrit-CC: Per Åhgren <pe...@chromium.org>
Gerrit-CC: Sam Zackrisson <sa...@chromium.org>
Gerrit-Attention: Colin Blundell <blun...@chromium.org>
Gerrit-Attention: Eugene Zemtsov <eug...@chromium.org>
Gerrit-Comment-Date: Fri, 07 Nov 2025 22:31:45 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
satisfied_requirement
unsatisfied_requirement
open
diffy

Eugene Zemtsov (Gerrit)

unread,
Nov 10, 2025, 2:14:03 PM (2 days ago) Nov 10
to Sergio Solano, Colin Blundell, Eugene Zemtsov, Chromium LUCI CQ, Ale Bzk, chromium...@chromium.org, Per Åhgren, Sam Zackrisson, feature-me...@chromium.org
Attention needed from Colin Blundell and Sergio Solano

Eugene Zemtsov added 4 comments

File media/webrtc/audio_processor.cc
Line 16, Patchset 2 (Latest):#include "base/containers/span.h"
Eugene Zemtsov . unresolved

i don't think we need it, since it's already included in the header file

Line 99, Patchset 2 (Latest):
Eugene Zemtsov . unresolved

we don't need this line

Line 119, Patchset 2 (Latest): std::vector<float*> channel_ptrs_;
Eugene Zemtsov . unresolved

I think we can make it `vector<span<float>>` and it'll be nicer and safer

Line 479, Patchset 2 (Latest): base::span<float* const> output_ptrs) {
Eugene Zemtsov . unresolved

can we make it `span<span<float>>` and create whose spans (with UNSAFE_TODO) where we call this?

Open in Gerrit

Related details

Attention is currently required from:
  • Colin Blundell
  • Sergio Solano
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Owners
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: I94b908c53e31b80455bba766e9418cb883597dbb
    Gerrit-Change-Number: 7132522
    Gerrit-PatchSet: 2
    Gerrit-Owner: Sergio Solano <sergio...@google.com>
    Gerrit-Reviewer: Colin Blundell <blun...@chromium.org>
    Gerrit-Reviewer: Eugene Zemtsov <eug...@chromium.org>
    Gerrit-Reviewer: Sergio Solano <sergio...@google.com>
    Gerrit-CC: Ale Bzk <ales...@chromium.org>
    Gerrit-CC: Per Åhgren <pe...@chromium.org>
    Gerrit-CC: Sam Zackrisson <sa...@chromium.org>
    Gerrit-Attention: Colin Blundell <blun...@chromium.org>
    Gerrit-Attention: Sergio Solano <sergio...@google.com>
    Gerrit-Comment-Date: Mon, 10 Nov 2025 19:13:54 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Sergio Solano (Gerrit)

    unread,
    Nov 10, 2025, 3:45:02 PM (2 days ago) Nov 10
    to Colin Blundell, Eugene Zemtsov, Chromium LUCI CQ, Ale Bzk, chromium...@chromium.org, Per Åhgren, Sam Zackrisson, feature-me...@chromium.org
    Attention needed from Colin Blundell and Eugene Zemtsov

    Sergio Solano voted and added 5 comments

    Votes added by Sergio Solano

    Auto-Submit+1

    5 comments

    Patchset-level comments
    File-level comment, Patchset 3 (Latest):
    Sergio Solano . resolved

    Hi, Eugene. I've responded and implemented some of your sugestions. Can you help me with another check?

    File media/webrtc/audio_processor.cc
    Line 16, Patchset 2:#include "base/containers/span.h"
    Eugene Zemtsov . resolved

    i don't think we need it, since it's already included in the header file

    Sergio Solano

    Done

    Line 99, Patchset 2:
    Eugene Zemtsov . resolved

    we don't need this line

    Sergio Solano

    Done

    Line 119, Patchset 2: std::vector<float*> channel_ptrs_;
    Eugene Zemtsov . resolved

    I think we can make it `vector<span<float>>` and it'll be nicer and safer

    Sergio Solano

    Changing channel_ptrs_ to std::vector<base::span<float>> would add overhead and complexity because the WebRTC ProcessStream API requires raw float** style arguments (we would need to convert back from spans on each call). The current approach seems more direct and readable. The necessary safety guarantees for buffer access are provided by media::AudioBus.

    Line 479, Patchset 2: base::span<float* const> output_ptrs) {
    Eugene Zemtsov . resolved

    can we make it `span<span<float>>` and create whose spans (with UNSAFE_TODO) where we call this?

    Sergio Solano

    I think we wold just move where the UNSAFE macro is, not avoiding it. Additionally, ProcessData would still need to extract the raw float* pointers to call the WebRTC ProcessStream API.

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Colin Blundell
    • Eugene Zemtsov
    Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement is not satisfiedCode-Owners
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedReview-Enforcement
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I94b908c53e31b80455bba766e9418cb883597dbb
      Gerrit-Change-Number: 7132522
      Gerrit-PatchSet: 3
      Gerrit-Owner: Sergio Solano <sergio...@google.com>
      Gerrit-Reviewer: Colin Blundell <blun...@chromium.org>
      Gerrit-Reviewer: Eugene Zemtsov <eug...@chromium.org>
      Gerrit-Reviewer: Sergio Solano <sergio...@google.com>
      Gerrit-CC: Ale Bzk <ales...@chromium.org>
      Gerrit-CC: Per Åhgren <pe...@chromium.org>
      Gerrit-CC: Sam Zackrisson <sa...@chromium.org>
      Gerrit-Attention: Colin Blundell <blun...@chromium.org>
      Gerrit-Attention: Eugene Zemtsov <eug...@chromium.org>
      Gerrit-Comment-Date: Mon, 10 Nov 2025 20:44:57 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: Yes
      Comment-In-Reply-To: Eugene Zemtsov <eug...@chromium.org>
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Eugene Zemtsov (Gerrit)

      unread,
      Nov 10, 2025, 7:21:51 PM (2 days ago) Nov 10
      to Sergio Solano, Eugene Zemtsov, Thomas Guilbert, Colin Blundell, Chromium LUCI CQ, Ale Bzk, chromium...@chromium.org, Per Åhgren, Sam Zackrisson, feature-me...@chromium.org
      Attention needed from Colin Blundell and Sergio Solano

      Eugene Zemtsov voted and added 1 comment

      Votes added by Eugene Zemtsov

      Code-Review+1

      1 comment

      File media/webrtc/audio_processor.cc
      Line 564, Patchset 3 (Latest): auto dest = UNSAFE_BUFFERS(base::span(
      Eugene Zemtsov . unresolved

      I feel like these need to be `UNSAFE_TODO`, because ultimately we want all buffers to carry their size

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Colin Blundell
      • Sergio Solano
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement satisfiedCode-Owners
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      • requirement satisfiedReview-Enforcement
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I94b908c53e31b80455bba766e9418cb883597dbb
      Gerrit-Change-Number: 7132522
      Gerrit-PatchSet: 3
      Gerrit-Owner: Sergio Solano <sergio...@google.com>
      Gerrit-Reviewer: Colin Blundell <blun...@chromium.org>
      Gerrit-Reviewer: Eugene Zemtsov <eug...@chromium.org>
      Gerrit-Reviewer: Sergio Solano <sergio...@google.com>
      Gerrit-CC: Ale Bzk <ales...@chromium.org>
      Gerrit-CC: Per Åhgren <pe...@chromium.org>
      Gerrit-CC: Sam Zackrisson <sa...@chromium.org>
      Gerrit-CC: Thomas Guilbert <tgui...@chromium.org>
      Gerrit-Attention: Colin Blundell <blun...@chromium.org>
      Gerrit-Attention: Sergio Solano <sergio...@google.com>
      Gerrit-Comment-Date: Tue, 11 Nov 2025 00:21:42 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: Yes
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy

      Colin Blundell (Gerrit)

      unread,
      4:20 AM (14 hours ago) 4:20 AM
      to Sergio Solano, Colin Blundell, Eugene Zemtsov, Thomas Guilbert, Chromium LUCI CQ, Ale Bzk, chromium...@chromium.org, Per Åhgren, Sam Zackrisson, feature-me...@chromium.org
      Attention needed from Eugene Zemtsov and Sergio Solano

      Colin Blundell voted and added 1 comment

      Votes added by Colin Blundell

      Code-Review+1

      1 comment

      Patchset-level comments
      File-level comment, Patchset 4 (Latest):
      Colin Blundell . resolved

      Thanks!

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Eugene Zemtsov
      • Sergio Solano
      Submit Requirements:
      • requirement satisfiedCode-Coverage
      • requirement satisfiedCode-Owners
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      • requirement satisfiedReview-Enforcement
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: comment
      Gerrit-Project: chromium/src
      Gerrit-Branch: main
      Gerrit-Change-Id: I94b908c53e31b80455bba766e9418cb883597dbb
      Gerrit-Change-Number: 7132522
      Gerrit-PatchSet: 4
      Gerrit-Owner: Sergio Solano <sergio...@google.com>
      Gerrit-Reviewer: Colin Blundell <blun...@chromium.org>
      Gerrit-Reviewer: Eugene Zemtsov <eug...@chromium.org>
      Gerrit-Reviewer: Sergio Solano <sergio...@google.com>
      Gerrit-CC: Ale Bzk <ales...@chromium.org>
      Gerrit-CC: Per Åhgren <pe...@chromium.org>
      Gerrit-CC: Sam Zackrisson <sa...@chromium.org>
      Gerrit-CC: Thomas Guilbert <tgui...@chromium.org>
      Gerrit-Attention: Eugene Zemtsov <eug...@chromium.org>
      Gerrit-Attention: Sergio Solano <sergio...@google.com>
      Gerrit-Comment-Date: Wed, 12 Nov 2025 09:20:19 +0000
      Gerrit-HasComments: Yes
      Gerrit-Has-Labels: Yes
      satisfied_requirement
      unsatisfied_requirement
      open
      diffy
      Reply all
      Reply to author
      Forward
      0 new messages