diff --git a/api/media_transport_interface.cc b/api/media_transport_interface.cc index 571e82b720..63d7ea1565 100644 --- a/api/media_transport_interface.cc +++ b/api/media_transport_interface.cc @@ -74,33 +74,10 @@ MediaTransportEncodedVideoFrame::MediaTransportEncodedVideoFrame( referenced_frame_ids_(std::move(referenced_frame_ids)) {} MediaTransportEncodedVideoFrame& MediaTransportEncodedVideoFrame::operator=( - const MediaTransportEncodedVideoFrame& o) { - payload_type_ = o.payload_type_; - encoded_image_ = o.encoded_image_; - encoded_data_ = o.encoded_data_; - frame_id_ = o.frame_id_; - referenced_frame_ids_ = o.referenced_frame_ids_; - if (!encoded_data_.empty()) { - // We own the underlying data. - encoded_image_.set_buffer(encoded_data_.data(), encoded_data_.size()); - } - return *this; -} + const MediaTransportEncodedVideoFrame&) = default; MediaTransportEncodedVideoFrame& MediaTransportEncodedVideoFrame::operator=( - MediaTransportEncodedVideoFrame&& o) { - payload_type_ = o.payload_type_; - encoded_image_ = o.encoded_image_; - encoded_data_ = std::move(o.encoded_data_); - frame_id_ = o.frame_id_; - referenced_frame_ids_ = std::move(o.referenced_frame_ids_); - if (!encoded_data_.empty()) { - // We take over ownership of the underlying data. - encoded_image_.set_buffer(encoded_data_.data(), encoded_data_.size()); - o.encoded_image_.set_buffer(nullptr, 0); - } - return *this; -} + MediaTransportEncodedVideoFrame&&) = default; MediaTransportEncodedVideoFrame::MediaTransportEncodedVideoFrame( const MediaTransportEncodedVideoFrame& o) @@ -114,14 +91,6 @@ MediaTransportEncodedVideoFrame::MediaTransportEncodedVideoFrame( *this = std::move(o); } -void MediaTransportEncodedVideoFrame::Retain() { - if (encoded_image_.data() && encoded_data_.empty()) { - encoded_data_ = std::vector( - encoded_image_.data(), encoded_image_.data() + encoded_image_.size()); - encoded_image_.set_buffer(encoded_data_.data(), encoded_image_.size()); - } -} - SendDataParams::SendDataParams() = default; SendDataParams::SendDataParams(const SendDataParams&) = default; diff --git a/api/media_transport_interface.h b/api/media_transport_interface.h index 126600062b..5cd29234b6 100644 --- a/api/media_transport_interface.h +++ b/api/media_transport_interface.h @@ -204,23 +204,20 @@ class MediaTransportEncodedVideoFrame final { return referenced_frame_ids_; } - // Hack to workaround lack of ownership of the encoded_image_._buffer. If we + // Hack to workaround lack of ownership of the EncodedImage buffer. If we // don't already own the underlying data, make a copy. - void Retain(); + void Retain() { encoded_image_.Retain(); } private: MediaTransportEncodedVideoFrame(); int payload_type_; - // The buffer is not owned by the encoded image. On the sender it means that - // it will need to make a copy using the Retain() method, if it wants to + // The buffer is not always owned by the encoded image. On the sender it means + // that it will need to make a copy using the Retain() method, if it wants to // deliver it asynchronously. webrtc::EncodedImage encoded_image_; - // If non-empty, this is the data for the encoded image. - std::vector encoded_data_; - // Frame id uniquely identifies a frame in a stream. It needs to be unique in // a given time window (i.e. technically unique identifier for the lifetime of // the connection is not needed, but you need to guarantee that remote side diff --git a/api/video/encoded_image.cc b/api/video/encoded_image.cc index b093d1b603..f96b9c2783 100644 --- a/api/video/encoded_image.cc +++ b/api/video/encoded_image.cc @@ -29,10 +29,24 @@ size_t EncodedImage::GetBufferPaddingBytes(VideoCodecType codec_type) { EncodedImage::EncodedImage() : EncodedImage(nullptr, 0, 0) {} +EncodedImage::EncodedImage(EncodedImage&&) = default; EncodedImage::EncodedImage(const EncodedImage&) = default; EncodedImage::EncodedImage(uint8_t* buffer, size_t size, size_t capacity) - : buffer_(buffer), size_(size), capacity_(capacity) {} + : size_(size), buffer_(buffer), capacity_(capacity) {} + +EncodedImage::~EncodedImage() = default; + +EncodedImage& EncodedImage::operator=(EncodedImage&&) = default; +EncodedImage& EncodedImage::operator=(const EncodedImage&) = default; + +void EncodedImage::Retain() { + if (buffer_) { + encoded_data_ = std::vector(size_); + memcpy(encoded_data_.data(), buffer_, size_); + buffer_ = nullptr; + } +} void EncodedImage::SetEncodeTime(int64_t encode_start_ms, int64_t encode_finish_ms) { diff --git a/api/video/encoded_image.h b/api/video/encoded_image.h index fb3288f2f9..363d34cfec 100644 --- a/api/video/encoded_image.h +++ b/api/video/encoded_image.h @@ -12,6 +12,7 @@ #define API_VIDEO_ENCODED_IMAGE_H_ #include +#include #include "absl/types/optional.h" #include "api/video/color_space.h" @@ -37,9 +38,17 @@ class RTC_EXPORT EncodedImage { static size_t GetBufferPaddingBytes(VideoCodecType codec_type); EncodedImage(); + EncodedImage(EncodedImage&&); + // Discouraged: potentially expensive. EncodedImage(const EncodedImage&); EncodedImage(uint8_t* buffer, size_t length, size_t capacity); + ~EncodedImage(); + + EncodedImage& operator=(EncodedImage&&); + // Discouraged: potentially expensive. + EncodedImage& operator=(const EncodedImage&); + // TODO(nisse): Change style to timestamp(), set_timestamp(), for consistency // with the VideoFrame class. // Set frame timestamp (90kHz). @@ -68,19 +77,38 @@ class RTC_EXPORT EncodedImage { size_t size() const { return size_; } void set_size(size_t new_size) { - RTC_DCHECK_LE(new_size, capacity_); + RTC_DCHECK_LE(new_size, capacity()); size_ = new_size; } - size_t capacity() const { return capacity_; } + size_t capacity() const { return buffer_ ? capacity_ : encoded_data_.size(); } void set_buffer(uint8_t* buffer, size_t capacity) { buffer_ = buffer; capacity_ = capacity; } - // TODO(bugs.webrtc.org/9378): When changed to owning the buffer, data() on a - // const object should return a const uint8_t*. - uint8_t* data() const { return buffer_; } + void Allocate(size_t capacity) { + encoded_data_.resize(capacity); + buffer_ = nullptr; + } + + uint8_t* data() { return buffer_ ? buffer_ : encoded_data_.data(); } + const uint8_t* data() const { + return buffer_ ? buffer_ : encoded_data_.data(); + } + // TODO(nisse): At some places, code accepts a const ref EncodedImage, but + // still writes to it, to clear padding at the end of the encoded data. + // Padding is required by ffmpeg; the best way to deal with that is likely to + // make this class ensure that buffers always have a few zero padding bytes. + uint8_t* mutable_data() const { return const_cast(data()); } + + // TODO(bugs.webrtc.org/9378): Delete. Used by code that wants to modify a + // buffer corresponding to a const EncodedImage. Requires an un-owned buffer. + uint8_t* buffer() const { return buffer_; } + + // Hack to workaround lack of ownership of the encoded data. If we don't + // already own the underlying data, make an owned copy. + void Retain(); uint32_t _encodedWidth = 0; uint32_t _encodedHeight = 0; @@ -111,11 +139,14 @@ class RTC_EXPORT EncodedImage { } timing_; private: - // TODO(bugs.webrtc.org/9378): Fix ownership. Currently not owning the data - // buffer. - uint8_t* buffer_; + // TODO(bugs.webrtc.org/9378): We're transitioning to always owning the + // encoded data. + std::vector encoded_data_; size_t size_; // Size of encoded frame data. - size_t capacity_; // Allocated size of _buffer. + // Non-null when used with an un-owned buffer. + uint8_t* buffer_; + // Allocated size of _buffer; relevant only if it's non-null. + size_t capacity_; uint32_t timestamp_rtp_ = 0; absl::optional spatial_index_; absl::optional color_space_; diff --git a/modules/video_coding/codecs/h264/h264_decoder_impl.cc b/modules/video_coding/codecs/h264/h264_decoder_impl.cc index 9f4c696ec2..73190d1a65 100644 --- a/modules/video_coding/codecs/h264/h264_decoder_impl.cc +++ b/modules/video_coding/codecs/h264/h264_decoder_impl.cc @@ -259,12 +259,12 @@ int32_t H264DecoderImpl::Decode(const EncodedImage& input_image, // "If the first 23 bits of the additional bytes are not 0, then damaged MPEG // bitstreams could cause overread and segfault." See // AV_INPUT_BUFFER_PADDING_SIZE. We'll zero the entire padding just in case. - memset(input_image.data() + input_image.size(), 0, + memset(input_image.mutable_data() + input_image.size(), 0, EncodedImage::GetBufferPaddingBytes(kVideoCodecH264)); AVPacket packet; av_init_packet(&packet); - packet.data = input_image.data(); + packet.data = input_image.mutable_data(); if (input_image.size() > static_cast(std::numeric_limits::max())) { ReportError(); diff --git a/modules/video_coding/codecs/multiplex/multiplex_encoded_image_packer.cc b/modules/video_coding/codecs/multiplex/multiplex_encoded_image_packer.cc index af1c8a65e2..1afdf289eb 100644 --- a/modules/video_coding/codecs/multiplex/multiplex_encoded_image_packer.cc +++ b/modules/video_coding/codecs/multiplex/multiplex_encoded_image_packer.cc @@ -221,7 +221,7 @@ EncodedImage MultiplexEncodedImagePacker::PackAndRelease( for (size_t i = 0; i < images.size(); i++) { PackBitstream(combined_image.data() + frame_headers[i].bitstream_offset, images[i]); - delete[] images[i].encoded_image.data(); + delete[] images[i].encoded_image.buffer(); } return combined_image; @@ -263,7 +263,7 @@ MultiplexImage MultiplexEncodedImagePacker::Unpack( encoded_image.SetTimestamp(combined_image.Timestamp()); encoded_image._frameType = frame_headers[i].frame_type; encoded_image.set_buffer( - combined_image.data() + frame_headers[i].bitstream_offset, + combined_image.mutable_data() + frame_headers[i].bitstream_offset, static_cast(frame_headers[i].bitstream_length)); const size_t padding = EncodedImage::GetBufferPaddingBytes(image_component.codec_type); diff --git a/modules/video_coding/codecs/multiplex/multiplex_encoder_adapter.cc b/modules/video_coding/codecs/multiplex/multiplex_encoder_adapter.cc index 637bce538b..fb588eba46 100644 --- a/modules/video_coding/codecs/multiplex/multiplex_encoder_adapter.cc +++ b/modules/video_coding/codecs/multiplex/multiplex_encoder_adapter.cc @@ -253,8 +253,8 @@ int MultiplexEncoderAdapter::Release() { } } stashed_images_.clear(); - if (combined_image_.data()) { - delete[] combined_image_.data(); + if (combined_image_.buffer()) { + delete[] combined_image_.buffer(); combined_image_.set_buffer(nullptr, 0); } return WEBRTC_VIDEO_CODEC_OK; @@ -302,8 +302,8 @@ EncodedImageCallback::Result MultiplexEncoderAdapter::OnEncodedImage( // We have to send out those stashed frames, otherwise the delta frame // dependency chain is broken. - if (combined_image_.data()) - delete[] combined_image_.data(); + if (combined_image_.buffer()) + delete[] combined_image_.buffer(); combined_image_ = MultiplexEncodedImagePacker::PackAndRelease(iter->second); diff --git a/modules/video_coding/codecs/test/videoprocessor.cc b/modules/video_coding/codecs/test/videoprocessor.cc index b30c0321ef..c64d2905c0 100644 --- a/modules/video_coding/codecs/test/videoprocessor.cc +++ b/modules/video_coding/codecs/test/videoprocessor.cc @@ -587,9 +587,9 @@ const webrtc::EncodedImage* VideoProcessor::BuildAndStoreSuperframe( copied_image.set_size(payload_size_bytes); // Replace previous EncodedImage for this spatial layer. - uint8_t* old_data = merged_encoded_frames_.at(spatial_idx).data(); - if (old_data) { - delete[] old_data; + uint8_t* old_buffer = merged_encoded_frames_.at(spatial_idx).buffer(); + if (old_buffer) { + delete[] old_buffer; } merged_encoded_frames_.at(spatial_idx) = copied_image; diff --git a/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc b/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc index 5e6a402999..42e57a65fa 100644 --- a/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc +++ b/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc @@ -888,7 +888,7 @@ int LibvpxVp8Encoder::GetEncodedPartitions(const VideoFrame& input_image) { encoded_images_[encoder_idx].capacity()) { uint8_t* buffer = new uint8_t[pkt->data.frame.sz + length]; memcpy(buffer, encoded_images_[encoder_idx].data(), length); - delete[] encoded_images_[encoder_idx].data(); + delete[] encoded_images_[encoder_idx].buffer(); encoded_images_[encoder_idx].set_buffer( buffer, pkt->data.frame.sz + length); } diff --git a/modules/video_coding/codecs/vp9/vp9_impl.cc b/modules/video_coding/codecs/vp9/vp9_impl.cc index 3bf03fa4b7..5feca0b0a9 100644 --- a/modules/video_coding/codecs/vp9/vp9_impl.cc +++ b/modules/video_coding/codecs/vp9/vp9_impl.cc @@ -185,8 +185,8 @@ VP9EncoderImpl::~VP9EncoderImpl() { int VP9EncoderImpl::Release() { int ret_val = WEBRTC_VIDEO_CODEC_OK; - if (encoded_image_.data() != nullptr) { - delete[] encoded_image_.data(); + if (encoded_image_.buffer() != nullptr) { + delete[] encoded_image_.buffer(); encoded_image_.set_buffer(nullptr, 0); } if (encoder_ != nullptr) { @@ -1266,7 +1266,7 @@ int VP9EncoderImpl::GetEncodedLayerFrame(const vpx_codec_cx_pkt* pkt) { } if (pkt->data.frame.sz > encoded_image_.capacity()) { - delete[] encoded_image_.data(); + delete[] encoded_image_.buffer(); encoded_image_.set_buffer(new uint8_t[pkt->data.frame.sz], pkt->data.frame.sz); } diff --git a/modules/video_coding/encoded_frame.cc b/modules/video_coding/encoded_frame.cc index aff9c5a5d4..c18ef131a6 100644 --- a/modules/video_coding/encoded_frame.cc +++ b/modules/video_coding/encoded_frame.cc @@ -31,15 +31,7 @@ VCMEncodedFrame::VCMEncodedFrame() } VCMEncodedFrame::~VCMEncodedFrame() { - Free(); -} - -void VCMEncodedFrame::Free() { Reset(); - if (data() != nullptr) { - delete[] data(); - set_buffer(nullptr, 0); - } } void VCMEncodedFrame::Reset() { @@ -156,15 +148,10 @@ void VCMEncodedFrame::CopyCodecSpecific(const RTPVideoHeader* header) { void VCMEncodedFrame::VerifyAndAllocate(size_t minimumSize) { size_t old_capacity = capacity(); if (minimumSize > old_capacity) { - // create buffer of sufficient size - uint8_t* old_data = data(); - - set_buffer(new uint8_t[minimumSize], minimumSize); - if (old_data) { - // copy old data - memcpy(data(), old_data, old_capacity); - delete[] old_data; - } + // TODO(nisse): EncodedImage::Allocate is implemented as + // std::vector::resize, which means that old contents is kept. Find out if + // any code depends on that behavior. + Allocate(minimumSize); } } diff --git a/modules/video_coding/encoded_frame.h b/modules/video_coding/encoded_frame.h index 2cdcff96fb..eeaea1532e 100644 --- a/modules/video_coding/encoded_frame.h +++ b/modules/video_coding/encoded_frame.h @@ -27,10 +27,6 @@ class VCMEncodedFrame : protected EncodedImage { VCMEncodedFrame(const VCMEncodedFrame&) = delete; ~VCMEncodedFrame(); - /** - * Delete VideoFrame and resets members to zero - */ - void Free(); /** * Set render time in milliseconds */ diff --git a/modules/video_coding/frame_object.cc b/modules/video_coding/frame_object.cc index 1e0b647b23..c3c9f2346e 100644 --- a/modules/video_coding/frame_object.cc +++ b/modules/video_coding/frame_object.cc @@ -170,15 +170,11 @@ void RtpFrameObject::AllocateBitstreamBuffer(size_t frame_size) { // Since FFmpeg use an optimized bitstream reader that reads in chunks of // 32/64 bits we have to add at least that much padding to the buffer // to make sure the decoder doesn't read out of bounds. - // NOTE! EncodedImage::_size is the size of the buffer (think capacity of - // an std::vector) and EncodedImage::_length is the actual size of - // the bitstream (think size of an std::vector). size_t new_size = frame_size + (codec_type_ == kVideoCodecH264 ? EncodedImage::kBufferPaddingBytesH264 : 0); if (capacity() < new_size) { - delete[] data(); - set_buffer(new uint8_t[new_size], new_size); + Allocate(new_size); } set_size(frame_size); diff --git a/modules/video_coding/utility/simulcast_test_fixture_impl.cc b/modules/video_coding/utility/simulcast_test_fixture_impl.cc index c65941a930..5af14cc3fc 100644 --- a/modules/video_coding/utility/simulcast_test_fixture_impl.cc +++ b/modules/video_coding/utility/simulcast_test_fixture_impl.cc @@ -82,7 +82,7 @@ class SimulcastTestFixtureImpl::TestEncodedImageCallback // Only store the base layer. if (encoded_image.SpatialIndex().value_or(0) == 0) { if (encoded_image._frameType == kVideoFrameKey) { - delete[] encoded_key_frame_.data(); + delete[] encoded_key_frame_.buffer(); encoded_key_frame_.set_buffer(new uint8_t[encoded_image.capacity()], encoded_image.capacity()); encoded_key_frame_.set_size(encoded_image.size()); @@ -91,7 +91,7 @@ class SimulcastTestFixtureImpl::TestEncodedImageCallback memcpy(encoded_key_frame_.data(), encoded_image.data(), encoded_image.size()); } else { - delete[] encoded_frame_.data(); + delete[] encoded_frame_.buffer(); encoded_frame_.set_buffer(new uint8_t[encoded_image.capacity()], encoded_image.capacity()); encoded_frame_.set_size(encoded_image.size()); @@ -905,7 +905,7 @@ void SimulcastTestFixtureImpl::TestDecodeWidthHeightSet() { EXPECT_EQ(0, decoder_->Decode(encoded_frame[2], false, NULL, 0)); for (int i = 0; i < 3; ++i) { - delete[] encoded_frame[i].data(); + delete[] encoded_frame[i].buffer(); } } diff --git a/test/fake_encoder.cc b/test/fake_encoder.cc index c4cbfe11ea..85e61510c1 100644 --- a/test/fake_encoder.cc +++ b/test/fake_encoder.cc @@ -313,23 +313,26 @@ EncodedImageCallback::Result FakeH264Encoder::OnEncodedImage( const size_t kSpsNalHeader = 0x67; const size_t kPpsNalHeader = 0x68; const size_t kIdrNalHeader = 0x65; - encoded_image.data()[fragmentation.fragmentationOffset[0]] = kSpsNalHeader; - encoded_image.data()[fragmentation.fragmentationOffset[1]] = kPpsNalHeader; - encoded_image.data()[fragmentation.fragmentationOffset[2]] = kIdrNalHeader; + encoded_image.buffer()[fragmentation.fragmentationOffset[0]] = + kSpsNalHeader; + encoded_image.buffer()[fragmentation.fragmentationOffset[1]] = + kPpsNalHeader; + encoded_image.buffer()[fragmentation.fragmentationOffset[2]] = + kIdrNalHeader; } else { const size_t kNumSlices = 1; fragmentation.VerifyAndAllocateFragmentationHeader(kNumSlices); fragmentation.fragmentationOffset[0] = 0; fragmentation.fragmentationLength[0] = encoded_image.size(); const size_t kNalHeader = 0x41; - encoded_image.data()[fragmentation.fragmentationOffset[0]] = kNalHeader; + encoded_image.buffer()[fragmentation.fragmentationOffset[0]] = kNalHeader; } uint8_t value = 0; int fragment_counter = 0; for (size_t i = 0; i < encoded_image.size(); ++i) { if (fragment_counter == fragmentation.fragmentationVectorSize || i != fragmentation.fragmentationOffset[fragment_counter]) { - encoded_image.data()[i] = value++; + encoded_image.buffer()[i] = value++; } else { ++fragment_counter; } diff --git a/test/fake_vp8_encoder.cc b/test/fake_vp8_encoder.cc index b6a89b81da..c368eb50f0 100644 --- a/test/fake_vp8_encoder.cc +++ b/test/fake_vp8_encoder.cc @@ -127,7 +127,7 @@ EncodedImageCallback::Result FakeVP8Encoder::OnEncodedImage( // Write width and height to the payload the same way as the real encoder // does. - WriteFakeVp8(encoded_image.data(), encoded_image._encodedWidth, + WriteFakeVp8(encoded_image.buffer(), encoded_image._encodedWidth, encoded_image._encodedHeight, encoded_image._frameType == kVideoFrameKey); return callback_->OnEncodedImage(encoded_image, &overrided_specific_info, diff --git a/video/video_receive_stream.cc b/video/video_receive_stream.cc index 836ad94ebb..4e6444a595 100644 --- a/video/video_receive_stream.cc +++ b/video/video_receive_stream.cc @@ -124,16 +124,20 @@ class EncodedFrameForMediaTransport : public video_coding::EncodedFrame { public: explicit EncodedFrameForMediaTransport( MediaTransportEncodedVideoFrame frame) { - // TODO(nisse): This is too ugly. We copy the EncodedImage (a base class of - // ours, in several steps), to get all the meta data. But we then need to - // reset the buffer and allocate a new copy, since EncodedFrame must own it. + // TODO(nisse): This is ugly. We copy the EncodedImage (a base class of + // ours, in several steps), to get all the meta data. We should be using + // std::move in some way. Then we also need to handle the case of an unowned + // buffer, in which case we need to make an owned copy. *static_cast(this) = frame.encoded_image(); - // Don't use the copied _buffer pointer. - set_buffer(nullptr, 0); - VerifyAndAllocate(frame.encoded_image().size()); - set_size(frame.encoded_image().size()); - memcpy(data(), frame.encoded_image().data(), size()); + if (buffer()) { + // Unowned data. Make a copy we own. + set_buffer(nullptr, 0); + + VerifyAndAllocate(frame.encoded_image().size()); + set_size(frame.encoded_image().size()); + memcpy(data(), frame.encoded_image().data(), size()); + } _payloadType = static_cast(frame.payload_type());