Skip to content

Commit

Permalink
Reland "Re-land "Vaapi: Use GUARDED_BY() static annotations on decoder""
Browse files Browse the repository at this point in the history 
This reverts commit cb5feb0.

Bug: 909547
Test: CTS and GTS
Test: VDA unittest with --test_import

Original change's description:
> Revert "Re-land "Vaapi: Use GUARDED_BY() static annotations on decoder""
>
> This reverts commit b8ad3f6.
>
> Reason for revert: This causes inifite-loop in IMPORT mode.
>
> Original change's description:
> > Re-land "Vaapi: Use GUARDED_BY() static annotations on decoder"
> >
> > Deadlocks in the original code are fixed now.
> >
> > TEST=ran VDA tests on Nocturne
> >
> > Bug=909547
> >
> > > Vaapi: Use GUARDED_BY() static annotations on decoder
> > >
> > > This CL introduces GUARDED_BY() static annotation on VaVDA; not all
> > > members variables need to be guarded, so it adds on a number of them
> > > a comment about on which thread it is used.
> >
> > > Bug: 909547
> > > Change-Id: I437a82320795aebc56f77cd1254bb7c8c2f2f5da
> > > Reviewed-on: https://chromium-review.googlesource.com/c/1368590
> > > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > > Commit-Queue: Miguel Casas <mcasas@chromium.org>
> > > Cr-Commit-Position: refs/heads/master@{#616196}
> >
> > Change-Id: If37bfd2d1f24b9b833bf8b291363b5c19861a6c7
> > Reviewed-on: https://chromium-review.googlesource.com/c/1377479
> > Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> > Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#617781}
>
> TBR=mcasas@chromium.org,hiroh@chromium.org,dstaessens@chromium.org
>
> Change-Id: Ic4b1e7e8de8403173f71a415094ed23cab51f5e4
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://chromium-review.googlesource.com/c/1385689
> Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
> Commit-Queue: Hirokazu Honda <hiroh@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#618085}

Change-Id: I96f758b3f99032db219ffce1d432c348e6a30e90
Reviewed-on: https://chromium-review.googlesource.com/c/1385728
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618257}
  • Loading branch information
Hirokazu Honda authored and Commit Bot committed Dec 20, 2018
1 parent c90bb9c commit 5c66ed1
Show file tree
Hide file tree
Showing 3 changed files with 88 additions and 70 deletions.
108 changes: 60 additions & 48 deletions media/gpu/vaapi/vaapi_video_decode_accelerator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -294,9 +294,12 @@ void VaapiVideoDecodeAccelerator::OutputPicture(
PLATFORM_FAILURE, );
}

TRACE_COUNTER_ID2("media,gpu", "Vaapi frames at client", this, "used",
pictures_.size() - available_picture_buffers_.size(),
"available", available_picture_buffers_.size());
{
base::AutoLock auto_lock(lock_);
TRACE_COUNTER_ID2("media,gpu", "Vaapi frames at client", this, "used",
pictures_.size() - available_picture_buffers_.size(),
"available", available_picture_buffers_.size());
}

DVLOGF(4) << "Notifying output picture id " << output_id << " for input "
<< input_id
Expand All @@ -317,8 +320,11 @@ void VaapiVideoDecodeAccelerator::TryOutputPicture() {
if (!client_)
return;

if (pending_output_cbs_.empty() || available_picture_buffers_.empty())
return;
{
base::AutoLock auto_lock(lock_);
if (pending_output_cbs_.empty() || available_picture_buffers_.empty())
return;
}

auto output_cb = std::move(pending_output_cbs_.front());
pending_output_cbs_.pop();
Expand Down Expand Up @@ -535,6 +541,7 @@ void VaapiVideoDecodeAccelerator::TryFinishSurfaceSetChange() {
if (!awaiting_va_surfaces_recycle_)
return;

base::AutoLock auto_lock(lock_);
if (!pending_output_cbs_.empty() ||
pictures_.size() != available_va_surfaces_.size()) {
// Either:
Expand Down Expand Up @@ -567,7 +574,7 @@ void VaapiVideoDecodeAccelerator::TryFinishSurfaceSetChange() {
VLOGF(2) << "Requesting " << requested_num_pics_
<< " pictures of size: " << requested_pic_size_.ToString();

VideoPixelFormat format = GfxBufferFormatToVideoPixelFormat(
const VideoPixelFormat format = GfxBufferFormatToVideoPixelFormat(
vaapi_picture_factory_->GetBufferFormat());
task_runner_->PostTask(
FROM_HERE,
Expand Down Expand Up @@ -702,27 +709,30 @@ void VaapiVideoDecodeAccelerator::ImportBufferForPicture(
return;
}

if (!pictures_.count(picture_buffer_id)) {
CloseGpuMemoryBufferHandle(gpu_memory_buffer_handle);

// It's possible that we've already posted a DismissPictureBuffer for this
// picture, but it has not yet executed when this ImportBufferForPicture
// was posted to us by the client. In that case just ignore this (we've
// already dismissed it and accounted for that).
DVLOGF(3) << "got picture id=" << picture_buffer_id
<< " not in use (anymore?).";
return;
}
{
base::AutoLock auto_lock(lock_);
if (!pictures_.count(picture_buffer_id)) {
CloseGpuMemoryBufferHandle(gpu_memory_buffer_handle);

// It's possible that we've already posted a DismissPictureBuffer for this
// picture, but it has not yet executed when this ImportBufferForPicture
// was posted to us by the client. In that case just ignore this (we've
// already dismissed it and accounted for that).
DVLOGF(3) << "got picture id=" << picture_buffer_id
<< " not in use (anymore?).";
return;
}

VaapiPicture* picture = pictures_[picture_buffer_id].get();
if (!picture->ImportGpuMemoryBufferHandle(
VideoPixelFormatToGfxBufferFormat(pixel_format),
gpu_memory_buffer_handle)) {
// ImportGpuMemoryBufferHandle will close the handles even on failure, so
// we don't need to do this ourselves.
VLOGF(1) << "Failed to import GpuMemoryBufferHandle";
NotifyError(PLATFORM_FAILURE);
return;
VaapiPicture* picture = pictures_[picture_buffer_id].get();
if (!picture->ImportGpuMemoryBufferHandle(
VideoPixelFormatToGfxBufferFormat(pixel_format),
gpu_memory_buffer_handle)) {
// ImportGpuMemoryBufferHandle will close the handles even on failure, so
// we don't need to do this ourselves.
VLOGF(1) << "Failed to import GpuMemoryBufferHandle";
NotifyError(PLATFORM_FAILURE);
return;
}
}

ReusePictureBuffer(picture_buffer_id);
Expand All @@ -736,25 +746,25 @@ void VaapiVideoDecodeAccelerator::ReusePictureBuffer(
TRACE_EVENT1("media,gpu", "VAVDA::ReusePictureBuffer", "Picture id",
picture_buffer_id);

if (!pictures_.count(picture_buffer_id)) {
// It's possible that we've already posted a DismissPictureBuffer for this
// picture, but it has not yet executed when this ReusePictureBuffer
// was posted to us by the client. In that case just ignore this (we've
// already dismissed it and accounted for that).
DVLOGF(3) << "got picture id=" << picture_buffer_id
<< " not in use (anymore?).";
return;
}

{
base::AutoLock auto_lock(lock_);

if (!pictures_.count(picture_buffer_id)) {
// It's possible that we've already posted a DismissPictureBuffer for this
// picture, but it has not yet executed when this ReusePictureBuffer
// was posted to us by the client. In that case just ignore this (we've
// already dismissed it and accounted for that).
DVLOGF(3) << "got picture id=" << picture_buffer_id
<< " not in use (anymore?).";
return;
}

available_picture_buffers_.push_back(picture_buffer_id);
TRACE_COUNTER_ID2("media,gpu", "Vaapi frames at client", this, "used",
pictures_.size() - available_picture_buffers_.size(),
"available", available_picture_buffers_.size());
}

TRACE_COUNTER_ID2("media,gpu", "Vaapi frames at client", this, "used",
pictures_.size() - available_picture_buffers_.size(),
"available", available_picture_buffers_.size());

TryOutputPicture();
}

Expand Down Expand Up @@ -966,10 +976,10 @@ void VaapiVideoDecodeAccelerator::SurfaceReady(
if (state_ == kResetting || state_ == kDestroying)
return;
}

pending_output_cbs_.push(
base::BindOnce(&VaapiVideoDecodeAccelerator::OutputPicture, weak_this_,
dec_surface, bitstream_id, visible_rect, color_space));

TryOutputPicture();
}

Expand Down Expand Up @@ -1017,14 +1027,16 @@ scoped_refptr<VASurface> VaapiVideoDecodeAccelerator::CreateSurface() {
void VaapiVideoDecodeAccelerator::RecycleVASurfaceID(
VASurfaceID va_surface_id) {
DCHECK(task_runner_->BelongsToCurrentThread());
base::AutoLock auto_lock(lock_);
available_va_surfaces_.push_back(va_surface_id);
if (!decode_using_client_picture_buffers_) {
TRACE_COUNTER_ID2("media,gpu", "Vaapi VASurfaceIDs", this, "used",
pictures_.size() - available_va_surfaces_.size(),
"available", available_va_surfaces_.size());
{
base::AutoLock auto_lock(lock_);
available_va_surfaces_.push_back(va_surface_id);
if (!decode_using_client_picture_buffers_) {
TRACE_COUNTER_ID2("media,gpu", "Vaapi VASurfaceIDs", this, "used",
pictures_.size() - available_va_surfaces_.size(),
"available", available_va_surfaces_.size());
}
surfaces_available_.Signal();
}
surfaces_available_.Signal();

TryOutputPicture();
}
Expand Down
49 changes: 27 additions & 22 deletions media/gpu/vaapi/vaapi_video_decode_accelerator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "base/single_thread_task_runner.h"
#include "base/synchronization/condition_variable.h"
#include "base/synchronization/lock.h"
#include "base/thread_annotations.h"
#include "base/threading/thread.h"
#include "base/trace_event/memory_dump_provider.h"
#include "media/base/bitstream_buffer.h"
Expand Down Expand Up @@ -115,16 +116,16 @@ class MEDIA_GPU_EXPORT VaapiVideoDecodeAccelerator
// |decoder_|. This method will sleep if no |input_buffers_| are available.
// Returns true if a new buffer has been set up, false if an early exit has
// been requested (due to initiated reset/flush/destroy).
bool GetCurrInputBuffer_Locked();
bool GetCurrInputBuffer_Locked() EXCLUSIVE_LOCKS_REQUIRED(lock_);

// Signals the client that |curr_input_buffer_| has been read and can be
// returned. Will also release the mapping.
void ReturnCurrInputBuffer_Locked();
void ReturnCurrInputBuffer_Locked() EXCLUSIVE_LOCKS_REQUIRED(lock_);

// Waits for more surfaces to become available. Returns true once they do or
// false if an early exit has been requested (due to an initiated
// reset/flush/destroy).
bool WaitForSurfaces_Locked();
bool WaitForSurfaces_Locked() EXCLUSIVE_LOCKS_REQUIRED(lock_);

// Continue decoding given input buffers and sleep waiting for input/output
// as needed. Will exit if a new set of surfaces or reset/flush/destroy
Expand Down Expand Up @@ -198,24 +199,26 @@ class MEDIA_GPU_EXPORT VaapiVideoDecodeAccelerator
kDestroying,
};

// |lock_| protects |input_buffers_|, |curr_input_buffer_|, |state_| and
// |available_picture_buffers_|.
base::Lock lock_;
State state_;
State state_ GUARDED_BY(lock_);
// Only used on |task_runner_|.
Config::OutputMode output_mode_;

// Queue of available InputBuffers.
base::queue<std::unique_ptr<InputBuffer>> input_buffers_;
base::queue<std::unique_ptr<InputBuffer>> input_buffers_ GUARDED_BY(lock_);
// Signalled when input buffers are queued onto |input_buffers_| queue.
base::ConditionVariable input_ready_;

// Current input buffer at decoder.
// Current input buffer at decoder. Only used on |decoder_thread_task_runner_|
std::unique_ptr<InputBuffer> curr_input_buffer_;

// Only used on |task_runner_|.
std::unique_ptr<VaapiPictureFactory> vaapi_picture_factory_;

// Constructed in Initialize() when the codec information is received.
// The following variables are constructed/initialized in Initialize() when
// the codec information is received. |vaapi_wrapper_| is thread safe.
scoped_refptr<VaapiWrapper> vaapi_wrapper_;
// Only used on |decoder_thread_task_runner_|.
std::unique_ptr<AcceleratedVideoDecoder> decoder_;

// VaapiWrapper for VPP (Video Post Processing). This is used for copying
Expand All @@ -226,14 +229,15 @@ class MEDIA_GPU_EXPORT VaapiVideoDecodeAccelerator
// are allocated at AssignPictureBuffers() and are kept until dtor or
// TryFinishSurfaceSetChange(). Comes after |vaapi_wrapper_| to ensure all
// pictures are destroyed before this is destroyed.
base::small_map<std::map<int32_t, std::unique_ptr<VaapiPicture>>> pictures_;
base::small_map<std::map<int32_t, std::unique_ptr<VaapiPicture>>> pictures_
GUARDED_BY(lock_);
// List of PictureBuffer ids available to be sent to |client_| via
// OutputPicture() (|client_| returns them via ReusePictureBuffer()).
std::list<int32_t> available_picture_buffers_;
std::list<int32_t> available_picture_buffers_ GUARDED_BY(lock_);

// VASurfaceIDs no longer in use that can be passed back to |decoder_| for
// reuse, once it requests them.
std::list<VASurfaceID> available_va_surfaces_;
std::list<VASurfaceID> available_va_surfaces_ GUARDED_BY(lock_);
// Signalled when output surfaces are queued into |available_va_surfaces_|.
base::ConditionVariable surfaces_available_;

Expand All @@ -245,15 +249,13 @@ class MEDIA_GPU_EXPORT VaapiVideoDecodeAccelerator
// If we don't have any available |pictures_| at the time when the decoder
// requests output, we'll store the request in this queue for later and run it
// once the client gives us more textures via ReusePictureBuffer().
// Only used on |task_runner_|.
base::queue<base::OnceClosure> pending_output_cbs_;

// Under some circumstances, we can pass to libva our own VASurfaceIDs to
// decode onto, which skips one copy.
// decode onto, which skips one copy. Only used on |task_runner_|.
bool decode_using_client_picture_buffers_;

// ChildThread's task runner.
const scoped_refptr<base::SingleThreadTaskRunner> task_runner_;

// WeakPtr<> pointing to |this| for use in posting tasks from the decoder
// thread back to the ChildThread. Because the decoder thread is a member of
// this class, any task running on the decoder thread is guaranteed that this
Expand All @@ -262,26 +264,29 @@ class MEDIA_GPU_EXPORT VaapiVideoDecodeAccelerator
// decoder thread to the ChildThread should use |weak_this_|.
base::WeakPtr<VaapiVideoDecodeAccelerator> weak_this_;

// Callback used when creating VASurface objects.
// Callback used when creating VASurface objects. Only used on |task_runner_|.
VASurface::ReleaseCB va_surface_release_cb_;

// To expose client callbacks from VideoDecodeAccelerator.
// NOTE: all calls to these objects *MUST* be executed on task_runner_.
// To expose client callbacks from VideoDecodeAccelerator. Used only on
// |task_runner_|.
std::unique_ptr<base::WeakPtrFactory<Client>> client_ptr_factory_;
base::WeakPtr<Client> client_;

// ChildThread's task runner.
const scoped_refptr<base::SingleThreadTaskRunner> task_runner_;

base::Thread decoder_thread_;
// Use this to post tasks to |decoder_thread_| instead of
// |decoder_thread_.task_runner()| because the latter will be NULL once
// |decoder_thread_.Stop()| returns.
scoped_refptr<base::SingleThreadTaskRunner> decoder_thread_task_runner_;

// Whether we are waiting for any pending_output_cbs_ to be run before
// NotifyingFlushDone.
// Whether we are waiting for any |pending_output_cbs_| to be run before
// NotifyingFlushDone. Only used on |task_runner_|.
bool finish_flush_pending_;

// Decoder requested a new surface set and we are waiting for all the surfaces
// to be returned before we can free them.
// to be returned before we can free them. Only used on |task_runner_|.
bool awaiting_va_surfaces_recycle_;

// Last requested number/resolution of output picture buffers and their
Expand Down
1 change: 1 addition & 0 deletions media/gpu/vaapi/vaapi_video_decode_accelerator_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ class VaapiVideoDecodeAcceleratorTest : public TestWithParam<VideoCodecProfile>,
}

void SetVdaStateToUnitialized() {
base::AutoLock auto_lock(vda_.lock_);
vda_.state_ = VaapiVideoDecodeAccelerator::kUninitialized;
}

Expand Down

0 comments on commit 5c66ed1

Please sign in to comment.