From 7a6c2a346104a0269767c7174d09355bd88b752e Mon Sep 17 00:00:00 2001 From: magjed Date: Wed, 10 Dec 2014 17:23:03 -0800 Subject: [PATCH] VideoResourceUpdater: Reuse resources for same frame, even if ResourceProvider has references to it This CL is a follow up to https://codereview.chromium.org/759143003/. The problem with the previous CL is that it doesn't always reuse the resources, because they may still be in use by ResourceProvider. When VideoLayerImpl::DidDraw is called, it will try to free the resources by calling: for (size_t i = 0; i < frame_resources_.size(); ++i) resource_provider->DeleteResource(frame_resources_[i]); ... void ResourceProvider::DeleteResource(ResourceId id) { ... if (resource->exported_count > 0 || resource->lock_for_read_count > 0) { resource->marked_for_deletion = true; return; } else { DeleteResourceInternal(it, Normal); } } It is common that the "marked_for_deletion" path is used, and subsequent calls to VideoLayerImpl::WillDraw may be forced to upload the data again. This CL refactors VideoResourceUpdater so it can reuse the uploaded data in those cases. BUG=437653 Review URL: https://codereview.chromium.org/787723002 Cr-Commit-Position: refs/heads/master@{#307823} --- cc/resources/video_resource_updater.cc | 213 +++++++++++++----------- cc/resources/video_resource_updater.h | 17 +- cc/resources/video_resource_updater_unittest.cc | 50 +++--- 3 files changed, 156 insertions(+), 124 deletions(-) diff --git a/cc/resources/video_resource_updater.cc b/cc/resources/video_resource_updater.cc index 2c9a688619ad..edbea9b53c92 100644 --- a/cc/resources/video_resource_updater.cc +++ b/cc/resources/video_resource_updater.cc @@ -4,6 +4,8 @@ #include "cc/resources/video_resource_updater.h" +#include + #include "base/bind.h" #include "base/debug/trace_event.h" #include "cc/output/gl_renderer.h" @@ -48,6 +50,7 @@ VideoResourceUpdater::PlaneResource::PlaneResource( resource_size(resource_size), resource_format(resource_format), mailbox(mailbox), + ref_count(0), frame_ptr(nullptr), plane_index(0) { } @@ -81,17 +84,43 @@ VideoResourceUpdater::VideoResourceUpdater(ContextProvider* context_provider, } VideoResourceUpdater::~VideoResourceUpdater() { - while (!all_resources_.empty()) { - resource_provider_->DeleteResource(all_resources_.back()); - all_resources_.pop_back(); + for (const PlaneResource& plane_resource : all_resources_) + resource_provider_->DeleteResource(plane_resource.resource_id); +} + +VideoResourceUpdater::ResourceList::iterator +VideoResourceUpdater::AllocateResource(const gfx::Size& plane_size, + ResourceFormat format, + bool has_mailbox) { + // TODO(danakj): Abstract out hw/sw resource create/delete from + // ResourceProvider and stop using ResourceProvider in this class. + const ResourceProvider::ResourceId resource_id = + resource_provider_->CreateResource(plane_size, GL_CLAMP_TO_EDGE, + ResourceProvider::TextureHintImmutable, + format); + if (resource_id == 0) + return all_resources_.end(); + + gpu::Mailbox mailbox; + if (has_mailbox) { + DCHECK(context_provider_); + + gpu::gles2::GLES2Interface* gl = context_provider_->ContextGL(); + + GLC(gl, gl->GenMailboxCHROMIUM(mailbox.name)); + ResourceProvider::ScopedWriteLockGL lock(resource_provider_, resource_id); + GLC(gl, gl->ProduceTextureDirectCHROMIUM(lock.texture_id(), GL_TEXTURE_2D, + mailbox.name)); } + all_resources_.push_front( + PlaneResource(resource_id, plane_size, format, mailbox)); + return all_resources_.begin(); } -void VideoResourceUpdater::DeleteResource(unsigned resource_id) { - resource_provider_->DeleteResource(resource_id); - all_resources_.erase(std::remove(all_resources_.begin(), - all_resources_.end(), - resource_id)); +void VideoResourceUpdater::DeleteResource(ResourceList::iterator resource_it) { + DCHECK_EQ(resource_it->ref_count, 0); + resource_provider_->DeleteResource(resource_it->resource_id); + all_resources_.erase(resource_it); } VideoFrameExternalResources VideoResourceUpdater:: @@ -157,19 +186,15 @@ VideoFrameExternalResources VideoResourceUpdater::CreateForSoftwarePlanes( #endif // defined(VIDEO_HOLE) // Only YUV software video frames are supported. - DCHECK(input_frame_format == media::VideoFrame::YV12 || - input_frame_format == media::VideoFrame::I420 || - input_frame_format == media::VideoFrame::YV12A || - input_frame_format == media::VideoFrame::YV12J || - input_frame_format == media::VideoFrame::YV16 || - input_frame_format == media::VideoFrame::YV24); if (input_frame_format != media::VideoFrame::YV12 && input_frame_format != media::VideoFrame::I420 && input_frame_format != media::VideoFrame::YV12A && input_frame_format != media::VideoFrame::YV12J && input_frame_format != media::VideoFrame::YV16 && - input_frame_format != media::VideoFrame::YV24) + input_frame_format != media::VideoFrame::YV24) { + NOTREACHED() << input_frame_format; return VideoFrameExternalResources(); + } bool software_compositor = context_provider_ == NULL; @@ -186,81 +211,69 @@ VideoFrameExternalResources VideoResourceUpdater::CreateForSoftwarePlanes( output_plane_count = 1; } - int max_resource_size = resource_provider_->max_texture_size(); - std::vector plane_resources; - bool allocation_success = true; + // Drop recycled resources that are the wrong format. + for (auto it = all_resources_.begin(); it != all_resources_.end();) { + if (it->ref_count == 0 && it->resource_format != output_resource_format) + DeleteResource(it++); + else + ++it; + } + const int max_resource_size = resource_provider_->max_texture_size(); + std::vector plane_resources; for (size_t i = 0; i < output_plane_count; ++i) { gfx::Size output_plane_resource_size = SoftwarePlaneDimension(video_frame, software_compositor, i); if (output_plane_resource_size.IsEmpty() || output_plane_resource_size.width() > max_resource_size || output_plane_resource_size.height() > max_resource_size) { - allocation_success = false; break; } // Try recycle a previously-allocated resource. - auto recycled_it = recycled_resources_.end(); - for (auto it = recycled_resources_.begin(); it != recycled_resources_.end(); - ++it) { - const bool resource_matches = - it->resource_format == output_resource_format && - it->resource_size == output_plane_resource_size; - const bool in_use = software_compositor && - resource_provider_->InUseByConsumer(it->resource_id); - if (resource_matches && !in_use) { - // We found a recycled resource with the allocation size and format we - // are looking for. - recycled_it = it; - // Keep looking for a recycled resource that also contains the data we - // are planning to put in it. - if (PlaneResourceMatchesUniqueID(*it, video_frame.get(), i)) + ResourceList::iterator resource_it = all_resources_.end(); + for (auto it = all_resources_.begin(); it != all_resources_.end(); ++it) { + if (it->resource_size == output_plane_resource_size && + it->resource_format == output_resource_format) { + if (PlaneResourceMatchesUniqueID(*it, video_frame.get(), i)) { + // Bingo, we found a resource that already contains the data we are + // planning to put in it. It's safe to reuse it even if + // resource_provider_ holds some references to it, because those + // references are read-only. + resource_it = it; break; + } + + // This extra check is needed because resources backed by SharedMemory + // are not ref-counted, unlike mailboxes. Full discussion in + // codereview.chromium.org/145273021. + const bool in_use = + software_compositor && + resource_provider_->InUseByConsumer(it->resource_id); + if (it->ref_count == 0 && !in_use) { + // We found a resource with the correct size that we can overwrite. + resource_it = it; + } } } - // Check if we can avoid allocating a new resource. - if (recycled_it != recycled_resources_.end()) { - plane_resources.push_back(*recycled_it); - recycled_resources_.erase(recycled_it); - continue; + // Check if we need to allocate a new resource. + if (resource_it == all_resources_.end()) { + resource_it = + AllocateResource(output_plane_resource_size, output_resource_format, + !software_compositor); } - - // TODO(danakj): Abstract out hw/sw resource create/delete from - // ResourceProvider and stop using ResourceProvider in this class. - const ResourceProvider::ResourceId resource_id = - resource_provider_->CreateResource( - output_plane_resource_size, GL_CLAMP_TO_EDGE, - ResourceProvider::TextureHintImmutable, output_resource_format); - if (resource_id == 0) { - allocation_success = false; + if (resource_it == all_resources_.end()) break; - } - all_resources_.push_back(resource_id); - - gpu::Mailbox mailbox; - if (!software_compositor) { - DCHECK(context_provider_); - gpu::gles2::GLES2Interface* gl = context_provider_->ContextGL(); - - GLC(gl, gl->GenMailboxCHROMIUM(mailbox.name)); - ResourceProvider::ScopedWriteLockGL lock(resource_provider_, resource_id); - GLC(gl, gl->ProduceTextureDirectCHROMIUM(lock.texture_id(), GL_TEXTURE_2D, - mailbox.name)); - } - - DCHECK(software_compositor || !mailbox.IsZero()); - plane_resources.push_back(PlaneResource(resource_id, - output_plane_resource_size, - output_resource_format, - mailbox)); + ++resource_it->ref_count; + plane_resources.push_back(resource_it); } - if (!allocation_success) { - for (size_t i = 0; i < plane_resources.size(); ++i) - DeleteResource(plane_resources[i].resource_id); + if (plane_resources.size() != output_plane_count) { + // Allocation failed, nothing will be returned so restore reference counts. + for (ResourceList::iterator resource_it : plane_resources) + --resource_it->ref_count; return VideoFrameExternalResources(); } @@ -268,54 +281,52 @@ VideoFrameExternalResources VideoResourceUpdater::CreateForSoftwarePlanes( if (software_compositor) { DCHECK_EQ(plane_resources.size(), 1u); - DCHECK_EQ(plane_resources[0].resource_format, kRGBResourceFormat); - DCHECK(plane_resources[0].mailbox.IsZero()); + PlaneResource& plane_resource = *plane_resources[0]; + DCHECK_EQ(plane_resource.resource_format, kRGBResourceFormat); + DCHECK(plane_resource.mailbox.IsZero()); - if (!PlaneResourceMatchesUniqueID(plane_resources[0], video_frame.get(), - 0)) { + if (!PlaneResourceMatchesUniqueID(plane_resource, video_frame.get(), 0)) { // We need to transfer data from |video_frame| to the plane resource. if (!video_renderer_) video_renderer_.reset(new media::SkCanvasVideoRenderer); ResourceProvider::ScopedWriteLockSoftware lock( - resource_provider_, plane_resources[0].resource_id); + resource_provider_, plane_resource.resource_id); SkCanvas canvas(lock.sk_bitmap()); video_renderer_->Copy(video_frame, &canvas); - SetPlaneResourceUniqueId(video_frame.get(), 0, &plane_resources[0]); + SetPlaneResourceUniqueId(video_frame.get(), 0, &plane_resource); } - external_resources.software_resources.push_back( - plane_resources[0].resource_id); + external_resources.software_resources.push_back(plane_resource.resource_id); external_resources.software_release_callback = - base::Bind(&RecycleResource, AsWeakPtr(), plane_resources[0]); + base::Bind(&RecycleResource, AsWeakPtr(), plane_resource.resource_id); external_resources.type = VideoFrameExternalResources::SOFTWARE_RESOURCE; - return external_resources; } for (size_t i = 0; i < plane_resources.size(); ++i) { + PlaneResource& plane_resource = *plane_resources[i]; // Update each plane's resource id with its content. - DCHECK_EQ(plane_resources[i].resource_format, + DCHECK_EQ(plane_resource.resource_format, resource_provider_->yuv_resource_format()); - if (!PlaneResourceMatchesUniqueID(plane_resources[i], video_frame.get(), - i)) { + if (!PlaneResourceMatchesUniqueID(plane_resource, video_frame.get(), i)) { // We need to transfer data from |video_frame| to the plane resource. const uint8_t* input_plane_pixels = video_frame->data(i); gfx::Rect image_rect(0, 0, video_frame->stride(i), - plane_resources[i].resource_size.height()); - gfx::Rect source_rect(plane_resources[i].resource_size); - resource_provider_->SetPixels(plane_resources[i].resource_id, + plane_resource.resource_size.height()); + gfx::Rect source_rect(plane_resource.resource_size); + resource_provider_->SetPixels(plane_resource.resource_id, input_plane_pixels, image_rect, source_rect, gfx::Vector2d()); - SetPlaneResourceUniqueId(video_frame.get(), i, &plane_resources[i]); + SetPlaneResourceUniqueId(video_frame.get(), i, &plane_resource); } external_resources.mailboxes.push_back( - TextureMailbox(plane_resources[i].mailbox, GL_TEXTURE_2D, 0)); + TextureMailbox(plane_resource.mailbox, GL_TEXTURE_2D, 0)); external_resources.release_callbacks.push_back( - base::Bind(&RecycleResource, AsWeakPtr(), plane_resources[i])); + base::Bind(&RecycleResource, AsWeakPtr(), plane_resource.resource_id)); } external_resources.type = VideoFrameExternalResources::YUV_RESOURCE; @@ -382,7 +393,7 @@ VideoFrameExternalResources VideoResourceUpdater::CreateForHardwarePlanes( // static void VideoResourceUpdater::RecycleResource( base::WeakPtr updater, - PlaneResource data, + ResourceProvider::ResourceId resource_id, uint32 sync_point, bool lost_resource, BlockingTaskRunner* main_thread_task_runner) { @@ -391,6 +402,14 @@ void VideoResourceUpdater::RecycleResource( return; } + const ResourceList::iterator resource_it = std::find_if( + updater->all_resources_.begin(), updater->all_resources_.end(), + [resource_id](const PlaneResource& plane_resource) { + return plane_resource.resource_id == resource_id; + }); + if (resource_it == updater->all_resources_.end()) + return; + ContextProvider* context_provider = updater->context_provider_; if (context_provider && sync_point) { GLC(context_provider->ContextGL(), @@ -398,19 +417,13 @@ void VideoResourceUpdater::RecycleResource( } if (lost_resource) { - updater->DeleteResource(data.resource_id); + resource_it->ref_count = 0; + updater->DeleteResource(resource_it); return; } - // Drop recycled resources that are the wrong format. - while (!updater->recycled_resources_.empty() && - updater->recycled_resources_.back().resource_format != - data.resource_format) { - updater->DeleteResource(updater->recycled_resources_.back().resource_id); - updater->recycled_resources_.pop_back(); - } - - updater->recycled_resources_.push_back(data); + --resource_it->ref_count; + DCHECK_GE(resource_it->ref_count, 0); } } // namespace cc diff --git a/cc/resources/video_resource_updater.h b/cc/resources/video_resource_updater.h index 38f603312867..622fe68d7c63 100644 --- a/cc/resources/video_resource_updater.h +++ b/cc/resources/video_resource_updater.h @@ -5,6 +5,7 @@ #ifndef CC_RESOURCES_VIDEO_RESOURCE_UPDATER_H_ #define CC_RESOURCES_VIDEO_RESOURCE_UPDATER_H_ +#include #include #include "base/basictypes.h" @@ -78,6 +79,9 @@ class CC_EXPORT VideoResourceUpdater gfx::Size resource_size; ResourceFormat resource_format; gpu::Mailbox mailbox; + // The balance between the number of times this resource has been returned + // from CreateForSoftwarePlanes vs released in RecycleResource. + int ref_count; // These last three members will be used for identifying the data stored in // this resource, and uniquely identifies a media::VideoFrame plane. The // frame pointer will only be used for pointer comparison, i.e. the @@ -100,7 +104,13 @@ class CC_EXPORT VideoResourceUpdater int plane_index, PlaneResource* plane_resource); - void DeleteResource(unsigned resource_id); + // This needs to be a container where iterators can be erased without + // invalidating other iterators. + typedef std::list ResourceList; + ResourceList::iterator AllocateResource(const gfx::Size& plane_size, + ResourceFormat format, + bool has_mailbox); + void DeleteResource(ResourceList::iterator resource_it); bool VerifyFrame(const scoped_refptr& video_frame); VideoFrameExternalResources CreateForHardwarePlanes( const scoped_refptr& video_frame); @@ -108,7 +118,7 @@ class CC_EXPORT VideoResourceUpdater const scoped_refptr& video_frame); static void RecycleResource(base::WeakPtr updater, - PlaneResource data, + unsigned resource_id, uint32 sync_point, bool lost_resource, BlockingTaskRunner* main_thread_task_runner); @@ -122,10 +132,9 @@ class CC_EXPORT VideoResourceUpdater ResourceProvider* resource_provider_; scoped_ptr video_renderer_; - std::vector all_resources_; // Recycle resources so that we can reduce the number of allocations and // data transfers. - std::vector recycled_resources_; + ResourceList all_resources_; DISALLOW_COPY_AND_ASSIGN(VideoResourceUpdater); }; diff --git a/cc/resources/video_resource_updater_unittest.cc b/cc/resources/video_resource_updater_unittest.cc index 1e3481bb3b23..5433bcf7d842 100644 --- a/cc/resources/video_resource_updater_unittest.cc +++ b/cc/resources/video_resource_updater_unittest.cc @@ -115,26 +115,36 @@ TEST_F(VideoResourceUpdaterTest, ReuseResource) { // Expect exactly three texture uploads, one for each plane. EXPECT_EQ(3, context3d_->UploadCount()); - const ResourceProvider::ResourceId y_resource = - resource_provider3d_->CreateResourceFromTextureMailbox( - resources.mailboxes[media::VideoFrame::kYPlane], - SingleReleaseCallbackImpl::Create( - resources.release_callbacks[media::VideoFrame::kYPlane])); - const ResourceProvider::ResourceId u_resource = - resource_provider3d_->CreateResourceFromTextureMailbox( - resources.mailboxes[media::VideoFrame::kUPlane], - SingleReleaseCallbackImpl::Create( - resources.release_callbacks[media::VideoFrame::kUPlane])); - const ResourceProvider::ResourceId v_resource = - resource_provider3d_->CreateResourceFromTextureMailbox( - resources.mailboxes[media::VideoFrame::kVPlane], - SingleReleaseCallbackImpl::Create( - resources.release_callbacks[media::VideoFrame::kVPlane])); - - // Delete the resources. - resource_provider3d_->DeleteResource(y_resource); - resource_provider3d_->DeleteResource(u_resource); - resource_provider3d_->DeleteResource(v_resource); + // Simulate the ResourceProvider releasing the resources back to the video + // updater. + for (ReleaseCallbackImpl& release_callback : resources.release_callbacks) + release_callback.Run(0, false, nullptr); + + // Allocate resources for the same frame. + context3d_->ResetUploadCount(); + resources = updater.CreateExternalResourcesFromVideoFrame(video_frame); + EXPECT_EQ(VideoFrameExternalResources::YUV_RESOURCE, resources.type); + EXPECT_EQ(size_t(3), resources.mailboxes.size()); + EXPECT_EQ(size_t(3), resources.release_callbacks.size()); + // The data should be reused so expect no texture uploads. + EXPECT_EQ(0, context3d_->UploadCount()); +} + +TEST_F(VideoResourceUpdaterTest, ReuseResourceNoDelete) { + VideoResourceUpdater updater(output_surface3d_->context_provider(), + resource_provider3d_.get()); + scoped_refptr video_frame = CreateTestYUVVideoFrame(); + video_frame->set_timestamp(base::TimeDelta::FromSeconds(1234)); + + // Allocate the resources for a YUV video frame. + context3d_->ResetUploadCount(); + VideoFrameExternalResources resources = + updater.CreateExternalResourcesFromVideoFrame(video_frame); + EXPECT_EQ(VideoFrameExternalResources::YUV_RESOURCE, resources.type); + EXPECT_EQ(size_t(3), resources.mailboxes.size()); + EXPECT_EQ(size_t(3), resources.release_callbacks.size()); + // Expect exactly three texture uploads, one for each plane. + EXPECT_EQ(3, context3d_->UploadCount()); // Allocate resources for the same frame. context3d_->ResetUploadCount(); -- 2.11.4.GIT