Skip to content

Commit

Permalink
media/gpu/v4l2: fix v4l2 VDA/VEA comment on choosing Unretained(this).
Browse files Browse the repository at this point in the history 
v4l2 VDA/VEA use Unretained(this) to create error callback and frame
ready callback for v4l2 IP. It is safe, however, the reason is not
simply because |this| outlives v4l2 IP. It is safe because functions to
execute ErrorCB and FrameReadyCB are bound with V4L2ImageProcessor's
weak pointer. Due to it, they will not be called after
V4L2ImageProcessor is destructed. Since V4L2 VEA/VDA are outlive
V4L2ImageProcessor, base::Unretained(this) is safe for those callbacks.

Also, rename v4l2 IP member: child_task_runner_ to client_task_runner_
because it is not always referred as task runner of child_thread, which
has special meaning (GPU main thread).

Add a type ErrorCB in ImageProcessor interface and express that it is
expected to be set in ImageProcessor subclass's factory method.

BUG=917310
TEST=run VDA test on elm; run VEA test on peach_pit

Change-Id: Ie89f449bc03b2e610429ce6a0cc8a05312145a44
Reviewed-on: https://chromium-review.googlesource.com/c/1390029
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
Commit-Queue: Shuo-Peng Liao <deanliao@google.com>
Cr-Commit-Position: refs/heads/master@{#618903}
  • Loading branch information
Dean Liao authored and Commit Bot committed Dec 26, 2018
1 parent 2112209 commit b98dfa1
Show file tree
Hide file tree
Showing 5 changed files with 87 additions and 54 deletions.
14 changes: 14 additions & 0 deletions media/gpu/image_processor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,22 @@ class ImageProcessor {
// Callback to be used to return the index of a processed image to the
// client. After the client is done with the frame, call Process with the
// index to return the output buffer to the image processor.
// FrameReadyCB shall be executed on the thread that creates ImageProcessor.
// ImageProcessor has to bind its weak pointer to the task to execute
// FrameReadyCB so that the task will not be called after ImageProcessor
// instance is destructed. Note that ImageProcessor client instance should
// have the same lifetime of or outlive ImageProcessor.
using FrameReadyCB = base::OnceCallback<void(scoped_refptr<VideoFrame>)>;

// Callback to be used to notify client when ImageProcess encounters error.
// It should be assigned in subclass's factory method.
// ErrorCB shall be executed on the thread that creates ImageProcessor.
// ImageProcessor has to bind its weak pointer to the task to execute ErrorCB
// so that the task will not be called after ImageProcessor instance is
// destructed. Note that ImageProcessor client instance should have the same
// lifetime of or outlive ImageProcessor.
using ErrorCB = base::RepeatingClosure;

// Called by client to process |frame|. The resulting processed frame will be
// stored in |output_buffer_index| output buffer and notified via |cb|. The
// processor will drop all its references to |frame| after it finishes
Expand Down
55 changes: 27 additions & 28 deletions media/gpu/v4l2/v4l2_image_processor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ V4L2ImageProcessor::V4L2ImageProcessor(
gfx::Size input_visible_size,
gfx::Size output_visible_size,
size_t num_buffers,
const base::Closure& error_cb)
ErrorCB error_cb)
: input_layout_(input_layout),
input_visible_size_(input_visible_size),
input_memory_type_(input_memory_type),
Expand All @@ -84,7 +84,7 @@ V4L2ImageProcessor::V4L2ImageProcessor(
output_memory_type_(output_memory_type),
output_storage_type_(output_storage_type),
output_mode_(output_mode),
child_task_runner_(base::ThreadTaskRunnerHandle::Get()),
client_task_runner_(base::ThreadTaskRunnerHandle::Get()),
device_(device),
device_thread_("V4L2ImageProcessorThread"),
device_poll_thread_("V4L2ImageProcessorDevicePollThread"),
Expand All @@ -99,7 +99,7 @@ V4L2ImageProcessor::V4L2ImageProcessor(
}

V4L2ImageProcessor::~V4L2ImageProcessor() {
DCHECK(child_task_runner_->BelongsToCurrentThread());
DCHECK(client_task_runner_->BelongsToCurrentThread());

Destroy();

Expand All @@ -112,15 +112,14 @@ V4L2ImageProcessor::~V4L2ImageProcessor() {

void V4L2ImageProcessor::NotifyError() {
VLOGF(1);
DCHECK(!child_task_runner_->BelongsToCurrentThread());
child_task_runner_->PostTask(
FROM_HERE, base::BindOnce(&V4L2ImageProcessor::NotifyErrorOnChildThread,
weak_this_, error_cb_));
DCHECK(!client_task_runner_->BelongsToCurrentThread());
client_task_runner_->PostTask(
FROM_HERE, base::BindOnce(&V4L2ImageProcessor::NotifyErrorOnClientThread,
weak_this_));
}

void V4L2ImageProcessor::NotifyErrorOnChildThread(
const base::Closure& error_cb) {
DCHECK(child_task_runner_->BelongsToCurrentThread());
void V4L2ImageProcessor::NotifyErrorOnClientThread() {
DCHECK(client_task_runner_->BelongsToCurrentThread());
error_cb_.Run();
}

Expand All @@ -144,16 +143,16 @@ v4l2_memory InputStorageTypeToV4L2Memory(VideoFrame::StorageType storage_type) {

// static
std::unique_ptr<V4L2ImageProcessor> V4L2ImageProcessor::Create(
scoped_refptr<V4L2Device> device,
VideoFrame::StorageType input_storage_type,
VideoFrame::StorageType output_storage_type,
OutputMode output_mode,
const VideoFrameLayout& input_layout,
const VideoFrameLayout& output_layout,
gfx::Size input_visible_size,
gfx::Size output_visible_size,
size_t num_buffers,
const base::Closure& error_cb) {
scoped_refptr<V4L2Device> device,
VideoFrame::StorageType input_storage_type,
VideoFrame::StorageType output_storage_type,
OutputMode output_mode,
const VideoFrameLayout& input_layout,
const VideoFrameLayout& output_layout,
gfx::Size input_visible_size,
gfx::Size output_visible_size,
size_t num_buffers,
ErrorCB error_cb) {
VLOGF(2);
DCHECK_GT(num_buffers, 0u);
if (!device) {
Expand Down Expand Up @@ -450,7 +449,7 @@ void V4L2ImageProcessor::ProcessTask(std::unique_ptr<JobRecord> job_record) {

bool V4L2ImageProcessor::Reset() {
VLOGF(2);
DCHECK(child_task_runner_->BelongsToCurrentThread());
DCHECK(client_task_runner_->BelongsToCurrentThread());
DCHECK(device_thread_.IsRunning());

weak_this_factory_.InvalidateWeakPtrs();
Expand All @@ -472,7 +471,7 @@ bool V4L2ImageProcessor::Reset() {

void V4L2ImageProcessor::Destroy() {
VLOGF(2);
DCHECK(child_task_runner_->BelongsToCurrentThread());
DCHECK(client_task_runner_->BelongsToCurrentThread());

weak_this_factory_.InvalidateWeakPtrs();

Expand All @@ -491,7 +490,7 @@ void V4L2ImageProcessor::Destroy() {

bool V4L2ImageProcessor::CreateInputBuffers() {
VLOGF(2);
DCHECK(child_task_runner_->BelongsToCurrentThread());
DCHECK(client_task_runner_->BelongsToCurrentThread());
DCHECK(!input_streamon_);

struct v4l2_control control;
Expand Down Expand Up @@ -558,7 +557,7 @@ bool V4L2ImageProcessor::CreateInputBuffers() {

bool V4L2ImageProcessor::CreateOutputBuffers() {
VLOGF(2);
DCHECK(child_task_runner_->BelongsToCurrentThread());
DCHECK(client_task_runner_->BelongsToCurrentThread());
DCHECK(!output_streamon_);

struct v4l2_rect visible_rect;
Expand Down Expand Up @@ -615,7 +614,7 @@ bool V4L2ImageProcessor::CreateOutputBuffers() {

void V4L2ImageProcessor::DestroyInputBuffers() {
VLOGF(2);
DCHECK(child_task_runner_->BelongsToCurrentThread());
DCHECK(client_task_runner_->BelongsToCurrentThread());
DCHECK(!input_streamon_);

struct v4l2_requestbuffers reqbufs;
Expand All @@ -631,7 +630,7 @@ void V4L2ImageProcessor::DestroyInputBuffers() {

void V4L2ImageProcessor::DestroyOutputBuffers() {
VLOGF(2);
DCHECK(child_task_runner_->BelongsToCurrentThread());
DCHECK(client_task_runner_->BelongsToCurrentThread());
DCHECK(!output_streamon_);

output_buffer_map_.clear();
Expand Down Expand Up @@ -807,7 +806,7 @@ void V4L2ImageProcessor::Dequeue() {

DVLOGF(4) << "Processing finished, returning frame, index=" << dqbuf.index;

child_task_runner_->PostTask(
client_task_runner_->PostTask(
FROM_HERE, base::BindOnce(&V4L2ImageProcessor::FrameReady, weak_this_,
std::move(job_record->ready_cb),
job_record->output_frame));
Expand Down Expand Up @@ -985,7 +984,7 @@ void V4L2ImageProcessor::StopDevicePoll() {

void V4L2ImageProcessor::FrameReady(FrameReadyCB cb,
scoped_refptr<VideoFrame> frame) {
DCHECK(child_task_runner_->BelongsToCurrentThread());
DCHECK(client_task_runner_->BelongsToCurrentThread());
std::move(cb).Run(frame);
}

Expand Down
35 changes: 21 additions & 14 deletions media/gpu/v4l2/v4l2_image_processor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ class MEDIA_GPU_EXPORT V4L2ImageProcessor : public ImageProcessor {
// input_format to output_format. Caller shall provide input and output
// storage type as well as output mode. The number of input buffers and output
// buffers will be |num_buffers|. Provided |error_cb| will be posted to the
// child thread if an error occurs after initialization. Returns nullptr if
// V4L2ImageProcessor fails to create.
// same thread Create() is called if an error occurs after initialization.
// Returns nullptr if V4L2ImageProcessor fails to create.
// Note: output_mode will be removed once all its clients use import mode.
static std::unique_ptr<V4L2ImageProcessor> Create(
scoped_refptr<V4L2Device> device,
Expand All @@ -80,7 +80,7 @@ class MEDIA_GPU_EXPORT V4L2ImageProcessor : public ImageProcessor {
gfx::Size input_visible_size,
gfx::Size output_visible_size,
size_t num_buffers,
const base::Closure& error_cb);
ErrorCB error_cb);

private:
// Record for input buffers.
Expand Down Expand Up @@ -130,7 +130,7 @@ class MEDIA_GPU_EXPORT V4L2ImageProcessor : public ImageProcessor {
gfx::Size input_visible_size,
gfx::Size output_visible_size,
size_t num_buffers,
const base::Closure& error_cb);
ErrorCB error_cb);

bool Initialize();
void EnqueueInput();
Expand All @@ -143,8 +143,12 @@ class MEDIA_GPU_EXPORT V4L2ImageProcessor : public ImageProcessor {
void DestroyInputBuffers();
void DestroyOutputBuffers();

// Posts error on |client_task_runner_| thread. This must be called in a
// thread |client_task_runner_| doesn't belong to.
void NotifyError();
void NotifyErrorOnChildThread(const base::Closure& error_cb);
// Invokes ErrorCB given by factory method. This must be called in
// |client_task_runner_|'s thread.
void NotifyErrorOnClientThread();

void ProcessTask(std::unique_ptr<JobRecord> job_record);
void ServiceDeviceTask();
Expand Down Expand Up @@ -176,8 +180,9 @@ class MEDIA_GPU_EXPORT V4L2ImageProcessor : public ImageProcessor {
const VideoFrame::StorageType output_storage_type_;
const OutputMode output_mode_;

// Our original calling task runner for the child thread.
const scoped_refptr<base::SingleThreadTaskRunner> child_task_runner_;
// The task runner which belongs to the thread where V4L2ImageProcessor is
// created.
const scoped_refptr<base::SingleThreadTaskRunner> client_task_runner_;

// V4L2 device in use.
scoped_refptr<V4L2Device> device_;
Expand Down Expand Up @@ -211,17 +216,19 @@ class MEDIA_GPU_EXPORT V4L2ImageProcessor : public ImageProcessor {
const size_t num_buffers_;

// Error callback to the client.
base::Closure error_cb_;
ErrorCB error_cb_;

// WeakPtr<> pointing to |this| for use in posting tasks from the device
// worker threads back to the child thread. Because the worker threads
// are members of this class, any task running on those threads is guaranteed
// that this object is still alive. As a result, tasks posted from the child
// thread to the device thread should use base::Unretained(this),
// and tasks posted the other way should use |weak_this_|.
// worker threads back to the the thread where V4L2ImageProcessor is created.
// Because the worker threads are members of this class, any task running on
// those threads is guaranteed that this object is still alive. As a result,
// tasks posted from |client_task_runner_|'s thread to the device thread
// should use base::Unretained(this), and tasks posted the other way should
// use |weak_this_|.
base::WeakPtr<V4L2ImageProcessor> weak_this_;

// Weak factory for producing weak pointers on the child thread.
// Weak factory for producing weak pointers on the |client_task_runner_|'s
// thread.
base::WeakPtrFactory<V4L2ImageProcessor> weak_this_factory_;

DISALLOW_COPY_AND_ASSIGN(V4L2ImageProcessor);
Expand Down
20 changes: 14 additions & 6 deletions media/gpu/v4l2/v4l2_video_decode_accelerator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2392,14 +2392,17 @@ bool V4L2VideoDecodeAccelerator::CreateImageProcessor() {
return false;
}

// Unretained is safe because |this| owns image processor and there will be
// no callbacks after processor destroys.
// Unretained(this) is safe in creating ErrorCB because |image_processor_|
// is destructed by Destroy(), which posts DestroyTask() to |decoder_thread_|,
// which is the same thread ErrorCB being called. In DestroyTask(), it
// destructs |image_processor_| so no more ErrorCB is invoked after
// DestroyTask() is called.
image_processor_ = V4L2ImageProcessor::Create(
image_processor_device_, VideoFrame::STORAGE_DMABUFS,
VideoFrame::STORAGE_DMABUFS, image_processor_output_mode, *input_layout,
*output_layout, visible_size_, visible_size_, output_buffer_map_.size(),
base::Bind(&V4L2VideoDecodeAccelerator::ImageProcessorError,
base::Unretained(this)));
base::BindRepeating(&V4L2VideoDecodeAccelerator::ImageProcessorError,
base::Unretained(this)));

if (!image_processor_) {
VLOGF(1) << "Initialize image processor failed";
Expand Down Expand Up @@ -2451,8 +2454,13 @@ bool V4L2VideoDecodeAccelerator::ProcessFrame(int32_t bitstream_buffer_id,
if (output_fds.empty())
return false;
}
// Unretained is safe because |this| owns image processor and there will
// be no callbacks after processor destroys.
// Unretained(this) is safe in creating FrameReadyCB because
// |image_processor_| is destructed by Destroy(), which posts DestroyTask() to
// |decoder_thread_|, which is the same thread ErrorCB being called. In
// DestroyTask(), it destructs |image_processor_| so no more ErrorCB is
// invoked after DestroyTask() is called. Unretained is safe because |this|
// owns image processor and there will be no callbacks after processor
// destroys.
image_processor_->Process(
input_frame, output_buffer_index, std::move(output_fds),
base::BindOnce(&V4L2VideoDecodeAccelerator::FrameProcessed,
Expand Down
17 changes: 11 additions & 6 deletions media/gpu/v4l2/v4l2_video_encode_accelerator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,8 +223,10 @@ bool V4L2VideoEncodeAccelerator::Initialize(const Config& config,
// Convert from |config.input_format| to |device_input_layout_->format()|,
// keeping the size at |visible_size_| and requiring the output buffers to
// be of at least |device_input_layout_->coded_size()|.
// Unretained is safe because |this| owns image processor and there will be
// no callbacks after processor destroys.
// Unretained(this) is safe in creating ErrorCB because |image_processor_|
// is destructed by Destroy() and Destroy() is posted in child_task_runner_,
// which is the same thread ErrorCB being called. So after Destroy() is
// called, no more ErrorCB will be invoked.
// |input_storage_type| can be STORAGE_SHMEM and
// STORAGE_MOJO_SHARED_BUFFER. However, it doesn't matter
// VideoFrame::STORAGE_OWNED_MEMORY is specified for |input_storage_type|
Expand All @@ -235,8 +237,8 @@ bool V4L2VideoEncodeAccelerator::Initialize(const Config& config,
VideoFrame::STORAGE_DMABUFS, ImageProcessor::OutputMode::ALLOCATE,
*input_layout, *device_input_layout_, visible_size_, visible_size_,
kImageProcBufferCount,
base::Bind(&V4L2VideoEncodeAccelerator::ImageProcessorError,
base::Unretained(this)));
base::BindRepeating(&V4L2VideoEncodeAccelerator::ImageProcessorError,
base::Unretained(this)));
if (!image_processor_) {
VLOGF(1) << "Failed initializing image processor";
return false;
Expand Down Expand Up @@ -315,8 +317,11 @@ void V4L2VideoEncodeAccelerator::Encode(const scoped_refptr<VideoFrame>& frame,
if (free_image_processor_output_buffers_.size() > 0) {
int output_buffer_index = free_image_processor_output_buffers_.back();
free_image_processor_output_buffers_.pop_back();
// Unretained is safe because |this| owns image processor and there will
// be no callbacks after processor destroys.
// Unretained(this) is safe in creating FrameReadyCB because
// |image_processor_| is destructed by Destroy() and Destroy() is posted
// in child_task_runner_, which is the same thread FrameReadyCB being
// called. So after Destroy() is called, no more FrameReadyCB will be
// invoked.
if (!image_processor_->Process(
frame, output_buffer_index, std::vector<base::ScopedFD>(),
base::BindOnce(&V4L2VideoEncodeAccelerator::FrameProcessed,
Expand Down

0 comments on commit b98dfa1

Please sign in to comment.