Skip to content

Commit

Permalink
VaJDA: a bit of a cleanup
Browse files Browse the repository at this point in the history 
A couple of cleanups I stumbled upon while reading the code:
- struct DecodeRequest can be made internal.
- unique_ptr<> doesn't need to be made const& (strange pattern).
- A few consts and initializing structs with = {} (ISO memset),
 using implicit initialization [1].
- Removed unnecessary |weak_this_|.

All in all, less lines of code :-)

TBR=dcastagna@chromium.org

[1] http://en.cppreference.com/w/c/language/struct_initialization

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: If1a4e781abfa7ae96b85a6d621f5c96d4effb162
Reviewed-on: https://chromium-review.googlesource.com/1100001
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#567839}
  • Loading branch information
@yellowdoge
yellowdoge authored and Commit Bot committed Jun 16, 2018
1 parent 074de24 commit 2c1c1bd
Show file tree
Hide file tree
Showing 3 changed files with 50 additions and 59 deletions.
56 changes: 30 additions & 26 deletions media/gpu/vaapi/vaapi_jpeg_decode_accelerator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,15 +80,21 @@ static unsigned int VaSurfaceFormatForJpeg(

} // namespace

VaapiJpegDecodeAccelerator::DecodeRequest::DecodeRequest(
int32_t bitstream_buffer_id,
std::unique_ptr<UnalignedSharedMemory> shm,
const scoped_refptr<VideoFrame>& video_frame)
: bitstream_buffer_id(bitstream_buffer_id),
shm(std::move(shm)),
video_frame(video_frame) {}

VaapiJpegDecodeAccelerator::DecodeRequest::~DecodeRequest() {}
// An input buffer and the corresponding output video frame awaiting
// consumption, provided by the client.
struct VaapiJpegDecodeAccelerator::DecodeRequest {
DecodeRequest(int32_t bitstream_buffer_id,
std::unique_ptr<UnalignedSharedMemory> shm,
const scoped_refptr<VideoFrame>& video_frame)
: bitstream_buffer_id(bitstream_buffer_id),
shm(std::move(shm)),
video_frame(video_frame) {}
~DecodeRequest() = default;

int32_t bitstream_buffer_id;
std::unique_ptr<UnalignedSharedMemory> shm;
scoped_refptr<VideoFrame> video_frame;
};

void VaapiJpegDecodeAccelerator::NotifyError(int32_t bitstream_buffer_id,
Error error) {
Expand All @@ -102,9 +108,10 @@ void VaapiJpegDecodeAccelerator::NotifyErrorFromDecoderThread(
int32_t bitstream_buffer_id,
Error error) {
DCHECK(decoder_task_runner_->BelongsToCurrentThread());
task_runner_->PostTask(FROM_HERE,
base::Bind(&VaapiJpegDecodeAccelerator::NotifyError,
weak_this_, bitstream_buffer_id, error));
task_runner_->PostTask(
FROM_HERE, base::BindOnce(&VaapiJpegDecodeAccelerator::NotifyError,
weak_this_factory_.GetWeakPtr(),
bitstream_buffer_id, error));
}

void VaapiJpegDecodeAccelerator::VideoFrameReady(int32_t bitstream_buffer_id) {
Expand All @@ -118,9 +125,7 @@ VaapiJpegDecodeAccelerator::VaapiJpegDecodeAccelerator(
io_task_runner_(io_task_runner),
decoder_thread_("VaapiJpegDecoderThread"),
va_surface_id_(VA_INVALID_SURFACE),
weak_this_factory_(this) {
weak_this_ = weak_this_factory_.GetWeakPtr();
}
weak_this_factory_(this) {}

VaapiJpegDecodeAccelerator::~VaapiJpegDecodeAccelerator() {
DCHECK(task_runner_->BelongsToCurrentThread());
Expand Down Expand Up @@ -167,12 +172,9 @@ bool VaapiJpegDecodeAccelerator::OutputPicture(
<< " into video_frame associated with input buffer id "
<< input_buffer_id;

VAImage image;
VAImageFormat format;
const uint32_t kI420Fourcc = VA_FOURCC('I', '4', '2', '0');
memset(&image, 0, sizeof(image));
memset(&format, 0, sizeof(format));
format.fourcc = kI420Fourcc;
VAImage image = {};
VAImageFormat format = {};
format.fourcc = VA_FOURCC_I420;
format.byte_order = VA_LSB_FIRST;
format.bits_per_pixel = 12; // 12 for I420

Expand Down Expand Up @@ -216,14 +218,15 @@ bool VaapiJpegDecodeAccelerator::OutputPicture(
vaapi_wrapper_->ReturnVaImage(&image);

task_runner_->PostTask(
FROM_HERE, base::Bind(&VaapiJpegDecodeAccelerator::VideoFrameReady,
weak_this_, input_buffer_id));
FROM_HERE,
base::BindOnce(&VaapiJpegDecodeAccelerator::VideoFrameReady,
weak_this_factory_.GetWeakPtr(), input_buffer_id));

return true;
}

void VaapiJpegDecodeAccelerator::DecodeTask(
const std::unique_ptr<DecodeRequest>& request) {
std::unique_ptr<DecodeRequest> request) {
DVLOGF(4);
DCHECK(decoder_task_runner_->BelongsToCurrentThread());
TRACE_EVENT0("jpeg", "DecodeTask");
Expand Down Expand Up @@ -314,8 +317,9 @@ void VaapiJpegDecodeAccelerator::Decode(
new DecodeRequest(bitstream_buffer.id(), std::move(shm), video_frame));

decoder_task_runner_->PostTask(
FROM_HERE, base::Bind(&VaapiJpegDecodeAccelerator::DecodeTask,
base::Unretained(this), base::Passed(&request)));
FROM_HERE,
base::BindOnce(&VaapiJpegDecodeAccelerator::DecodeTask,
base::Unretained(this), base::Passed(std::move(request))));
}

bool VaapiJpegDecodeAccelerator::IsSupported() {
Expand Down
33 changes: 9 additions & 24 deletions media/gpu/vaapi/vaapi_jpeg_decode_accelerator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,18 +46,7 @@ class MEDIA_GPU_EXPORT VaapiJpegDecodeAccelerator
bool IsSupported() override;

private:
// An input buffer and the corresponding output video frame awaiting
// consumption, provided by the client.
struct DecodeRequest {
DecodeRequest(int32_t bitstream_buffer_id,
std::unique_ptr<UnalignedSharedMemory> shm,
const scoped_refptr<VideoFrame>& video_frame);
~DecodeRequest();

int32_t bitstream_buffer_id;
std::unique_ptr<UnalignedSharedMemory> shm;
scoped_refptr<VideoFrame> video_frame;
};
struct DecodeRequest;

// Notifies the client that an error has occurred and decoding cannot
// continue.
Expand All @@ -66,7 +55,7 @@ class MEDIA_GPU_EXPORT VaapiJpegDecodeAccelerator
void VideoFrameReady(int32_t bitstream_buffer_id);

// Processes one decode |request|.
void DecodeTask(const std::unique_ptr<DecodeRequest>& request);
void DecodeTask(std::unique_ptr<DecodeRequest> request);

// Puts contents of |va_surface| into given |video_frame|, releases the
// surface and passes the |input_buffer_id| of the resulting picture to
Expand All @@ -76,22 +65,14 @@ class MEDIA_GPU_EXPORT VaapiJpegDecodeAccelerator
const scoped_refptr<VideoFrame>& video_frame);

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

// GPU IO task runner.
scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;
const scoped_refptr<base::SingleThreadTaskRunner> io_task_runner_;

// The client of this class.
Client* client_;

// 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
// object is still alive. As a result, tasks posted from ChildThread to
// decoder thread should use base::Unretained(this), and tasks posted from the
// decoder thread to the ChildThread should use |weak_this_|.
base::WeakPtr<VaapiJpegDecodeAccelerator> weak_this_;

scoped_refptr<VaapiWrapper> vaapi_wrapper_;

// Comes after vaapi_wrapper_ to ensure its destructor is executed before
Expand All @@ -110,7 +91,11 @@ class MEDIA_GPU_EXPORT VaapiJpegDecodeAccelerator
// The VA RT format associated with |va_surface_id_|.
unsigned int va_rt_format_;

// The WeakPtrFactory for |weak_this_|.
// WeakPtr factory for use in posting tasks from |decoder_task_runner_| back
// to |task_runner_|. Since |decoder_thread_| is a fully owned member of
// this class, tasks posted to it may use base::Unretained(this), and tasks
// posted from the |decoder_task_runner_| to |task_runner_| should use a
// WeakPtr (obtained via weak_this_factory_.GetWeakPtr()).
base::WeakPtrFactory<VaapiJpegDecodeAccelerator> weak_this_factory_;

DISALLOW_COPY_AND_ASSIGN(VaapiJpegDecodeAccelerator);
Expand Down
20 changes: 11 additions & 9 deletions media/gpu/vaapi/vaapi_jpeg_decoder.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,41 +189,43 @@ bool VaapiJpegDecoder::Decode(VaapiWrapper* vaapi_wrapper,
VAPictureParameterBufferJPEGBaseline pic_param;
FillPictureParameters(parse_result.frame_header, &pic_param);
if (!vaapi_wrapper->SubmitBuffer(VAPictureParameterBufferType,
sizeof(pic_param), &pic_param))
sizeof(pic_param), &pic_param)) {
return false;
}

// Set quantization table.
VAIQMatrixBufferJPEGBaseline iq_matrix;
FillIQMatrix(parse_result.q_table, &iq_matrix);
if (!vaapi_wrapper->SubmitBuffer(VAIQMatrixBufferType, sizeof(iq_matrix),
&iq_matrix))
&iq_matrix)) {
return false;
}

// Set huffman table.
VAHuffmanTableBufferJPEGBaseline huffman_table;
FillHuffmanTable(parse_result.dc_table, parse_result.ac_table,
&huffman_table);
if (!vaapi_wrapper->SubmitBuffer(VAHuffmanTableBufferType,
sizeof(huffman_table), &huffman_table))
sizeof(huffman_table), &huffman_table)) {
return false;
}

// Set slice parameters.
VASliceParameterBufferJPEGBaseline slice_param;
FillSliceParameters(parse_result, &slice_param);
if (!vaapi_wrapper->SubmitBuffer(VASliceParameterBufferType,
sizeof(slice_param), &slice_param))
sizeof(slice_param), &slice_param)) {
return false;
}

// Set scan data.
if (!vaapi_wrapper->SubmitBuffer(VASliceDataBufferType,
parse_result.data_size,
const_cast<char*>(parse_result.data)))
return false;

if (!vaapi_wrapper->ExecuteAndDestroyPendingBuffers(va_surface))
const_cast<char*>(parse_result.data))) {
return false;
}

return true;
return vaapi_wrapper->ExecuteAndDestroyPendingBuffers(va_surface);
}

} // namespace media

0 comments on commit 2c1c1bd

Please sign in to comment.