From 5acd9d0393260b63e80b435fed9e5024ea6f4498 Mon Sep 17 00:00:00 2001 From: Zijie He Date: Thu, 24 Aug 2017 12:41:53 -0700 Subject: [PATCH] Add DesktopFrame::MoveFrameInfoFrom() and DesktopFrame::CopyFrameInfoFrom() The original change https://chromium-review.googlesource.com/c/575315 and https://chromium-review.googlesource.com/c/590508 have not been well-considered. So this change reverts part of two changes and adds a DesktopFrame::set_top_left() function A DesktopFrame usually contains a very large chunk of memory, which should be reused as much as possible to reduce the memory allocations. The size of the memory usually controls by the DesktopFrame::size(). So it's reasonable to const DesktopFrame::size_: changing it is wrong if the underly buffer is not large enough. But DesktopFrame::top_left() is a different story, same as capturer_id, capture_time_ms and other information in the DesktopFrame, it can be changed to any value without needing to reconstruct a DesktopFrame instance. So instead of adding it to the constructor, a DesktopFrame::set_top_left() is added to adjust the top-left of the DesktopFrame in the entire display coordinate. After adding DesktopFrame::set_top_left(), we have five variables in a DesktopFrame which is not initialized in the constructor. For any kind of wrapper DesktopFrame, say, SharedDesktopFrame and CroppedDesktopFrame, they needs to copy these five variables after constructing themselves. This is not convenient and easily to be broken if an implementation forgot to copy them. So DesktopFrame::MoveFrameInfoFrom() and DesktopFrame::CopyFrameInfoFrom() are added to the DesktopFrame to help derived classes to copy or move these variables in one function call. The difference between MoveFrameInfoFrom() and CopyFrameInfoFrom() is that the former one uses DesktopRegion::Swap() to move the DesktopRegion from the source DesktopFrame to this instance, while the later one uses copy-operator to copy the DesktopRegion from the source DesktopFrame. So CopyFrameInfoFrom() is usually used when sharing a source DesktopFrame with several clients. I.e. the source DesktopFrame should be kept unchanged. For example, BasicDesktopFrame::CopyOf() and SharedDesktopFrame::Share(). On the other side, MoveFrameInfoFrom() is usually used when wrapping a DesktopFrame. E.g. CroppedDesktopFrame and DesktopFrameWithCursor. Bug: webrtc:7950 Change-Id: I8b23418960fb681d2ea1f012d1b453f514da2272 Reviewed-on: https://chromium-review.googlesource.com/622453 Commit-Queue: Zijie He Reviewed-by: Jamie Walch Cr-Commit-Position: refs/heads/master@{#19504} --- .../desktop_capture/cropped_desktop_frame.cc | 23 ++---- .../desktop_and_cursor_composer.cc | 13 ++- .../desktop_capturer_differ_wrapper.cc | 5 +- .../modules/desktop_capture/desktop_frame.cc | 81 ++++++++----------- .../modules/desktop_capture/desktop_frame.h | 54 ++++++------- .../desktop_capture/shared_desktop_frame.cc | 19 +++-- .../desktop_capture/shared_desktop_frame.h | 8 +- 7 files changed, 93 insertions(+), 110 deletions(-) diff --git a/webrtc/modules/desktop_capture/cropped_desktop_frame.cc b/webrtc/modules/desktop_capture/cropped_desktop_frame.cc index ac3cd1c5e5..67f03e74ee 100644 --- a/webrtc/modules/desktop_capture/cropped_desktop_frame.cc +++ b/webrtc/modules/desktop_capture/cropped_desktop_frame.cc @@ -17,17 +17,6 @@ namespace webrtc { -namespace { - -DesktopRect TranslateRect(const DesktopRect& rect, - const DesktopVector& top_left) { - DesktopRect result = rect; - result.Translate(top_left); - return result; -} - -} // namespace - // A DesktopFrame that is a sub-rect of another DesktopFrame. class CroppedDesktopFrame : public DesktopFrame { public: @@ -35,7 +24,7 @@ class CroppedDesktopFrame : public DesktopFrame { const DesktopRect& rect); private: - std::unique_ptr frame_; + const std::unique_ptr frame_; RTC_DISALLOW_COPY_AND_ASSIGN(CroppedDesktopFrame); }; @@ -59,11 +48,15 @@ std::unique_ptr CreateCroppedDesktopFrame( CroppedDesktopFrame::CroppedDesktopFrame(std::unique_ptr frame, const DesktopRect& rect) - : DesktopFrame(TranslateRect(rect, frame->top_left()), + : DesktopFrame(frame->size(), frame->stride(), frame->GetFrameDataAtPos(rect.top_left()), - frame->shared_memory()) { - frame_ = std::move(frame); + frame->shared_memory()), + frame_(std::move(frame)) { + MoveFrameInfoFrom(frame_.get()); + set_top_left(frame_->top_left().add(rect.top_left())); + mutable_updated_region()->IntersectWith(rect); + mutable_updated_region()->Translate(-rect.left(), -rect.top()); } } // namespace webrtc diff --git a/webrtc/modules/desktop_capture/desktop_and_cursor_composer.cc b/webrtc/modules/desktop_capture/desktop_and_cursor_composer.cc index cc7c522cfc..87f5726581 100644 --- a/webrtc/modules/desktop_capture/desktop_and_cursor_composer.cc +++ b/webrtc/modules/desktop_capture/desktop_and_cursor_composer.cc @@ -67,7 +67,7 @@ class DesktopFrameWithCursor : public DesktopFrame { ~DesktopFrameWithCursor() override; private: - std::unique_ptr original_frame_; + const std::unique_ptr original_frame_; DesktopVector restore_position_; std::unique_ptr restore_frame_; @@ -79,15 +79,12 @@ DesktopFrameWithCursor::DesktopFrameWithCursor( std::unique_ptr frame, const MouseCursor& cursor, const DesktopVector& position) - : DesktopFrame(frame->rect(), + : DesktopFrame(frame->size(), frame->stride(), frame->data(), - frame->shared_memory()) { - set_dpi(frame->dpi()); - set_capture_time_ms(frame->capture_time_ms()); - set_capturer_id(frame->capturer_id()); - mutable_updated_region()->Swap(frame->mutable_updated_region()); - original_frame_ = std::move(frame); + frame->shared_memory()), + original_frame_(std::move(frame)) { + MoveFrameInfoFrom(original_frame_.get()); DesktopVector image_pos = position.subtract(cursor.hotspot()); DesktopRect target_rect = DesktopRect::MakeSize(cursor.image()->size()); diff --git a/webrtc/modules/desktop_capture/desktop_capturer_differ_wrapper.cc b/webrtc/modules/desktop_capture/desktop_capturer_differ_wrapper.cc index f3a846c765..8a93bc15b8 100644 --- a/webrtc/modules/desktop_capture/desktop_capturer_differ_wrapper.cc +++ b/webrtc/modules/desktop_capture/desktop_capturer_differ_wrapper.cc @@ -201,7 +201,7 @@ void DesktopCapturerDifferWrapper::OnCaptureResult( if (last_frame_) { DesktopRegion hints; - hints.Swap(frame->GetUnderlyingFrame()->mutable_updated_region()); + hints.Swap(frame->mutable_updated_region()); for (DesktopRegion::Iterator it(hints); !it.IsAtEnd(); it.Advance()) { CompareFrames(*last_frame_, *frame, it.rect(), frame->mutable_updated_region()); @@ -212,10 +212,9 @@ void DesktopCapturerDifferWrapper::OnCaptureResult( } last_frame_ = frame->Share(); - frame->set_capture_time_ms(frame->GetUnderlyingFrame()->capture_time_ms() + + frame->set_capture_time_ms(frame->capture_time_ms() + (rtc::TimeNanos() - start_time_nanos) / rtc::kNumNanosecsPerMillisec); - frame->set_capturer_id(frame->GetUnderlyingFrame()->capturer_id()); callback_->OnCaptureResult(result, std::move(frame)); } diff --git a/webrtc/modules/desktop_capture/desktop_frame.cc b/webrtc/modules/desktop_capture/desktop_frame.cc index dbc3e3ebac..75317ce6a1 100644 --- a/webrtc/modules/desktop_capture/desktop_frame.cc +++ b/webrtc/modules/desktop_capture/desktop_frame.cc @@ -24,23 +24,14 @@ DesktopFrame::DesktopFrame(DesktopSize size, int stride, uint8_t* data, SharedMemory* shared_memory) - : DesktopFrame(DesktopRect::MakeSize(size), - stride, - data, - shared_memory) {} - -DesktopFrame::DesktopFrame(DesktopRect rect, - int stride, - uint8_t* data, - SharedMemory* shared_memory) : data_(data), shared_memory_(shared_memory), - rect_(rect), + size_(size), stride_(stride), capture_time_ms_(0), capturer_id_(DesktopCapturerId::kUnknown) { - RTC_DCHECK(rect.width() >= 0); - RTC_DCHECK(rect.height() >= 0); + RTC_DCHECK(size_.width() >= 0); + RTC_DCHECK(size_.height() >= 0); } DesktopFrame::~DesktopFrame() = default; @@ -71,18 +62,32 @@ uint8_t* DesktopFrame::GetFrameDataAtPos(const DesktopVector& pos) const { return data() + stride() * pos.y() + DesktopFrame::kBytesPerPixel * pos.x(); } -BasicDesktopFrame::BasicDesktopFrame(DesktopSize size) - : BasicDesktopFrame(DesktopRect::MakeSize(size)) {} +void DesktopFrame::CopyFrameInfoFrom(const DesktopFrame& other) { + set_dpi(other.dpi()); + set_capture_time_ms(other.capture_time_ms()); + set_capturer_id(other.capturer_id()); + *mutable_updated_region() = other.updated_region(); + set_top_left(other.top_left()); +} -BasicDesktopFrame::BasicDesktopFrame(DesktopRect rect) - : DesktopFrame(rect, kBytesPerPixel * rect.width(), - new uint8_t[kBytesPerPixel * rect.width() * rect.height()], +void DesktopFrame::MoveFrameInfoFrom(DesktopFrame* other) { + set_dpi(other->dpi()); + set_capture_time_ms(other->capture_time_ms()); + set_capturer_id(other->capturer_id()); + mutable_updated_region()->Swap(other->mutable_updated_region()); + set_top_left(other->top_left()); +} + +BasicDesktopFrame::BasicDesktopFrame(DesktopSize size) + : DesktopFrame(size, kBytesPerPixel * size.width(), + new uint8_t[kBytesPerPixel * size.width() * size.height()], nullptr) {} BasicDesktopFrame::~BasicDesktopFrame() { delete[] data_; } +// static DesktopFrame* BasicDesktopFrame::CopyOf(const DesktopFrame& frame) { DesktopFrame* result = new BasicDesktopFrame(frame.size()); for (int y = 0; y < frame.size().height(); ++y) { @@ -90,10 +95,7 @@ DesktopFrame* BasicDesktopFrame::CopyOf(const DesktopFrame& frame) { frame.data() + y * frame.stride(), frame.size().width() * kBytesPerPixel); } - result->set_dpi(frame.dpi()); - result->set_capture_time_ms(frame.capture_time_ms()); - result->set_capturer_id(frame.capturer_id()); - *result->mutable_updated_region() = frame.updated_region(); + result->CopyFrameInfoFrom(frame); return result; } @@ -101,49 +103,34 @@ DesktopFrame* BasicDesktopFrame::CopyOf(const DesktopFrame& frame) { std::unique_ptr SharedMemoryDesktopFrame::Create( DesktopSize size, SharedMemoryFactory* shared_memory_factory) { - return Create(DesktopRect::MakeSize(size), shared_memory_factory); -} - -// static -std::unique_ptr SharedMemoryDesktopFrame::Create( - DesktopRect rect, - SharedMemoryFactory* shared_memory_factory) { RTC_DCHECK(shared_memory_factory); - size_t buffer_size = rect.height() * rect.width() * kBytesPerPixel; + size_t buffer_size = size.height() * size.width() * kBytesPerPixel; std::unique_ptr shared_memory = shared_memory_factory->CreateSharedMemory(buffer_size); if (!shared_memory) return nullptr; return rtc::MakeUnique( - rect, rect.width() * kBytesPerPixel, std::move(shared_memory)); + size, size.width() * kBytesPerPixel, std::move(shared_memory)); } SharedMemoryDesktopFrame::SharedMemoryDesktopFrame(DesktopSize size, int stride, SharedMemory* shared_memory) - : SharedMemoryDesktopFrame(DesktopRect::MakeSize(size), - stride, - shared_memory) {} - -SharedMemoryDesktopFrame::SharedMemoryDesktopFrame( - DesktopRect rect, - int stride, - std::unique_ptr shared_memory) - : SharedMemoryDesktopFrame(rect, - stride, - shared_memory.release()) {} - -SharedMemoryDesktopFrame::SharedMemoryDesktopFrame( - DesktopRect rect, - int stride, - SharedMemory* shared_memory) - : DesktopFrame(rect, + : DesktopFrame(size, stride, reinterpret_cast(shared_memory->data()), shared_memory) {} +SharedMemoryDesktopFrame::SharedMemoryDesktopFrame( + DesktopSize size, + int stride, + std::unique_ptr shared_memory) + : SharedMemoryDesktopFrame(size, + stride, + shared_memory.release()) {} + SharedMemoryDesktopFrame::~SharedMemoryDesktopFrame() { delete shared_memory_; } diff --git a/webrtc/modules/desktop_capture/desktop_frame.h b/webrtc/modules/desktop_capture/desktop_frame.h index 556a35d5e6..cb1b5973eb 100644 --- a/webrtc/modules/desktop_capture/desktop_frame.h +++ b/webrtc/modules/desktop_capture/desktop_frame.h @@ -31,14 +31,12 @@ class DesktopFrame { virtual ~DesktopFrame(); // Size of the frame. - DesktopSize size() const { return rect_.size(); } + const DesktopSize& size() const { return size_; } // The top-left of the frame in full desktop coordinates. E.g. the top left // monitor should start from (0, 0). - DesktopVector top_left() const { return rect_.top_left(); } - - // Rectangle covered by the frame. - const DesktopRect& rect() const { return rect_; } + const DesktopVector& top_left() const { return top_left_; } + void set_top_left(const DesktopVector& top_left) { top_left_ = top_left; } // Distance in the buffer between two neighboring rows in bytes. int stride() const { return stride_; } @@ -84,18 +82,26 @@ class DesktopFrame { capturer_id_ = capturer_id; } - protected: - // Deprecated, use the constructor below. - // TODO(zijiehe): Remove deprecated constructors. DesktopFrame should describe - // its own position in the system. See - // https://bugs.chromium.org/p/webrtc/issues/detail?id=7950 - DesktopFrame(DesktopSize size, - int stride, - uint8_t* data, - SharedMemory* shared_memory); + // Copies various information from |other|. Anything initialized in + // constructor are not copied. + // This function is usually used when sharing a source DesktopFrame with + // several clients: the original DesktopFrame should be kept unchanged. For + // example, BasicDesktopFrame::CopyOf() and SharedDesktopFrame::Share(). + void CopyFrameInfoFrom(const DesktopFrame& other); - // Preferred. - DesktopFrame(DesktopRect rect, + // Copies various information from |other|. Anything initialized in + // constructor are not copied. Not like CopyFrameInfoFrom() function, this + // function uses swap or move constructor to avoid data copy. It won't break + // the |other|, but some of its information may be missing after this + // operation. E.g. other->updated_region_; + // This function is usually used when wrapping a DesktopFrame: the wrapper + // instance takes the ownership of |other|, so other components cannot access + // |other| anymore. For example, CroppedDesktopFrame and + // DesktopFrameWithCursor. + void MoveFrameInfoFrom(DesktopFrame* other); + + protected: + DesktopFrame(DesktopSize size, int stride, uint8_t* data, SharedMemory* shared_memory); @@ -107,10 +113,11 @@ class DesktopFrame { SharedMemory* const shared_memory_; private: - const DesktopRect rect_; + const DesktopSize size_; const int stride_; DesktopRegion updated_region_; + DesktopVector top_left_; DesktopVector dpi_; int64_t capture_time_ms_; uint32_t capturer_id_; @@ -121,15 +128,12 @@ class DesktopFrame { // A DesktopFrame that stores data in the heap. class BasicDesktopFrame : public DesktopFrame { public: - // Deprecated, use the next constructor. explicit BasicDesktopFrame(DesktopSize size); - // Preferred. - explicit BasicDesktopFrame(DesktopRect rect); - ~BasicDesktopFrame() override; // Creates a BasicDesktopFrame that contains copy of |frame|. + // TODO(zijiehe): Return std::unique_ptr static DesktopFrame* CopyOf(const DesktopFrame& frame); private: @@ -142,16 +146,10 @@ class SharedMemoryDesktopFrame : public DesktopFrame { // May return nullptr if |shared_memory_factory| failed to create a // SharedMemory instance. // |shared_memory_factory| should not be nullptr. - // Deprecated, use the next Create() function. static std::unique_ptr Create( DesktopSize size, SharedMemoryFactory* shared_memory_factory); - // Preferred. - static std::unique_ptr Create( - DesktopRect rect, - SharedMemoryFactory* shared_memory_factory); - // Takes ownership of |shared_memory|. // Deprecated, use the next constructor. SharedMemoryDesktopFrame(DesktopSize size, @@ -159,7 +157,7 @@ class SharedMemoryDesktopFrame : public DesktopFrame { SharedMemory* shared_memory); // Preferred. - SharedMemoryDesktopFrame(DesktopRect rect, + SharedMemoryDesktopFrame(DesktopSize size, int stride, std::unique_ptr shared_memory); diff --git a/webrtc/modules/desktop_capture/shared_desktop_frame.cc b/webrtc/modules/desktop_capture/shared_desktop_frame.cc index 42d13d1b82..f80dc88059 100644 --- a/webrtc/modules/desktop_capture/shared_desktop_frame.cc +++ b/webrtc/modules/desktop_capture/shared_desktop_frame.cc @@ -11,8 +11,10 @@ #include "webrtc/modules/desktop_capture/shared_desktop_frame.h" #include +#include #include "webrtc/rtc_base/constructormagic.h" +#include "webrtc/rtc_base/ptr_util.h" namespace webrtc { @@ -33,13 +35,12 @@ DesktopFrame* SharedDesktopFrame::GetUnderlyingFrame() { return core_->get(); } +bool SharedDesktopFrame::ShareFrameWith(const SharedDesktopFrame& other) const { + return core_->get() == other.core_->get(); +} + std::unique_ptr SharedDesktopFrame::Share() { - std::unique_ptr result(new SharedDesktopFrame(core_)); - result->set_dpi(dpi()); - result->set_capture_time_ms(capture_time_ms()); - result->set_capturer_id(capturer_id()); - *result->mutable_updated_region() = updated_region(); - return result; + return std::unique_ptr(new SharedDesktopFrame(core_)); } bool SharedDesktopFrame::IsShared() { @@ -47,10 +48,12 @@ bool SharedDesktopFrame::IsShared() { } SharedDesktopFrame::SharedDesktopFrame(rtc::scoped_refptr core) - : DesktopFrame((*core)->rect(), + : DesktopFrame((*core)->size(), (*core)->stride(), (*core)->data(), (*core)->shared_memory()), - core_(core) {} + core_(core) { + CopyFrameInfoFrom(*(core_->get())); +} } // namespace webrtc diff --git a/webrtc/modules/desktop_capture/shared_desktop_frame.h b/webrtc/modules/desktop_capture/shared_desktop_frame.h index 4b1d7ba04f..6b2fea099d 100644 --- a/webrtc/modules/desktop_capture/shared_desktop_frame.h +++ b/webrtc/modules/desktop_capture/shared_desktop_frame.h @@ -31,9 +31,15 @@ class SharedDesktopFrame : public DesktopFrame { // TODO(sergeyu): remove this method. static SharedDesktopFrame* Wrap(DesktopFrame* desktop_frame); + // Deprecated. Clients do not need to know the underlying DesktopFrame + // instance. + // TODO(zijiehe): Remove this method. // Returns the underlying instance of DesktopFrame. DesktopFrame* GetUnderlyingFrame(); + // Returns whether |this| and |other| share the underlying DesktopFrame. + bool ShareFrameWith(const SharedDesktopFrame& other) const; + // Creates a clone of this object. std::unique_ptr Share(); @@ -46,7 +52,7 @@ class SharedDesktopFrame : public DesktopFrame { SharedDesktopFrame(rtc::scoped_refptr core); - rtc::scoped_refptr core_; + const rtc::scoped_refptr core_; RTC_DISALLOW_COPY_AND_ASSIGN(SharedDesktopFrame); };