Jianlin Qiu (Gerrit)

unread,

Apr 17, 2024, 9:15:49 AMApr 17

to Sergey Silkin, Sergey Silkin, Dale Curtis, Harald Alvestrand, Ilya Nikolaevskiy, Philip Eliasson, Erik Språng, Hirokazu Honda, Tricium, chromium...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, poscia...@chromium.org, video-networking...@google.com

Attention needed from Dale Curtis, Ilya Nikolaevskiy, Philip Eliasson, Sergey Silkin and Sergey Silkin

Jianlin Qiu added 6 comments

File media/webrtc/webrtc_features.h

Line 47, Patchset 10:BASE_DECLARE_FEATURE(kExperimentalWebRtcH265);

Sergey Silkin . resolved

Let's make naming consistent with other WebRTC related features. kWebRtcAllowH265 ?

Jianlin Qiu

How do you think about we have 2 flags for this：
kWebRtcAllowH265Send
kWebRtcAllowH265Receive
I would assume we probably does not have the intention to allow encode by default, but the capability of receiving H.265 streams improves interop with native applications in the future?

Sergey Silkin

Yes, I think it makes sense to split the flag into send/recv.

Jianlin Qiu

Done

File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_adapter.cc

Line 520, Patchset 10: if (!resolution_monitor && config.codec() != media::VideoCodec::kHEVC) {

Sergey Silkin . resolved

Should HasSoftwareFallback() be used here (and in other similar places) instead?

Jianlin Qiu

Done

File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_factory.cc

Line 51, Patchset 10:// To support H.265 level 3.1, the default size is set to 1920x1080.

Sergey Silkin . resolved

Sounds slightly misleading. Unlike in the case of H264 there is no SW decoder we need to align max supported level with. I guess we simply want to signal support of 1080p and derive necessary min level for this resolution.

Jianlin Qiu

Done

File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_factory_test.cc

Line 241, Patchset 10: true /*reference_scaling*/),

Sergey Silkin . resolved

should this be false?

Jianlin Qiu

Done

File third_party/blink/renderer/platform/peerconnection/rtc_video_encoder.cc

Line 1938, Patchset 10:

  if ((profile_ < media::HEVCPROFILE_MIN ||
       profile_ > media::HEVCPROFILE_MAX) &&

Sergey Silkin . unresolved

Any reason not to use codec_settings->codecType here?

Jianlin Qiu

No specific reason. I'm just following the code at line 1953 & 1954 for consistency.

File third_party/blink/renderer/platform/peerconnection/rtc_video_encoder_factory.cc

Line 35, Patchset 10:

#if BUILDFLAG(RTC_USE_H265)

#endif  // BUILDFLAG(RTC_USE_H265)

Sergey Silkin . resolved

remove?

Jianlin Qiu

Done

Open in Gerrit

Related details

Attention is currently required from:

Dale Curtis
Ilya Nikolaevskiy
Philip Eliasson
Sergey Silkin
Sergey Silkin

Submit Requirements:

Code-Coverage
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

Dale Curtis (Gerrit)

unread,

Apr 17, 2024, 1:24:32 PMApr 17

to Jianlin Qiu, Sergey Silkin, Sergey Silkin, Harald Alvestrand, Ilya Nikolaevskiy, Philip Eliasson, Erik Språng, Hirokazu Honda, Tricium, chromium...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, poscia...@chromium.org, video-networking...@google.com

Attention needed from Ilya Nikolaevskiy, Jianlin Qiu, Philip Eliasson, Sergey Silkin and Sergey Silkin

Dale Curtis added 6 comments

Patchset-level comments

File-level comment, Patchset 12 (Latest):

Dale Curtis . resolved

Defer to RTC owners for approval.

File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_adapter.cc

Line 107, Patchset 12 (Latest):bool HasSoftwareFallback(media::VideoCodec video_codec) {

Dale Curtis . unresolved

Maybe clearer just as:

```
if (video_codec == HEVC) {
  return false;
}
```

outside of the buildflags.

File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_factory.cc

Line 29, Patchset 12 (Latest):#if BUILDFLAG(RTC_USE_H265)

Dale Curtis . unresolved

Move build flag includes below others generally.

Line 48, Patchset 12 (Latest):const int kDefaultFps = 30;

Dale Curtis . unresolved

constexpr all these.

File third_party/blink/renderer/platform/peerconnection/rtc_video_encoder_factory.cc

Line 22, Patchset 12 (Latest):#if BUILDFLAG(RTC_USE_H265)

Dale Curtis . unresolved

Move below other includes.

Line 235, Patchset 12 (Latest): SupportedFormats supported_h265_formats;

Dale Curtis . unresolved

You could choose something more generic like `low_priority_formats` so more of the code doesn't need to be behind a buildflag.

Open in Gerrit

Related details

Attention is currently required from:

Ilya Nikolaevskiy
Jianlin Qiu

satisfied_requirement

unsatisfied_requirement

open

diffy

Jianlin Qiu (Gerrit)

unread,

Apr 17, 2024, 9:57:14 PMApr 17

to Sergey Silkin, Sergey Silkin, Dale Curtis, Harald Alvestrand, Ilya Nikolaevskiy, Philip Eliasson, Erik Språng, Hirokazu Honda, Tricium, chromium...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, poscia...@chromium.org, video-networking...@google.com

Attention needed from Dale Curtis, Ilya Nikolaevskiy, Philip Eliasson, Sergey Silkin and Sergey Silkin

Jianlin Qiu added 5 comments

File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_adapter.cc

Line 107, Patchset 12:bool HasSoftwareFallback(media::VideoCodec video_codec) {

Dale Curtis . resolved

Maybe clearer just as:
```
if (video_codec == HEVC) {
  return false;
}
```
outside of the buildflags.

Jianlin Qiu

Done

File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_factory.cc

Line 29, Patchset 12:#if BUILDFLAG(RTC_USE_H265)

Dale Curtis . resolved

Move build flag includes below others generally.

Jianlin Qiu

Done

Line 48, Patchset 12:const int kDefaultFps = 30;

Dale Curtis . resolved

constexpr all these.

Jianlin Qiu

Done

File third_party/blink/renderer/platform/peerconnection/rtc_video_encoder_factory.cc

Line 22, Patchset 12:#if BUILDFLAG(RTC_USE_H265)

Dale Curtis . resolved

Move below other includes.

Jianlin Qiu

Done

Line 235, Patchset 12: SupportedFormats supported_h265_formats;

Dale Curtis . resolved

You could choose something more generic like `low_priority_formats` so more of the code doesn't need to be behind a buildflag.

Jianlin Qiu

Done

Open in Gerrit

Related details

Attention is currently required from:

Dale Curtis
Ilya Nikolaevskiy

Philip Eliasson
Sergey Silkin
Sergey Silkin

Submit Requirements:

Code-Coverage
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

Sergey Silkin (Gerrit)

unread,

Apr 19, 2024, 6:46:32 AMApr 19

to Jianlin Qiu, Sergey Silkin, Dale Curtis, Harald Alvestrand, Ilya Nikolaevskiy, Philip Eliasson, Erik Språng, Hirokazu Honda, Tricium, chromium...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, poscia...@chromium.org, video-networking...@google.com

Attention needed from Ilya Nikolaevskiy, Jianlin Qiu, Philip Eliasson and Sergey Silkin

Sergey Silkin added 2 comments

File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_factory_test.cc

Line 188, Patchset 14 (Latest):#if BUILDFLAG(RTC_USE_H265)

Sergey Silkin . unresolved

Seems that this #if is not really needed.

File third_party/blink/renderer/platform/peerconnection/rtc_video_encoder.cc

Line 1938, Patchset 10:

  if ((profile_ < media::HEVCPROFILE_MIN ||
       profile_ > media::HEVCPROFILE_MAX) &&

Sergey Silkin . unresolved

Any reason not to use codec_settings->codecType here?

Jianlin Qiu

No specific reason. I'm just following the code at line 1953 & 1954 for consistency.

Sergey Silkin

Profile-based conditions were added in https://chromium-review.googlesource.com/c/chromium/src/+/3351622. The tests added in the same CL explicitly set codecType. For example: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/peerconnection/rtc_video_encoder_test.cc;l=802-805

Not sure why profile was used. codecType seems to be a better option in both places.

Open in Gerrit

Related details

Attention is currently required from:

Ilya Nikolaevskiy
Jianlin Qiu
Philip Eliasson

Sergey Silkin

Submit Requirements:

Code-Coverage
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

Jianlin Qiu (Gerrit)

unread,

Apr 19, 2024, 10:01:23 AMApr 19

to Sergey Silkin, Sergey Silkin, Dale Curtis, Harald Alvestrand, Ilya Nikolaevskiy, Philip Eliasson, Erik Språng, Hirokazu Honda, Tricium, chromium...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, poscia...@chromium.org, video-networking...@google.com

Attention needed from Ilya Nikolaevskiy, Philip Eliasson, Sergey Silkin and Sergey Silkin

Jianlin Qiu added 2 comments

File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_factory_test.cc

Line 188, Patchset 14:#if BUILDFLAG(RTC_USE_H265)

Sergey Silkin . resolved

Seems that this #if is not really needed.

Jianlin Qiu

Done

File third_party/blink/renderer/platform/peerconnection/rtc_video_encoder.cc

Line 1938, Patchset 10:

  if ((profile_ < media::HEVCPROFILE_MIN ||
       profile_ > media::HEVCPROFILE_MAX) &&

Sergey Silkin . resolved

Any reason not to use codec_settings->codecType here?

Jianlin Qiu

No specific reason. I'm just following the code at line 1953 & 1954 for consistency.

Sergey Silkin

Profile-based conditions were added in https://chromium-review.googlesource.com/c/chromium/src/+/3351622. The tests added in the same CL explicitly set codecType. For example: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/peerconnection/rtc_video_encoder_test.cc;l=802-805
Not sure why profile was used. codecType seems to be a better option in both places.

Jianlin Qiu

Done

Open in Gerrit

Related details

Attention is currently required from:

Ilya Nikolaevskiy
Philip Eliasson
Sergey Silkin
Sergey Silkin

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Sergey Silkin (Gerrit)

unread,

Apr 19, 2024, 10:17:11 AMApr 19

to Jianlin Qiu, Chromium LUCI CQ, Sergey Silkin, Dale Curtis, Harald Alvestrand, Ilya Nikolaevskiy, Philip Eliasson, Erik Språng, Hirokazu Honda, Tricium, chromium...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, poscia...@chromium.org, video-networking...@google.com

Attention needed from Ilya Nikolaevskiy, Jianlin Qiu, Philip Eliasson and Sergey Silkin

Sergey Silkin voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Ilya Nikolaevskiy
Jianlin Qiu
Philip Eliasson
Sergey Silkin

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

open

diffy

Jianlin Qiu (Gerrit)

unread,

Apr 19, 2024, 12:47:59 PMApr 19

to Daniel Cheng, Sergey Silkin, Chromium LUCI CQ, Sergey Silkin, Dale Curtis, Harald Alvestrand, Ilya Nikolaevskiy, Philip Eliasson, Erik Språng, Hirokazu Honda, Tricium, chromium...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, poscia...@chromium.org, video-networking...@google.com

Attention needed from Dale Curtis, Daniel Cheng, Harald Alvestrand, Ilya Nikolaevskiy, Jianlin Qiu, Philip Eliasson, Sergey Silkin and Sergey Silkin

Message from Jianlin Qiu

Set Ready For Review

Open in Gerrit

Related details

Attention is currently required from:

Dale Curtis
Daniel Cheng
Harald Alvestrand

Ilya Nikolaevskiy
Jianlin Qiu
Philip Eliasson
Sergey Silkin
Sergey Silkin

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

unsatisfied_requirement

open

diffy

Dale Curtis (Gerrit)

unread,

Apr 19, 2024, 4:05:19 PMApr 19

to Jianlin Qiu, Mirko Bonadei, Daniel Cheng, Sergey Silkin, Chromium LUCI CQ, Sergey Silkin, Harald Alvestrand, Ilya Nikolaevskiy, Philip Eliasson, Erik Språng, Hirokazu Honda, Tricium, chromium...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, poscia...@chromium.org, video-networking...@google.com

Attention needed from Daniel Cheng, Harald Alvestrand, Ilya Nikolaevskiy, Jianlin Qiu, Mirko Bonadei, Philip Eliasson, Sergey Silkin and Sergey Silkin

Dale Curtis added 1 comment

Patchset-level comments

File-level comment, Patchset 19 (Latest):

Dale Curtis . unresolved

Jianlin, can you pare down to just the required reviewers?

Open in Gerrit

Related details

Attention is currently required from:

Daniel Cheng
Harald Alvestrand
Ilya Nikolaevskiy
Jianlin Qiu

Mirko Bonadei

Philip Eliasson
Sergey Silkin
Sergey Silkin

Submit Requirements:

Code-Coverage
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

Daniel Cheng (Gerrit)

unread,

Apr 19, 2024, 4:12:57 PMApr 19

to Jianlin Qiu, Mirko Bonadei, Daniel Cheng, Sergey Silkin, Chromium LUCI CQ, Sergey Silkin, Dale Curtis, Harald Alvestrand, Ilya Nikolaevskiy, Philip Eliasson, Erik Språng, Hirokazu Honda, Tricium, chromium...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, poscia...@chromium.org, video-networking...@google.com

Attention needed from Harald Alvestrand, Ilya Nikolaevskiy, Jianlin Qiu, Mirko Bonadei, Philip Eliasson, Sergey Silkin and Sergey Silkin

Daniel Cheng added 2 comments

File third_party/blink/renderer/platform/peerconnection/DEPS

Line 59, Patchset 19 (Latest): "rtc_video_encoder_factory.cc": [

Daniel Cheng . unresolved

Probably easier to just add this in include rules rather than specific include rules, unless there's a specific reason to scope this?

File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_factory.cc

Line 62, Patchset 19 (Latest):constexpr std::array<CodecConfig, 11> kCodecConfigs = {{

Daniel Cheng . unresolved

This std::array needs to be sized according to how many elements are in it.

Unfortunately, CTAD requires specifying all or nothing.

We can:

use a regular C array, i.e. constexpr CodecConfig kCodecConfigs[] = {{ ... }};
or we can try to dynamically set the size here.

I would probably just use a regular C array.

Open in Gerrit

Related details

Attention is currently required from:

satisfied_requirement

unsatisfied_requirement

open

diffy

Daniel Cheng (Gerrit)

unread,

Apr 19, 2024, 4:19:07 PMApr 19

to Jianlin Qiu, Mirko Bonadei, Daniel Cheng, Sergey Silkin, Chromium LUCI CQ, Sergey Silkin, Dale Curtis, Harald Alvestrand, Ilya Nikolaevskiy, Philip Eliasson, Erik Språng, Hirokazu Honda, Tricium, chromium...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, poscia...@chromium.org, video-networking...@google.com

Attention needed from Harald Alvestrand, Ilya Nikolaevskiy, Jianlin Qiu, Mirko Bonadei, Philip Eliasson, Sergey Silkin and Sergey Silkin

Daniel Cheng added 1 comment

File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_factory.cc

Line 62, Patchset 19 (Latest):constexpr std::array<CodecConfig, 11> kCodecConfigs = {{

Daniel Cheng . unresolved

This std::array needs to be sized according to how many elements are in it.
Unfortunately, CTAD requires specifying all or nothing.
We can:
use a regular C array, i.e. constexpr CodecConfig kCodecConfigs[] = {{ ... }};
or we can try to dynamically set the size here.
I would probably just use a regular C array.

Daniel Cheng

Alternatively a CTAD variant:

```
constexpr std::array kCodecConfigs = {
  CodecConfig{...},
  CodecConfig{...},
  ...
};
```

satisfied_requirement

unsatisfied_requirement

open

diffy

Jianlin Qiu (Gerrit)

unread,

Apr 19, 2024, 7:31:35 PMApr 19

to Ilya Nikolaevskiy, Harald Alvestrand, Philip Eliasson, Mirko Bonadei, Daniel Cheng, Sergey Silkin, Chromium LUCI CQ, Sergey Silkin, Dale Curtis, Erik Språng, Hirokazu Honda, Tricium, chromium...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, poscia...@chromium.org, video-networking...@google.com

Attention needed from Daniel Cheng, Mirko Bonadei, Sergey Silkin and Sergey Silkin

Jianlin Qiu added 2 comments

File third_party/blink/renderer/platform/peerconnection/DEPS

Line 59, Patchset 19: "rtc_video_encoder_factory.cc": [

Daniel Cheng . resolved

Probably easier to just add this in include rules rather than specific include rules, unless there's a specific reason to scope this?

Jianlin Qiu

Done

File third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_factory.cc

Line 62, Patchset 19:constexpr std::array<CodecConfig, 11> kCodecConfigs = {{

Daniel Cheng . resolved

This std::array needs to be sized according to how many elements are in it.
Unfortunately, CTAD requires specifying all or nothing.
We can:
use a regular C array, i.e. constexpr CodecConfig kCodecConfigs[] = {{ ... }};
or we can try to dynamically set the size here.
I would probably just use a regular C array.

Daniel Cheng

Alternatively a CTAD variant:

```
constexpr std::array kCodecConfigs = {
  CodecConfig{...},
  CodecConfig{...},
  ...
};
```

Jianlin Qiu

Done

Open in Gerrit

Related details

Attention is currently required from:

Daniel Cheng
Mirko Bonadei
Sergey Silkin
Sergey Silkin

Submit Requirements:

Code-Coverage
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

Sergey Silkin (Gerrit)

unread,

Apr 22, 2024, 3:19:18 AMApr 22

to Jianlin Qiu, Sergey Silkin, Ilya Nikolaevskiy, Harald Alvestrand, Philip Eliasson, Mirko Bonadei, Daniel Cheng, Chromium LUCI CQ, Dale Curtis, Erik Språng, Hirokazu Honda, Tricium, chromium...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, poscia...@chromium.org, video-networking...@google.com

Attention needed from Daniel Cheng, Mirko Bonadei and Sergey Silkin

Sergey Silkin removed Sergey Silkin from this change

Deleted Reviewers:

Sergey Silkin

Open in Gerrit

Related details

Attention is currently required from:

Daniel Cheng
Mirko Bonadei
Sergey Silkin

Submit Requirements:

Code-Coverage
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

Sergey Silkin (Gerrit)

unread,

Apr 22, 2024, 3:19:38 AMApr 22

to Jianlin Qiu, Ilya Nikolaevskiy, Harald Alvestrand, Philip Eliasson, Mirko Bonadei, Daniel Cheng, Chromium LUCI CQ, Dale Curtis, Erik Språng, Hirokazu Honda, Tricium, chromium...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, poscia...@chromium.org, video-networking...@google.com

Attention needed from Daniel Cheng, Jianlin Qiu and Mirko Bonadei

Sergey Silkin voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Daniel Cheng
Jianlin Qiu
Mirko Bonadei

Submit Requirements:

Code-Coverage
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

Daniel Cheng (Gerrit)

unread,

Apr 22, 2024, 1:56:45 PMApr 22

to Jianlin Qiu, Daniel Cheng, Sergey Silkin, Ilya Nikolaevskiy, Harald Alvestrand, Philip Eliasson, Mirko Bonadei, Chromium LUCI CQ, Dale Curtis, Erik Språng, Hirokazu Honda, Tricium, chromium...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, poscia...@chromium.org, video-networking...@google.com

Attention needed from Jianlin Qiu and Mirko Bonadei

Daniel Cheng voted and added 1 comment

Votes added by Daniel Cheng

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 20 (Latest):

Daniel Cheng . resolved

LGTM

Open in Gerrit

Related details

Attention is currently required from:

Jianlin Qiu
Mirko Bonadei

satisfied_requirement

unsatisfied_requirement

open

diffy

Jianlin Qiu (Gerrit)

unread,

Apr 23, 2024, 3:50:23 AMApr 23

to Daniel Cheng, Sergey Silkin, Ilya Nikolaevskiy, Harald Alvestrand, Philip Eliasson, Mirko Bonadei, Chromium LUCI CQ, Dale Curtis, Erik Språng, Hirokazu Honda, Tricium, chromium...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, poscia...@chromium.org, video-networking...@google.com

Attention needed from Dale Curtis and Mirko Bonadei

Jianlin Qiu added 1 comment

Patchset-level comments

File-level comment, Patchset 20 (Latest):

Jianlin Qiu . resolved

Hi, dalecurtis@ could you please help to review */webrtc_features.*,
mbonadei@ could you please help to review webrtc_overries/BUILD.gn? Thanks!

Open in Gerrit

Related details

Attention is currently required from:

Dale Curtis
Mirko Bonadei

satisfied_requirement

unsatisfied_requirement

open

diffy

Mirko Bonadei (Gerrit)

unread,

Apr 24, 2024, 11:13:22 AMApr 24

to Jianlin Qiu, Daniel Cheng, Sergey Silkin, Ilya Nikolaevskiy, Harald Alvestrand, Philip Eliasson, Chromium LUCI CQ, Dale Curtis, Erik Språng, Hirokazu Honda, Tricium, chromium...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, poscia...@chromium.org, video-networking...@google.com

Attention needed from Dale Curtis and Jianlin Qiu

Mirko Bonadei voted and added 1 comment

Votes added by Mirko Bonadei

Code-Review

+1

1 comment

Patchset-level comments

File-level comment, Patchset 20 (Latest):

Jianlin Qiu . resolved

Hi, dalecurtis@ could you please help to review */webrtc_features.*,
mbonadei@ could you please help to review webrtc_overries/BUILD.gn? Thanks!

Mirko Bonadei

mbonadei@ could you please help to review webrtc_overries/BUILD.gn? Thanks!

LGTM for webrtc_overries.

Open in Gerrit

Related details

Attention is currently required from:

Dale Curtis
Jianlin Qiu

satisfied_requirement

unsatisfied_requirement

open

diffy

Dale Curtis (Gerrit)

unread,

Apr 24, 2024, 3:48:59 PMApr 24

to Jianlin Qiu, Mirko Bonadei, Daniel Cheng, Sergey Silkin, Ilya Nikolaevskiy, Harald Alvestrand, Philip Eliasson, Chromium LUCI CQ, Erik Språng, Hirokazu Honda, Tricium, chromium...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, poscia...@chromium.org, video-networking...@google.com

Attention needed from Jianlin Qiu

Dale Curtis voted Code-Review+1

Code-Review

+1

Open in Gerrit

Related details

Attention is currently required from:

Jianlin Qiu

satisfied_requirement

unsatisfied_requirement

open

diffy

Dale Curtis (Gerrit)

unread,

Apr 24, 2024, 3:49:04 PMApr 24

to Jianlin Qiu, Mirko Bonadei, Daniel Cheng, Sergey Silkin, Ilya Nikolaevskiy, Harald Alvestrand, Philip Eliasson, Chromium LUCI CQ, Erik Språng, Hirokazu Honda, Tricium, chromium...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, poscia...@chromium.org, video-networking...@google.com

Attention needed from Jianlin Qiu

Dale Curtis added 1 comment

Patchset-level comments

File-level comment, Patchset 19:

Dale Curtis . resolved

Jianlin, can you pare down to just the required reviewers?

Dale Curtis

Acknowledged

Open in Gerrit

Related details

Attention is currently required from:

Jianlin Qiu

Submit Requirements:

Code-Coverage
Code-Review

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

Gerrit

satisfied_requirement

open

diffy

Jianlin Qiu (Gerrit)

unread,

Apr 24, 2024, 5:18:16 PMApr 24

to Dale Curtis, Mirko Bonadei, Daniel Cheng, Sergey Silkin, Ilya Nikolaevskiy, Harald Alvestrand, Philip Eliasson, Chromium LUCI CQ, Erik Språng, Hirokazu Honda, Tricium, chromium...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, poscia...@chromium.org, video-networking...@google.com

Jianlin Qiu voted and added 1 comment

Votes added by Jianlin Qiu

Commit-Queue

+2

1 comment

Patchset-level comments

File-level comment, Patchset 20 (Latest):

Jianlin Qiu . resolved

Thanks all for the review! Submitting.

Open in Gerrit

Related details

Attention set is empty

satisfied_requirement

open

diffy

Chromium LUCI CQ (Gerrit)

unread,

Apr 24, 2024, 6:22:52 PMApr 24

to Jianlin Qiu, Dale Curtis, Mirko Bonadei, Daniel Cheng, Sergey Silkin, Ilya Nikolaevskiy, Harald Alvestrand, Philip Eliasson, Erik Språng, Hirokazu Honda, Tricium, chromium...@chromium.org, blink-...@chromium.org, feature-me...@chromium.org, kinuko...@chromium.org, poscia...@chromium.org, video-networking...@google.com

Chromium LUCI CQ submitted the change

Change information

Commit message:

RTC encoder/decoder factory support for H.265 behind flag.

This allows H.265 to be negotiated when build with RTC H.265 support, and at the same time necessary runtime flags are turned on for H.265.

A few flags needs to be provided at runtime for H.265 streaming:
First, to allow H.265 to be included in sender's capabilities,
--enable-features=PlatformHEVCEncoderSupport should be specified so that
encoder GPU factory will enumerate HW H.265 encoders;
--enable-features=WebRtcAllowH265Send should be specified so that RTC
encoder factory allows H.265 to be included in supported formats when GPU encoder factory reports supporting H.265.
--enable-features=WebRtcAllowH265Receive should be specified so that RTC
decoder factory allows H.265 to be included in supported formats when GPU decoder factory reports supporting H.265.
--force-fieldtrials=WebRTC-Video-H26xPacketBuffer/Enabled needs to be specified to switch frame assembly implementation to H.26x specific instead of using generic packet buffer.

Bug: webrtc:13485

Change-Id: Ifb2a3e3b7f09fa1c2307629e08d855a8ece53531

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

Reviewed-by: Mirko Bonadei <mbon...@chromium.org>

Commit-Queue: Jianlin Qiu <jianl...@intel.com>

Reviewed-by: Sergey Silkin <ssi...@chromium.org>

Reviewed-by: Daniel Cheng <dch...@chromium.org>

Reviewed-by: Dale Curtis <dalec...@chromium.org>

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

Files:

M media/webrtc/webrtc_features.cc
M media/webrtc/webrtc_features.h
M third_party/blink/renderer/platform/BUILD.gn
M third_party/blink/renderer/platform/peerconnection/DEPS
M third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_adapter.cc
M third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_adapter_test.cc
M third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_factory.cc
M third_party/blink/renderer/platform/peerconnection/rtc_video_decoder_factory_test.cc
M third_party/blink/renderer/platform/peerconnection/rtc_video_encoder.cc
M third_party/blink/renderer/platform/peerconnection/rtc_video_encoder_factory.cc
M third_party/blink/renderer/platform/peerconnection/rtc_video_encoder_factory_test.cc
M third_party/blink/renderer/platform/webrtc/webrtc_video_utils.cc
M third_party/webrtc_overrides/BUILD.gn

Change size: L

Delta: 13 files changed, 416 insertions(+), 29 deletions(-)

Branch: refs/heads/main

Submit Requirements:

Code-Review: +1 by Dale Curtis, +1 by Daniel Cheng, +1 by Sergey Silkin, +1 by Mirko Bonadei

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

RTC encoder/decoder factory support for H.265 behind flag. [chromium/src : main]

Jianlin Qiu (Gerrit)

Jianlin Qiu added 6 comments

Related details

Dale Curtis (Gerrit)

Dale Curtis added 6 comments

Related details

Jianlin Qiu (Gerrit)

Jianlin Qiu added 5 comments

Related details

Sergey Silkin (Gerrit)

Sergey Silkin added 2 comments

Related details

Jianlin Qiu (Gerrit)

Jianlin Qiu added 2 comments

Related details

Sergey Silkin (Gerrit)

Sergey Silkin voted Code-Review+1

Related details

Jianlin Qiu (Gerrit)

Message from Jianlin Qiu

Related details

Dale Curtis (Gerrit)

Dale Curtis added 1 comment

Related details

Daniel Cheng (Gerrit)

Daniel Cheng added 2 comments

Related details

Daniel Cheng (Gerrit)

Daniel Cheng added 1 comment

Jianlin Qiu (Gerrit)

Jianlin Qiu added 2 comments

Related details

Sergey Silkin (Gerrit)

Sergey Silkin removed Sergey Silkin from this change

Related details

Sergey Silkin (Gerrit)

Sergey Silkin voted Code-Review+1

Related details

Daniel Cheng (Gerrit)

Daniel Cheng voted and added 1 comment

Votes added by Daniel Cheng

1 comment

Related details

Jianlin Qiu (Gerrit)

Jianlin Qiu added 1 comment

Related details

Mirko Bonadei (Gerrit)

Mirko Bonadei voted and added 1 comment

Votes added by Mirko Bonadei

1 comment

Related details

Dale Curtis (Gerrit)

Dale Curtis voted Code-Review+1

Related details

Dale Curtis (Gerrit)

Dale Curtis added 1 comment

Related details

Jianlin Qiu (Gerrit)

Jianlin Qiu voted and added 1 comment

Votes added by Jianlin Qiu

1 comment

Related details

Chromium LUCI CQ (Gerrit)

Chromium LUCI CQ submitted the change

Change information