Remove rule that discourages passing optional by const reference
include example to demonstrate:
(subjectively) increased readability
(objectively) decreased binary size
Bug: None
Change-Id: I970e668af98d98725b2d527f44169a8b7c9d2338
Reviewed-on: https://webrtc-review.googlesource.com/c/121420
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Reviewed-by: Johannes Kron <kron@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26545}diff --git a/api/video/encoded_image.h b/api/video/encoded_image.h
index 717489f..fb3288f 100644
--- a/api/video/encoded_image.h
+++ b/api/video/encoded_image.h
@@ -62,9 +62,8 @@
const webrtc::ColorSpace* ColorSpace() const {
return color_space_ ? &*color_space_ : nullptr;
}
- void SetColorSpace(const webrtc::ColorSpace* color_space) {
- color_space_ =
- color_space ? absl::make_optional(*color_space) : absl::nullopt;
+ void SetColorSpace(const absl::optional<webrtc::ColorSpace>& color_space) {
+ color_space_ = color_space;
}
size_t size() const { return size_; }
diff --git a/api/video/video_frame.cc b/api/video/video_frame.cc
index c91aeda..4075bf7 100644
--- a/api/video/video_frame.cc
+++ b/api/video/video_frame.cc
@@ -61,7 +61,7 @@
}
VideoFrame::Builder& VideoFrame::Builder::set_color_space(
- const ColorSpace& color_space) {
+ const absl::optional<ColorSpace>& color_space) {
color_space_ = color_space;
return *this;
}
diff --git a/api/video/video_frame.h b/api/video/video_frame.h
index f83345a..aa377c2 100644
--- a/api/video/video_frame.h
+++ b/api/video/video_frame.h
@@ -47,7 +47,7 @@
Builder& set_timestamp_rtp(uint32_t timestamp_rtp);
Builder& set_ntp_time_ms(int64_t ntp_time_ms);
Builder& set_rotation(VideoRotation rotation);
- Builder& set_color_space(const ColorSpace& color_space);
+ Builder& set_color_space(const absl::optional<ColorSpace>& color_space);
Builder& set_color_space(const ColorSpace* color_space);
Builder& set_id(uint16_t id);
Builder& set_partial_frame_description(
@@ -140,12 +140,9 @@
void set_rotation(VideoRotation rotation) { rotation_ = rotation; }
// Get color space when available.
- const ColorSpace* color_space() const {
- return color_space_ ? &*color_space_ : nullptr;
- }
- void set_color_space(ColorSpace* color_space) {
- color_space_ =
- color_space ? absl::make_optional(*color_space) : absl::nullopt;
+ const absl::optional<ColorSpace>& color_space() const { return color_space_; }
+ void set_color_space(const absl::optional<ColorSpace>& color_space) {
+ color_space_ = color_space;
}
const PartialFrameDescription* partial_frame_description() const {
diff --git a/call/rtp_payload_params_unittest.cc b/call/rtp_payload_params_unittest.cc
index 5343cf9..c46d975 100644
--- a/call/rtp_payload_params_unittest.cc
+++ b/call/rtp_payload_params_unittest.cc
@@ -126,7 +126,7 @@
ColorSpace color_space(
ColorSpace::PrimaryID::kSMPTE170M, ColorSpace::TransferID::kSMPTE170M,
ColorSpace::MatrixID::kSMPTE170M, ColorSpace::RangeID::kFull);
- encoded_image.SetColorSpace(&color_space);
+ encoded_image.SetColorSpace(color_space);
header = params.GetRtpVideoHeader(encoded_image, &codec_info, kDontCare);
EXPECT_EQ(kVideoRotation_90, header.rotation);
diff --git a/modules/video_coding/codecs/h264/test/h264_impl_unittest.cc b/modules/video_coding/codecs/h264/test/h264_impl_unittest.cc
index 8fa2d49..f93c463 100644
--- a/modules/video_coding/codecs/h264/test/h264_impl_unittest.cc
+++ b/modules/video_coding/codecs/h264/test/h264_impl_unittest.cc
@@ -123,7 +123,7 @@
VideoFrame input_frame_w_color_space =
VideoFrame::Builder()
.set_video_frame_buffer(input_frame->video_frame_buffer())
- .set_color_space(&color_space)
+ .set_color_space(color_space)
.build();
EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK,
@@ -141,7 +141,7 @@
ASSERT_TRUE(WaitForEncodedFrame(&encoded_frame, &codec_specific_info));
// Add color space to encoded frame.
ColorSpace color_space = CreateTestColorSpace(/*with_hdr_metadata=*/false);
- encoded_frame.SetColorSpace(&color_space);
+ encoded_frame.SetColorSpace(color_space);
EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK,
decoder_->Decode(encoded_frame, false, nullptr, 0));
std::unique_ptr<VideoFrame> decoded_frame;
diff --git a/modules/video_coding/codecs/vp8/libvpx_vp8_decoder.cc b/modules/video_coding/codecs/vp8/libvpx_vp8_decoder.cc
index 000fc3a..d7258c9 100644
--- a/modules/video_coding/codecs/vp8/libvpx_vp8_decoder.cc
+++ b/modules/video_coding/codecs/vp8/libvpx_vp8_decoder.cc
@@ -283,11 +283,12 @@
return WEBRTC_VIDEO_CODEC_OK;
}
-int LibvpxVp8Decoder::ReturnFrame(const vpx_image_t* img,
- uint32_t timestamp,
- int64_t ntp_time_ms,
- int qp,
- const ColorSpace* explicit_color_space) {
+int LibvpxVp8Decoder::ReturnFrame(
+ const vpx_image_t* img,
+ uint32_t timestamp,
+ int64_t ntp_time_ms,
+ int qp,
+ const webrtc::ColorSpace* explicit_color_space) {
if (img == NULL) {
// Decoder OK and NULL image => No show frame
return WEBRTC_VIDEO_CODEC_NO_OUTPUT;
diff --git a/modules/video_coding/codecs/vp8/libvpx_vp8_decoder.h b/modules/video_coding/codecs/vp8/libvpx_vp8_decoder.h
index 0db4c3b..a6ddfef 100644
--- a/modules/video_coding/codecs/vp8/libvpx_vp8_decoder.h
+++ b/modules/video_coding/codecs/vp8/libvpx_vp8_decoder.h
@@ -54,7 +54,7 @@
uint32_t timeStamp,
int64_t ntp_time_ms,
int qp,
- const ColorSpace* explicit_color_space);
+ const webrtc::ColorSpace* explicit_color_space);
const bool use_postproc_arm_;
I420BufferPool buffer_pool_;
diff --git a/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc b/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc
index 12e13a9..06d9448 100644
--- a/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc
+++ b/modules/video_coding/codecs/vp8/test/vp8_impl_unittest.cc
@@ -193,7 +193,7 @@
VideoFrame input_frame_w_color_space =
VideoFrame::Builder()
.set_video_frame_buffer(input_frame->video_frame_buffer())
- .set_color_space(&color_space)
+ .set_color_space(color_space)
.build();
EncodeAndWaitForFrame(input_frame_w_color_space, &encoded_frame,
@@ -229,7 +229,7 @@
// Encoded frame with explicit color space information.
ColorSpace color_space = CreateTestColorSpace(/*with_hdr_metadata=*/false);
- encoded_frame.SetColorSpace(&color_space);
+ encoded_frame.SetColorSpace(color_space);
EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK,
decoder_->Decode(encoded_frame, false, nullptr, -1));
std::unique_ptr<VideoFrame> decoded_frame;
diff --git a/modules/video_coding/codecs/vp9/test/vp9_impl_unittest.cc b/modules/video_coding/codecs/vp9/test/vp9_impl_unittest.cc
index d03e196..7b5f07b 100644
--- a/modules/video_coding/codecs/vp9/test/vp9_impl_unittest.cc
+++ b/modules/video_coding/codecs/vp9/test/vp9_impl_unittest.cc
@@ -185,7 +185,7 @@
VideoFrame input_frame_w_hdr =
VideoFrame::Builder()
.set_video_frame_buffer(input_frame->video_frame_buffer())
- .set_color_space(&color_space)
+ .set_color_space(color_space)
.build();
EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK,
encoder_->Encode(input_frame_w_hdr, nullptr, nullptr));
@@ -215,7 +215,7 @@
// Encoded frame with explicit color space information.
ColorSpace color_space = CreateTestColorSpace(/*with_hdr_metadata=*/true);
- encoded_frame.SetColorSpace(&color_space);
+ encoded_frame.SetColorSpace(color_space);
EXPECT_EQ(WEBRTC_VIDEO_CODEC_OK,
decoder_->Decode(encoded_frame, false, nullptr, 0));
ASSERT_TRUE(WaitForDecodedFrame(&decoded_frame, &decoded_qp));
diff --git a/modules/video_coding/codecs/vp9/vp9_impl.cc b/modules/video_coding/codecs/vp9/vp9_impl.cc
index d86f78f..3bf03fa 100644
--- a/modules/video_coding/codecs/vp9/vp9_impl.cc
+++ b/modules/video_coding/codecs/vp9/vp9_impl.cc
@@ -1480,11 +1480,12 @@
return WEBRTC_VIDEO_CODEC_OK;
}
-int VP9DecoderImpl::ReturnFrame(const vpx_image_t* img,
- uint32_t timestamp,
- int64_t ntp_time_ms,
- int qp,
- const ColorSpace* explicit_color_space) {
+int VP9DecoderImpl::ReturnFrame(
+ const vpx_image_t* img,
+ uint32_t timestamp,
+ int64_t ntp_time_ms,
+ int qp,
+ const webrtc::ColorSpace* explicit_color_space) {
if (img == nullptr) {
// Decoder OK and nullptr image => No show frame.
return WEBRTC_VIDEO_CODEC_NO_OUTPUT;
@@ -1533,7 +1534,7 @@
.set_ntp_time_ms(ntp_time_ms)
.set_rotation(webrtc::kVideoRotation_0);
if (explicit_color_space) {
- builder.set_color_space(explicit_color_space);
+ builder.set_color_space(*explicit_color_space);
} else {
builder.set_color_space(
ExtractVP9ColorSpace(img->cs, img->range, img->bit_depth));
diff --git a/modules/video_coding/codecs/vp9/vp9_impl.h b/modules/video_coding/codecs/vp9/vp9_impl.h
index a2dab26..bb87dbf 100644
--- a/modules/video_coding/codecs/vp9/vp9_impl.h
+++ b/modules/video_coding/codecs/vp9/vp9_impl.h
@@ -177,7 +177,7 @@
uint32_t timestamp,
int64_t ntp_time_ms,
int qp,
- const ColorSpace* explicit_color_space);
+ const webrtc::ColorSpace* explicit_color_space);
// Memory pool used to share buffers between libvpx and webrtc.
Vp9FrameBufferPool frame_buffer_pool_;
diff --git a/modules/video_coding/frame_object.cc b/modules/video_coding/frame_object.cc
index 4f5f9d4..1e0b647 100644
--- a/modules/video_coding/frame_object.cc
+++ b/modules/video_coding/frame_object.cc
@@ -73,9 +73,7 @@
// frame (I-frame or IDR frame in H.264 (AVC), or an IRAP picture in H.265
// (HEVC)).
rotation_ = last_packet->video_header.rotation;
- SetColorSpace(last_packet->video_header.color_space
- ? &last_packet->video_header.color_space.value()
- : nullptr);
+ SetColorSpace(last_packet->video_header.color_space);
_rotation_set = true;
content_type_ = last_packet->video_header.content_type;
if (last_packet->video_header.video_timing.flags !=
diff --git a/style-guide.md b/style-guide.md
index 391c45b..2a35fdc 100644
--- a/style-guide.md
+++ b/style-guide.md
@@ -77,18 +77,6 @@
See [the source](api/array_view.h) for more detailed docs.
-### `absl::optional<T>` as function argument
-
-`absl::optional<T>` is generally a good choice when you want to pass a
-possibly missing `T` to a function—provided of course that `T`
-is a type that it makes sense to pass by value.
-
-However, when you want to avoid pass-by-value, generally **do not pass
-`const absl::optional<T>&`; use `const T*` instead.** `const
-absl::optional<T>&` forces the caller to store the `T` in an
-`absl::optional<T>`; `const T*`, on the other hand, makes no
-assumptions about how the `T` is stored.
-
### sigslot
sigslot is a lightweight library that adds a signal/slot language
diff --git a/test/frame_generator_capturer.cc b/test/frame_generator_capturer.cc
index d086b60..49ce40a 100644
--- a/test/frame_generator_capturer.cc
+++ b/test/frame_generator_capturer.cc
@@ -150,7 +150,7 @@
frame->set_ntp_time_ms(clock_->CurrentNtpInMilliseconds());
frame->set_rotation(fake_rotation_);
if (fake_color_space_) {
- frame->set_color_space(&fake_color_space_.value());
+ frame->set_color_space(fake_color_space_);
}
if (first_frame_capture_time_ == -1) {
first_frame_capture_time_ = frame->ntp_time_ms();
