Index: cc/resources/video_resource_updater.cc |
diff --git a/cc/resources/video_resource_updater.cc b/cc/resources/video_resource_updater.cc |
index 6c4f189aaff301863d7dc7c250e86589fa7d269c..542fd369c8cd75ec990698eb59f533d8f8c946ba 100644 |
--- a/cc/resources/video_resource_updater.cc |
+++ b/cc/resources/video_resource_updater.cc |
@@ -173,6 +173,47 @@ VideoResourceUpdater::~VideoResourceUpdater() { |
} |
VideoResourceUpdater::ResourceList::iterator |
+VideoResourceUpdater::RecycleOrAllocateResource( |
+ const gfx::Size& resource_size, |
+ ResourceFormat resource_format, |
+ const gfx::ColorSpace& color_space, |
+ bool software_resource, |
+ bool immutable_hint, |
+ int unique_id, |
+ int plane_index) { |
+ for (auto it = all_resources_.begin(); it != all_resources_.end(); ++it) { |
+ if (it->Matches(unique_id, plane_index)) { |
+ DCHECK(it->resource_size() == resource_size); |
+ DCHECK(it->resource_format() == resource_format); |
+ DCHECK(it->mailbox().IsZero() == software_resource); |
+ return it; |
+ } |
+ } |
+ for (auto it = all_resources_.begin(); it != all_resources_.end(); ++it) { |
+ if (it->resource_size() == resource_size && |
+ it->resource_format() == resource_format && |
+ it->mailbox().IsZero() == software_resource && !it->has_refs()) { |
+ if (software_resource) { |
+ // This extra check is needed because resources backed by SharedMemory |
+ // are not ref-counted, unlike mailboxes. Full discussion in |
+ // codereview.chromium.org/145273021. |
+ if (!resource_provider_->InUseByConsumer(it->resource_id())) |
+ return it; |
+ } else { |
+ if (immutable_hint == |
+ (resource_provider_->GetTextureHint(it->resource_id()) == |
danakj
2016/08/26 18:24:23
doesn't this need to match the immutable hint? In
Tobias Sargeant
2016/08/30 11:31:06
This should only return resources if they have sim
|
+ ResourceProvider::TEXTURE_HINT_IMMUTABLE)) |
+ return it; |
+ } |
+ } |
+ } |
+ |
+ // Otherwise allocate a new resource. |
+ return AllocateResource(resource_size, resource_format, color_space, |
+ !software_resource, immutable_hint); |
+} |
+ |
+VideoResourceUpdater::ResourceList::iterator |
VideoResourceUpdater::AllocateResource(const gfx::Size& plane_size, |
ResourceFormat format, |
const gfx::ColorSpace& color_space, |
@@ -298,15 +339,23 @@ VideoFrameExternalResources VideoResourceUpdater::CreateForSoftwarePlanes( |
ResourceFormat output_resource_format = |
resource_provider_->YuvResourceFormat(bits_per_channel); |
+ // If GPU compositing is enabled, but the output resource format |
+ // returned by the resource provider is RGBA_8888, then a GPU driver |
+ // bug workaround requires that YUV frames must be converted to RGB |
+ // before texture upload. |
+ bool texture_needs_rgb_conversion = |
+ !software_compositor && |
+ output_resource_format == ResourceFormat::RGBA_8888; |
size_t output_plane_count = media::VideoFrame::NumPlanes(input_frame_format); |
// TODO(skaslev): If we're in software compositing mode, we do the YUV -> RGB |
// conversion here. That involves an extra copy of each frame to a bitmap. |
// Obviously, this is suboptimal and should be addressed once ubercompositor |
// starts shaping up. |
- if (software_compositor) { |
+ if (software_compositor || texture_needs_rgb_conversion) { |
output_resource_format = kRGBResourceFormat; |
output_plane_count = 1; |
+ bits_per_channel = 8; |
} |
// Drop recycled resources that are the wrong format. |
@@ -328,40 +377,12 @@ VideoFrameExternalResources VideoResourceUpdater::CreateForSoftwarePlanes( |
break; |
} |
- // Try recycle a previously-allocated resource. |
- 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 (it->Matches(video_frame->unique_id(), 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->has_refs() && !in_use) { |
- // We found a resource with the correct size that we can overwrite. |
- resource_it = it; |
- } |
- } |
- } |
+ const bool is_immutable = true; |
+ ResourceList::iterator resource_it = RecycleOrAllocateResource( |
+ output_plane_resource_size, output_resource_format, |
+ video_frame->ColorSpace(), software_compositor, is_immutable, |
+ video_frame->unique_id(), i); |
- // Check if we need to allocate a new resource. |
- if (resource_it == all_resources_.end()) { |
- const bool is_immutable = true; |
- resource_it = AllocateResource( |
- output_plane_resource_size, output_resource_format, |
- video_frame->ColorSpace(), !software_compositor, is_immutable); |
- } |
if (resource_it == all_resources_.end()) |
break; |
@@ -380,31 +401,55 @@ VideoFrameExternalResources VideoResourceUpdater::CreateForSoftwarePlanes( |
external_resources.bits_per_channel = bits_per_channel; |
- if (software_compositor) { |
+ if (software_compositor || texture_needs_rgb_conversion) { |
DCHECK_EQ(plane_resources.size(), 1u); |
PlaneResource& plane_resource = *plane_resources[0]; |
DCHECK_EQ(plane_resource.resource_format(), kRGBResourceFormat); |
- DCHECK(plane_resource.mailbox().IsZero()); |
+ DCHECK(software_compositor == plane_resource.mailbox().IsZero()); |
danakj
2016/08/26 18:24:23
DCHECK_EQ
Tobias Sargeant
2016/08/30 11:31:06
Done.
|
if (!plane_resource.Matches(video_frame->unique_id(), 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_resource.resource_id()); |
- SkCanvas canvas(lock.sk_bitmap()); |
- // This is software path, so canvas and video_frame are always backed |
- // by software. |
- video_renderer_->Copy(video_frame, &canvas, media::Context3D()); |
+ if (software_compositor) { |
+ if (!video_renderer_) |
+ video_renderer_.reset(new media::SkCanvasVideoRenderer); |
+ |
+ ResourceProvider::ScopedWriteLockSoftware lock( |
+ resource_provider_, plane_resource.resource_id()); |
+ SkCanvas canvas(lock.sk_bitmap()); |
+ // This is software path, so canvas and video_frame are always backed |
+ // by software. |
+ video_renderer_->Copy(video_frame, &canvas, media::Context3D()); |
+ } else { |
+ size_t bytes_per_row = ResourceUtil::CheckedWidthInBytes<size_t>( |
+ video_frame->coded_size().width(), ResourceFormat::RGBA_8888); |
+ size_t needed_size = bytes_per_row * video_frame->coded_size().height(); |
+ if (upload_pixels_.size() < needed_size) |
+ upload_pixels_.resize(needed_size); |
+ |
+ media::SkCanvasVideoRenderer::ConvertVideoFrameToRGBPixels( |
+ video_frame.get(), &upload_pixels_[0], bytes_per_row); |
+ |
+ resource_provider_->CopyToResource(plane_resource.resource_id(), |
+ &upload_pixels_[0], |
+ plane_resource.resource_size()); |
+ } |
plane_resource.SetUniqueId(video_frame->unique_id(), 0); |
} |
- external_resources.software_resources.push_back( |
- plane_resource.resource_id()); |
- external_resources.software_release_callback = |
- base::Bind(&RecycleResource, AsWeakPtr(), plane_resource.resource_id()); |
- external_resources.type = VideoFrameExternalResources::SOFTWARE_RESOURCE; |
+ if (software_compositor) { |
+ external_resources.software_resources.push_back( |
+ plane_resource.resource_id()); |
+ external_resources.type = VideoFrameExternalResources::SOFTWARE_RESOURCE; |
+ } else { |
+ TextureMailbox mailbox(plane_resource.mailbox(), gpu::SyncToken(), |
danakj
2016/08/26 18:24:23
why no SyncToken?
Tobias Sargeant
2016/08/30 11:31:05
This is identical to the mailbox construction on l
danakj
2016/09/02 22:58:15
Oh, I guess this class is relying on it sharing th
Tobias Sargeant
2016/09/12 14:25:25
I've added the comment. Given our discussion, it l
|
+ resource_provider_->GetResourceTextureTarget( |
+ plane_resource.resource_id())); |
+ mailbox.set_color_space(video_frame->ColorSpace()); |
+ external_resources.mailboxes.push_back(mailbox); |
+ external_resources.type = VideoFrameExternalResources::RGBA_RESOURCE; |
+ } |
+ external_resources.release_callbacks.push_back(base::Bind( |
+ &RecycleResource, AsWeakPtr(), plane_resource.resource_id())); |
return external_resources; |
} |
@@ -425,12 +470,12 @@ VideoFrameExternalResources VideoResourceUpdater::CreateForSoftwarePlanes( |
// uploading (including non-frame data to fill in the stride). |
int video_stride_bytes = video_frame->stride(i); |
- size_t bytes_per_row = ResourceUtil::UncheckedWidthInBytes<size_t>( |
+ size_t bytes_per_row = ResourceUtil::CheckedWidthInBytes<size_t>( |
resource_size_pixels.width(), plane_resource.resource_format()); |
// Use 4-byte row alignment (OpenGL default) for upload performance. |
// Assuming that GL_UNPACK_ALIGNMENT has not changed from default. |
size_t upload_image_stride = |
- MathUtil::UncheckedRoundUp<size_t>(bytes_per_row, 4u); |
+ MathUtil::CheckedRoundUp<size_t>(bytes_per_row, 4u); |
bool needs_conversion = false; |
int shift = 0; |
@@ -553,7 +598,7 @@ void VideoResourceUpdater::ReturnTexture( |
// Create a copy of a texture-backed source video frame in a new GL_TEXTURE_2D |
// texture. |
-void VideoResourceUpdater::CopyPlaneTexture( |
+bool VideoResourceUpdater::CopyPlaneTexture( |
media::VideoFrame* video_frame, |
const gpu::MailboxHolder& mailbox_holder, |
VideoFrameExternalResources* external_resources) { |
@@ -565,27 +610,13 @@ void VideoResourceUpdater::CopyPlaneTexture( |
// target to avoid loss of precision or dropping any alpha component. |
const ResourceFormat copy_target_format = ResourceFormat::RGBA_8888; |
- // Search for an existing resource to reuse. |
- VideoResourceUpdater::ResourceList::iterator resource = all_resources_.end(); |
- |
- for (auto it = all_resources_.begin(); it != all_resources_.end(); ++it) { |
- // Reuse resource if attributes match and the resource is a currently |
- // unreferenced texture. |
- if (it->resource_size() == output_plane_resource_size && |
- it->resource_format() == copy_target_format && |
- !it->mailbox().IsZero() && !it->has_refs() && |
- resource_provider_->GetTextureHint(it->resource_id()) != |
- ResourceProvider::TEXTURE_HINT_IMMUTABLE) { |
- resource = it; |
- break; |
- } |
- } |
- |
- // Otherwise allocate a new resource. |
+ const bool is_immutable = false; |
+ VideoResourceUpdater::ResourceList::iterator resource = |
+ RecycleOrAllocateResource(output_plane_resource_size, copy_target_format, |
danakj
2016/08/26 18:24:23
Hm, but if it uses a resource with references on i
Tobias Sargeant
2016/08/30 11:31:06
Passing -1 for the unique_id and plane_number prev
|
+ video_frame->ColorSpace(), false, is_immutable, |
+ -1, -1); |
if (resource == all_resources_.end()) { |
- const bool is_immutable = false; |
- resource = AllocateResource(output_plane_resource_size, copy_target_format, |
- video_frame->ColorSpace(), true, is_immutable); |
+ return false; |
} |
resource->add_ref(); |
@@ -621,6 +652,8 @@ void VideoResourceUpdater::CopyPlaneTexture( |
external_resources->release_callbacks.push_back( |
base::Bind(&RecycleResource, AsWeakPtr(), resource->resource_id())); |
+ |
+ return true; |
} |
VideoFrameExternalResources VideoResourceUpdater::CreateForHardwarePlanes( |
@@ -651,7 +684,10 @@ VideoFrameExternalResources VideoResourceUpdater::CreateForHardwarePlanes( |
if (video_frame->metadata()->IsTrue( |
media::VideoFrameMetadata::COPY_REQUIRED)) { |
- CopyPlaneTexture(video_frame.get(), mailbox_holder, &external_resources); |
+ if (!CopyPlaneTexture(video_frame.get(), mailbox_holder, |
+ &external_resources)) { |
+ return VideoFrameExternalResources(); |
+ } |
} else { |
TextureMailbox mailbox(mailbox_holder.mailbox, mailbox_holder.sync_token, |
mailbox_holder.texture_target, |
@@ -679,7 +715,6 @@ void VideoResourceUpdater::RecycleResource( |
// Resource was already deleted. |
return; |
} |
- |
const ResourceList::iterator resource_it = std::find_if( |
updater->all_resources_.begin(), updater->all_resources_.end(), |
[resource_id](const PlaneResource& plane_resource) { |