Prototype implementation of RtpTransport::readSentRtp() batch read api [chromium/src : main]

0 views
Skip to first unread message

Tony Herre (Gerrit)

unread,
Jun 26, 2024, 9:21:30 AM (3 days ago) Jun 26
to Guido Urdaneta, Philip Eliasson, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Hiroki Nakagawa, blink-revie...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, horo+...@chromium.org, jmedle...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko+ser...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, video-networking...@google.com
Attention needed from Guido Urdaneta and Philip Eliasson

New activity on the change

Open in Gerrit

Related details

Attention is currently required from:
  • Guido Urdaneta
  • Philip Eliasson
Submit Requirements:
  • requirement satisfiedCode-Coverage
  • requirement is not satisfiedCode-Review
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: Icd63a13a7953a0b77fc589f88e739aa869847fc8
Gerrit-Change-Number: 5646499
Gerrit-PatchSet: 4
Gerrit-Owner: Tony Herre <top...@chromium.org>
Gerrit-Reviewer: Guido Urdaneta <gui...@chromium.org>
Gerrit-Reviewer: Philip Eliasson <phil...@chromium.org>
Gerrit-Reviewer: Tony Herre <top...@chromium.org>
Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>
Gerrit-CC: Kentaro Hara <har...@chromium.org>
Gerrit-Attention: Guido Urdaneta <gui...@chromium.org>
Gerrit-Attention: Philip Eliasson <phil...@chromium.org>
Gerrit-Comment-Date: Wed, 26 Jun 2024 13:21:19 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
satisfied_requirement
unsatisfied_requirement
open
diffy

Guido Urdaneta (Gerrit)

unread,
Jun 26, 2024, 11:18:14 AM (3 days ago) Jun 26
to Tony Herre, Philip Eliasson, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Hiroki Nakagawa, blink-revie...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, horo+...@chromium.org, jmedle...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko+ser...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, video-networking...@google.com
Attention needed from Philip Eliasson and Tony Herre

Guido Urdaneta added 3 comments

File third_party/blink/renderer/modules/peerconnection/rtc_rtp_transport.h
Line 31, Patchset 4: HeapVector<Member<RTCRtpAcks>> readReceivedAcks(uint32_t maxCount);
Guido Urdaneta . unresolved

Put the web-exposed methods together and add a comment to mark them as such (i.e., `//Implements Foo` or `// Implements foo.idl` or whatever style you prefer).

May apply to other files in the CL.

File third_party/blink/renderer/modules/peerconnection/rtc_rtp_transport.cc
Line 45, Patchset 4: PostCrossThreadTask(
Guido Urdaneta . unresolved

Does it make sense to call synchronously if you're already in the destination task runner?

File third_party/blink/renderer/modules/peerconnection/rtc_rtp_transport.idl
Line 11, Patchset 4: // TODO: crbug.com/345101934 - This needs to actually be on some Worker-exposed
Guido Urdaneta . unresolved

nit: Not required, but would be nice to wrap at 80 columns

Open in Gerrit

Related details

Attention is currently required from:
  • Philip Eliasson
  • Tony Herre
Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    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: Icd63a13a7953a0b77fc589f88e739aa869847fc8
    Gerrit-Change-Number: 5646499
    Gerrit-PatchSet: 4
    Gerrit-Owner: Tony Herre <top...@chromium.org>
    Gerrit-Reviewer: Guido Urdaneta <gui...@chromium.org>
    Gerrit-Reviewer: Philip Eliasson <phil...@chromium.org>
    Gerrit-Reviewer: Tony Herre <top...@chromium.org>
    Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>
    Gerrit-CC: Kentaro Hara <har...@chromium.org>
    Gerrit-Attention: Tony Herre <top...@chromium.org>
    Gerrit-Attention: Philip Eliasson <phil...@chromium.org>
    Gerrit-Comment-Date: Wed, 26 Jun 2024 15:18:04 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Tony Herre (Gerrit)

    unread,
    Jun 26, 2024, 12:01:15 PM (3 days ago) Jun 26
    to Guido Urdaneta, Philip Eliasson, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Hiroki Nakagawa, blink-revie...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, horo+...@chromium.org, jmedle...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko+ser...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, video-networking...@google.com
    Attention needed from Guido Urdaneta and Philip Eliasson

    Tony Herre added 3 comments

    File third_party/blink/renderer/modules/peerconnection/rtc_rtp_transport.h
    Line 31, Patchset 4: HeapVector<Member<RTCRtpAcks>> readReceivedAcks(uint32_t maxCount);
    Guido Urdaneta . resolved

    Put the web-exposed methods together and add a comment to mark them as such (i.e., `//Implements Foo` or `// Implements foo.idl` or whatever style you prefer).

    May apply to other files in the CL.

    Tony Herre

    Done here and in rtc_rtp_send.h

    File third_party/blink/renderer/modules/peerconnection/rtc_rtp_transport.cc
    Line 45, Patchset 4: PostCrossThreadTask(
    Guido Urdaneta . resolved

    Does it make sense to call synchronously if you're already in the destination task runner?

    Tony Herre

    In practice this is always called from the WebRTC worker thread (via InterceptingNetworkController::OnSentPacket), so I don't think it's worth the complexity. Actually, might be better to make that explicit - added a CHECK.

    File third_party/blink/renderer/modules/peerconnection/rtc_rtp_transport.idl
    Line 11, Patchset 4: // TODO: crbug.com/345101934 - This needs to actually be on some Worker-exposed
    Guido Urdaneta . resolved

    nit: Not required, but would be nice to wrap at 80 columns

    Tony Herre

    Done

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Guido Urdaneta
    • Philip Eliasson
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement is not satisfiedCode-Review
    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: Icd63a13a7953a0b77fc589f88e739aa869847fc8
    Gerrit-Change-Number: 5646499
    Gerrit-PatchSet: 6
    Gerrit-Owner: Tony Herre <top...@chromium.org>
    Gerrit-Reviewer: Guido Urdaneta <gui...@chromium.org>
    Gerrit-Reviewer: Philip Eliasson <phil...@chromium.org>
    Gerrit-Reviewer: Tony Herre <top...@chromium.org>
    Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>
    Gerrit-CC: Kentaro Hara <har...@chromium.org>
    Gerrit-Attention: Guido Urdaneta <gui...@chromium.org>
    Gerrit-Attention: Philip Eliasson <phil...@chromium.org>
    Gerrit-Comment-Date: Wed, 26 Jun 2024 16:01:04 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    Comment-In-Reply-To: Guido Urdaneta <gui...@chromium.org>
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Guido Urdaneta (Gerrit)

    unread,
    Jun 27, 2024, 6:40:25 AM (2 days ago) Jun 27
    to Tony Herre, Philip Eliasson, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Hiroki Nakagawa, blink-revie...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, horo+...@chromium.org, jmedle...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko+ser...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, video-networking...@google.com
    Attention needed from Philip Eliasson and Tony Herre

    Guido Urdaneta voted and added 1 comment

    Votes added by Guido Urdaneta

    Code-Review+1

    1 comment

    File third_party/blink/renderer/modules/peerconnection/rtc_rtp_transport.idl
    Line 7, Patchset 6 (Latest): // TODO(crbug.com/345101934): This needs to actually be on some
    Guido Urdaneta . unresolved

    super optional nit: keep the new TODO format? (BTW, I like the old one better and AFAIK it's still acceptable).

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Philip Eliasson
    • Tony Herre
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    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: Icd63a13a7953a0b77fc589f88e739aa869847fc8
    Gerrit-Change-Number: 5646499
    Gerrit-PatchSet: 6
    Gerrit-Owner: Tony Herre <top...@chromium.org>
    Gerrit-Reviewer: Guido Urdaneta <gui...@chromium.org>
    Gerrit-Reviewer: Philip Eliasson <phil...@chromium.org>
    Gerrit-Reviewer: Tony Herre <top...@chromium.org>
    Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>
    Gerrit-CC: Kentaro Hara <har...@chromium.org>
    Gerrit-Attention: Tony Herre <top...@chromium.org>
    Gerrit-Attention: Philip Eliasson <phil...@chromium.org>
    Gerrit-Comment-Date: Thu, 27 Jun 2024 10:40:12 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    unsatisfied_requirement
    open
    diffy

    Tony Herre (Gerrit)

    unread,
    Jun 28, 2024, 3:33:32 AM (yesterday) Jun 28
    to Guido Urdaneta, Philip Eliasson, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Hiroki Nakagawa, blink-revie...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, horo+...@chromium.org, jmedle...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko+ser...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, video-networking...@google.com
    Attention needed from Philip Eliasson

    Tony Herre added 2 comments

    Patchset-level comments
    File-level comment, Patchset 6 (Latest):
    Tony Herre . resolved

    Thanks!

    File third_party/blink/renderer/modules/peerconnection/rtc_rtp_transport.idl
    Line 7, Patchset 6 (Latest): // TODO(crbug.com/345101934): This needs to actually be on some
    Guido Urdaneta . resolved

    super optional nit: keep the new TODO format? (BTW, I like the old one better and AFAIK it's still acceptable).

    Tony Herre

    Heh, yeah, I've had this back and forward with a few different people recently. Discovered https://chromium.googlesource.com/chromium/src/+/HEAD/styleguide/todos.md explicitly saying `TODO(crbug.com/40192027):` is preferred in chromium (and I prefer it too).

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Philip Eliasson
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    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: Icd63a13a7953a0b77fc589f88e739aa869847fc8
    Gerrit-Change-Number: 5646499
    Gerrit-PatchSet: 6
    Gerrit-Owner: Tony Herre <top...@chromium.org>
    Gerrit-Reviewer: Guido Urdaneta <gui...@chromium.org>
    Gerrit-Reviewer: Philip Eliasson <phil...@chromium.org>
    Gerrit-Reviewer: Tony Herre <top...@chromium.org>
    Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>
    Gerrit-CC: Kentaro Hara <har...@chromium.org>
    Gerrit-Attention: Philip Eliasson <phil...@chromium.org>
    Gerrit-Comment-Date: Fri, 28 Jun 2024 07:33:00 +0000
    satisfied_requirement
    open
    diffy

    Tony Herre (Gerrit)

    unread,
    Jun 28, 2024, 4:07:32 AM (yesterday) Jun 28
    to Guido Urdaneta, Philip Eliasson, Chromium LUCI CQ, chromium...@chromium.org, Kentaro Hara, Hiroki Nakagawa, blink-revie...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, horo+...@chromium.org, jmedle...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko+ser...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, video-networking...@google.com
    Attention needed from Philip Eliasson

    Tony Herre voted and added 1 comment

    Votes added by Tony Herre

    Commit-Queue+2

    1 comment

    Patchset-level comments
    Tony Herre . resolved

    I'll go ahead with landing this to unblock the later prototyping, but feel free to add comments Philip and I can fix them in followup prototype impl cls.

    Thanks

    Open in Gerrit

    Related details

    Attention is currently required from:
    • Philip Eliasson
    Submit Requirements:
    • requirement satisfiedCode-Coverage
    • requirement satisfiedCode-Review
    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: Icd63a13a7953a0b77fc589f88e739aa869847fc8
    Gerrit-Change-Number: 5646499
    Gerrit-PatchSet: 6
    Gerrit-Owner: Tony Herre <top...@chromium.org>
    Gerrit-Reviewer: Guido Urdaneta <gui...@chromium.org>
    Gerrit-Reviewer: Philip Eliasson <phil...@chromium.org>
    Gerrit-Reviewer: Tony Herre <top...@chromium.org>
    Gerrit-CC: Hiroki Nakagawa <nhi...@chromium.org>
    Gerrit-CC: Kentaro Hara <har...@chromium.org>
    Gerrit-Attention: Philip Eliasson <phil...@chromium.org>
    Gerrit-Comment-Date: Fri, 28 Jun 2024 08:07:20 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: Yes
    satisfied_requirement
    open
    diffy

    Chromium LUCI CQ (Gerrit)

    unread,
    Jun 28, 2024, 5:06:56 AM (yesterday) Jun 28
    to Tony Herre, Guido Urdaneta, Philip Eliasson, chromium...@chromium.org, Kentaro Hara, Hiroki Nakagawa, blink-revie...@chromium.org, blink-...@chromium.org, chromium-a...@chromium.org, extension...@chromium.org, horo+...@chromium.org, jmedle...@chromium.org, jsbell+ser...@chromium.org, kenjibah...@chromium.org, kinuko+ser...@chromium.org, servicewor...@chromium.org, shimazu+se...@chromium.org, video-networking...@google.com

    Chromium LUCI CQ submitted the change

    Change information

    Commit message:
    Prototype implementation of RtpTransport::readSentRtp() batch read api

    See https://github.com/w3c/webrtc-rtptransport/pull/42 and https://github.com/w3c/webrtc-rtptransport/blob/main/explainer-use-case-2.md.

    Would be used to allow BWE JS implementations to listen to the actual
    sent times of RTP packets, for comparison with readReceivedRtpAcks().

    All guarded by the blink feature RTCRtpTransport.
    Bug: 345101934
    Change-Id: Icd63a13a7953a0b77fc589f88e739aa869847fc8
    Reviewed-by: Guido Urdaneta <gui...@chromium.org>
    Commit-Queue: Tony Herre <top...@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#1320843}
    Files:
    • M third_party/blink/renderer/modules/peerconnection/adapters/web_rtc_cross_thread_copier.h
    • M third_party/blink/renderer/modules/peerconnection/intercepting_network_controller.h
    • M third_party/blink/renderer/modules/peerconnection/rtc_rtp_send_result.cc
    • M third_party/blink/renderer/modules/peerconnection/rtc_rtp_sent.cc
    • M third_party/blink/renderer/modules/peerconnection/rtc_rtp_sent.h
    • M third_party/blink/renderer/modules/peerconnection/rtc_rtp_transport.cc
    • M third_party/blink/renderer/modules/peerconnection/rtc_rtp_transport.h
    • M third_party/blink/renderer/modules/peerconnection/rtc_rtp_transport.idl
    • M third_party/blink/web_tests/webexposed/global-interface-listing-expected.txt
    Change size: M
    Delta: 9 files changed, 79 insertions(+), 9 deletions(-)
    Branch: refs/heads/main
    Submit Requirements:
    • requirement satisfiedCode-Review: +1 by Guido Urdaneta
    Open in Gerrit
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: merged
    Gerrit-Project: chromium/src
    Gerrit-Branch: main
    Gerrit-Change-Id: Icd63a13a7953a0b77fc589f88e739aa869847fc8
    Gerrit-Change-Number: 5646499
    Gerrit-PatchSet: 7
    Gerrit-Owner: Tony Herre <top...@chromium.org>
    Gerrit-Reviewer: Chromium LUCI CQ <chromiu...@luci-project-accounts.iam.gserviceaccount.com>
    Gerrit-Reviewer: Guido Urdaneta <gui...@chromium.org>
    Gerrit-Reviewer: Philip Eliasson <phil...@chromium.org>
    Gerrit-Reviewer: Tony Herre <top...@chromium.org>
    open
    diffy
    satisfied_requirement
    Reply all
    Reply to author
    Forward
    0 new messages