Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
333253002:
Add VaapiVideoEncodeAccelerator for HW-accelerated video encode. (Closed)
|
Created:
6 years, 6 months ago by Pawel Osciak Modified:
6 years, 6 months ago Reviewers:
xhwang, acolwell GONE FROM CHROMIUM, piman, Jorge Lucangeli Obes, wuchengli, jln (very slow on Chromium), Ilya Sherman CC:
chromium-reviews, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, asvitkine+watch_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAdd VaapiVideoEncodeAccelerator for HW-accelerated video encode.
Add an implementation of VideoEncodeAccelerator utilizing VA-API for
hardware encode on Intel-based ChromeOS platforms.
BUG=378962
TEST=video_encode_accelerator_unittest
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279650
Patch Set 1 : #
Total comments: 93
Patch Set 2 : #
Total comments: 18
Patch Set 3 : typos #Patch Set 4 : #
Total comments: 1
Patch Set 5 : #Patch Set 6 : update pre-sandbox hook to include accelerated encode #
Total comments: 24
Patch Set 7 : #
Total comments: 12
Patch Set 8 : move uma enum to cc #Patch Set 9 : rebase #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : compile fixes #Messages
Total messages: 32 (0 generated)
I just started the review. This will take some time. I'll continue tomorrow morning. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:730: // Due to crbug.com/345569, we we don't distinguish between constrained remove extra we https://codereview.chromium.org/333253002/diff/20001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/333253002/diff/20001/media/media.gyp#newcode663 media/media.gyp:663: ['target_arch != "arm" and chromeos == 1 and use_x11 == 1', { 'target_arch=="ia32" or target_arch=="x64"'? What if chromeos has arm64 or mips in the future? What is this related to use_x11?
vaapi_video_encode_accelerator.cc and h264_bitstream_buffer.cc are not reviewed yet. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... File content/common/gpu/media/va.sigs (right): https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/va.sigs:27: VAStatus vaQueryConfigEntrypoints (VADisplay dpy, VAProfile profile, VAEntrypoint *entrypoint_list, int *num_entrypoints); remove extra space https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:50: {VAConfigAttribRTFormat, VA_RT_FORMAT_YUV420}, I'm curious. What does RT stand for? https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:218: DVLOG(1) << "vaQueryConfigEntrypoints returned: " Doesn't this mean something is wrong? LOG(WARNING) or LOG(ERROR)? https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:229: DVLOG(1) << "Unsupported entrypoint"; print |entrypoint|? https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:237: kCommonVAConfigAttribs, This can be inserted by constructor. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:246: std::vector<VAConfigAttrib> attribs = required_attribs; nit: the constructor can take a vector. Not sure which is faster. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:255: if (attribs[i].type != required_attribs[i].type || No need to compare the type according to the function description? https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:256: !(attribs[i].value & required_attribs[i].value)) { Should be (attribs[i].value & required_attribs[i].value) == required_attribs[i].value. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:401: VA_LOG_ON_ERROR(va_res, "vaMapBuffer failed"); Use LOG_VA_ERROR_AND_REPORT and move it to if clause below. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:407: DCHECK(data_ptr); Do we need this? The code will crash in misc_param->type if data_ptr is NULL. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:414: VA_LOG_ON_ERROR(va_res, "vaUnmapBuffer failed"); If unmap fails, do things still work later? https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:452: void VaapiWrapper::DestroyCodedBuffers() { Need auto_lock(va_lock_) because this method is public. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:493: // Does not block and the job is not finished after it returns. "after this returns" seems clearer. I think you mean the job is not finished after this function returns. Using "it" sounds like you're referring to the job. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:500: bool VaapiWrapper::ExecuteAndDestroyPendingBuffers(VASurfaceID va_surface_id) { Do we need to make sure Execute and DestroyPendingBuffers are executed together? There's no auto_lock here. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:536: if (va_res != VA_STATUS_SUCCESS) Can use VA_SUCCESS_OR_RETURN. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:562: if (image.image_id != VA_INVALID_ID) Should this a DCHECK or this can happen? https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:615: bool VaapiWrapper::DownloadAndDestroyCodedBuffer(VABufferID buffer, buffer_id for consistency https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:91: // Execute job in hardware on target |va_surface_id} and destroy pending |va_surface_id} => |va_surface_id| https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:127: bool DownloadAndDestroyCodedBuffer(VABufferID buffer, buffer_id for consistency
Half way through vaapi_video_encode_accelerator.cc. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:7: #include "base/message_loop/message_loop.h" message_loop_proxy.h https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:12: #include "content/common/gpu/media/vaapi_video_encode_accelerator.h" This should be the first include. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:59: profile.max_resolution.SetSize(1920, 1088); define these as constants https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:102: weak_this_ = weak_this_ptr_factory_.GetWeakPtr(); Move all these to constructor initialization list. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:104: max_ref_idx_l0_size_ = kMaxNumReferenceFrames; max_ref_idx_l0_size_ doesn't change. Just use kMaxNumReferenceFrames? Same for below variables. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:138: DVLOGF(1) << "Unsupported output profile"; print |output_profile| https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:143: DVLOGF(1) << "Unsupported input format"; print |format| https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:154: coded_size_ = gfx::Size(mb_width_ * 16, mb_height_ * 16); Switch the order of coded_size_ and mb_width_ so we can save an operation? https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:163: if (!vaapi_wrapper_.get()) { People were removing get() of scoped_ptr boolean check. https://code.google.com/p/chromium/issues/detail?id=232084 https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:170: return false; Move encoder_thread creation before vaapi_wrapper_ creation so we don't need to worry about any cleanup. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:208: base::Bind(&Client::RequireBitstreamBuffers, Can this run at the end of VaapiVideoEncodeAccelerator::Initialize but before PostTask? https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:538: bool VaapiVideoEncodeAccelerator::PrepareNextJob() { Add DCHECK(encoder_thread_proxy_->BelongsToCurrentThread()); here so it's easier to know what thread it runs on. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:715: } delete |this| here according to API. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:815: bool VaapiVideoEncodeAccelerator::GeneratePackedSPS() { This always return true. Remove bool return value. Same for UpdatePPS and GeneratePackedPPS. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:973: // Only touch state on encoder thread, unless it's not running. This is alloying. Any way to get ride of this? I cannot think of one. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:1004: : coded_buffer(VA_INVALID_ID), keyframe(false) { initialize reference_surfaces? https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_video_encode_accelerator.h (right): https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.h:54: enum { Do other chrome classes also define different types of non-continuous constants in enum? This looks strange. Use static const int? Also chromium style guide says enums use all capital style. http://www.chromium.org/developers/coding-style https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.h:228: // All of the members below may only be accessed on the encoder_thread_, s/may/must/? remove extra "," at the end? https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.h:250: scoped_ptr<EncodeJob> curr_encode_job_; current_encode_jobs_ is easier to understand. Same below. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:29: #include "content/common/gpu/media/v4l2_video_encode_accelerator.h" This should be the first include. Move this before "base/at_exit.h". http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/video_encode_accelerator_unittest.cc:294: ASSERT_EQ(h264_parser_.ParseSliceHeader(nalu, &shdr), The expectation (media::H264Parser::kOk) should be the first argument. Same below.
https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... File content/common/gpu/media/va.sigs (right): https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/va.sigs:27: VAStatus vaQueryConfigEntrypoints (VADisplay dpy, VAProfile profile, VAEntrypoint *entrypoint_list, int *num_entrypoints); On 2014/06/17 14:33:12, wuchengli wrote: > remove extra space Done. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_video_encode_accelerator.cc:7: #include "base/message_loop/message_loop.h" On 2014/06/18 03:34:13, wuchengli wrote: > message_loop_proxy.h Done. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_video_encode_accelerator.cc:12: #include "content/common/gpu/media/vaapi_video_encode_accelerator.h" On 2014/06/18 03:34:12, wuchengli wrote: > This should be the first include. > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... There was a discussion about this and iirc the conclusion was this should be down here, but since the style says this, let's follow it. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_video_encode_accelerator.cc:59: profile.max_resolution.SetSize(1920, 1088); On 2014/06/18 03:34:12, wuchengli wrote: > define these as constants I prefer to leave it here to emphasize that we need to fix this function properly with GPUInfo. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_video_encode_accelerator.cc:102: weak_this_ = weak_this_ptr_factory_.GetWeakPtr(); On 2014/06/18 03:34:13, wuchengli wrote: > Move all these to constructor initialization list. This is intentional. GetWeakPtr() cannot be called before weak_this_ptr_factory is initialized, and the factory must be the last member of the class (see documentation of WeakPtrFactory). https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_video_encode_accelerator.cc:104: max_ref_idx_l0_size_ = kMaxNumReferenceFrames; On 2014/06/18 03:34:13, wuchengli wrote: > max_ref_idx_l0_size_ doesn't change. Just use kMaxNumReferenceFrames? Same for > below variables. That's not the same. Right now it's the same, but if we wanted to use b frames for example, the equation would be different. This is to emphasize that. By keeping those as variables I'm trying to emphasize that we can change them. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_video_encode_accelerator.cc:138: DVLOGF(1) << "Unsupported output profile"; On 2014/06/18 03:34:13, wuchengli wrote: > print |output_profile| Done. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_video_encode_accelerator.cc:143: DVLOGF(1) << "Unsupported input format"; On 2014/06/18 03:34:13, wuchengli wrote: > print |format| Done. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_video_encode_accelerator.cc:154: coded_size_ = gfx::Size(mb_width_ * 16, mb_height_ * 16); On 2014/06/18 03:34:12, wuchengli wrote: > Switch the order of coded_size_ and mb_width_ so we can save an operation? This is intentional. That would involve a cast in order for the cc::RoundUp to instantiate the template properly. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_video_encode_accelerator.cc:163: if (!vaapi_wrapper_.get()) { On 2014/06/18 03:34:13, wuchengli wrote: > People were removing get() of scoped_ptr boolean check. > https://code.google.com/p/chromium/issues/detail?id=232084 Done. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_video_encode_accelerator.cc:170: return false; On 2014/06/18 03:34:13, wuchengli wrote: > Move encoder_thread creation before vaapi_wrapper_ creation so we don't need to > worry about any cleanup. This is intentional and defensive so that I don't have to check for vaapi_wrapper_ anywhere else. Moreover creating a thread is less likely to fail than the wrapper initialization so it goes last. Also, what cleanup do you have in mind? https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_video_encode_accelerator.cc:208: base::Bind(&Client::RequireBitstreamBuffers, On 2014/06/18 03:34:12, wuchengli wrote: > Can this run at the end of VaapiVideoEncodeAccelerator::Initialize but before > PostTask? No, we need to be ready to accept encode requests after this. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_video_encode_accelerator.cc:538: bool VaapiVideoEncodeAccelerator::PrepareNextJob() { On 2014/06/18 03:34:13, wuchengli wrote: > Add DCHECK(encoder_thread_proxy_->BelongsToCurrentThread()); here so it's easier > to know what thread it runs on. The idea is not to do these on methods that are called from tasks. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_video_encode_accelerator.cc:715: } On 2014/06/18 03:34:12, wuchengli wrote: > delete |this| here according to API. Done. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_video_encode_accelerator.cc:730: // Due to crbug.com/345569, we we don't distinguish between constrained On 2014/06/16 15:51:52, wuchengli wrote: > remove extra we Done. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_video_encode_accelerator.cc:815: bool VaapiVideoEncodeAccelerator::GeneratePackedSPS() { On 2014/06/18 03:34:13, wuchengli wrote: > This always return true. Remove bool return value. Same for UpdatePPS and > GeneratePackedPPS. Done. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_video_encode_accelerator.cc:973: // Only touch state on encoder thread, unless it's not running. On 2014/06/18 03:34:13, wuchengli wrote: > This is alloying. Any way to get ride of this? I cannot think of one. This is a very widely used pattern in Chrome. I don't think there is a better way. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_video_encode_accelerator.cc:1004: : coded_buffer(VA_INVALID_ID), keyframe(false) { On 2014/06/18 03:34:12, wuchengli wrote: > initialize reference_surfaces? Initialize with? https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... File content/common/gpu/media/vaapi_video_encode_accelerator.h (right): https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_video_encode_accelerator.h:54: enum { On 2014/06/18 03:34:13, wuchengli wrote: > Do other chrome classes also define different types of non-continuous constants > in enum? This looks strange. Use static const int? Also chromium style guide > says enums use all capital style. > http://www.chromium.org/developers/coding-style There are other examples of this, https://code.google.com/p/chromium/codesearch#chromium/src/media/base/limits.... is one, https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... is another. I can switch to const ints, but it's going to be soooo verbose. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_video_encode_accelerator.h:228: // All of the members below may only be accessed on the encoder_thread_, On 2014/06/18 03:34:13, wuchengli wrote: > s/may/must/? remove extra "," at the end? 'may only' == 'must'. But I can do s/may only/must/ The ',' is intentional. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_video_encode_accelerator.h:250: scoped_ptr<EncodeJob> curr_encode_job_; On 2014/06/18 03:34:13, wuchengli wrote: > current_encode_jobs_ is easier to understand. Same below. Done. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... File content/common/gpu/media/vaapi_wrapper.cc (right): https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_wrapper.cc:50: {VAConfigAttribRTFormat, VA_RT_FORMAT_YUV420}, On 2014/06/17 14:33:12, wuchengli wrote: > I'm curious. What does RT stand for? My guess is raw texture, but I may be wrong. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_wrapper.cc:218: DVLOG(1) << "vaQueryConfigEntrypoints returned: " On 2014/06/17 14:33:12, wuchengli wrote: > Doesn't this mean something is wrong? LOG(WARNING) or LOG(ERROR)? LOG() would also show up in release. This goes back a few years when Ami convinced me we should not have codec errors polluting logs on release. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_wrapper.cc:229: DVLOG(1) << "Unsupported entrypoint"; On 2014/06/17 14:33:12, wuchengli wrote: > print |entrypoint|? Done. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_wrapper.cc:237: kCommonVAConfigAttribs, On 2014/06/17 14:33:12, wuchengli wrote: > This can be inserted by constructor. It'd have to be a member, but we don't really need it after Initialize() returns. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_wrapper.cc:246: std::vector<VAConfigAttrib> attribs = required_attribs; On 2014/06/17 14:33:12, wuchengli wrote: > nit: the constructor can take a vector. Not sure which is faster. In C++ assignment on definition calls the copy constructor. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_wrapper.cc:255: if (attribs[i].type != required_attribs[i].type || On 2014/06/17 14:33:12, wuchengli wrote: > No need to compare the type according to the function description? Yes, but I'd still prefer to keep this. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_wrapper.cc:256: !(attribs[i].value & required_attribs[i].value)) { On 2014/06/17 14:33:12, wuchengli wrote: > Should be (attribs[i].value & required_attribs[i].value) == > required_attribs[i].value. Good catch. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_wrapper.cc:401: VA_LOG_ON_ERROR(va_res, "vaMapBuffer failed"); On 2014/06/17 14:33:12, wuchengli wrote: > Use LOG_VA_ERROR_AND_REPORT and move it to if clause below. While you are right, honestly we have a lot of this in the file already and I preferred to stay in the "convention". https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_wrapper.cc:407: DCHECK(data_ptr); On 2014/06/17 14:33:13, wuchengli wrote: > Do we need this? The code will crash in misc_param->type if data_ptr is NULL. But the crash may not be as obvious. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_wrapper.cc:414: VA_LOG_ON_ERROR(va_res, "vaUnmapBuffer failed"); On 2014/06/17 14:33:12, wuchengli wrote: > If unmap fails, do things still work later? We'll destroy the buffer anyway. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_wrapper.cc:452: void VaapiWrapper::DestroyCodedBuffers() { On 2014/06/17 14:33:12, wuchengli wrote: > Need auto_lock(va_lock_) because this method is public. Oh good catch. I missed it when refactoring. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_wrapper.cc:493: // Does not block and the job is not finished after it returns. On 2014/06/17 14:33:12, wuchengli wrote: > "after this returns" seems clearer. I think you mean the job is not finished > after this function returns. Using "it" sounds like you're referring to the job. Done. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_wrapper.cc:500: bool VaapiWrapper::ExecuteAndDestroyPendingBuffers(VASurfaceID va_surface_id) { On 2014/06/17 14:33:12, wuchengli wrote: > Do we need to make sure Execute and DestroyPendingBuffers are executed together? No. > There's no auto_lock here. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_wrapper.cc:536: if (va_res != VA_STATUS_SUCCESS) On 2014/06/17 14:33:12, wuchengli wrote: > Can use VA_SUCCESS_OR_RETURN. I think this file needs more refactoring of those macros (as per your comment below), but I'd prefer to leave it to another CL. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_wrapper.cc:562: if (image.image_id != VA_INVALID_ID) On 2014/06/17 14:33:12, wuchengli wrote: > Should this a DCHECK or this can happen? This is defensive. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_wrapper.cc:615: bool VaapiWrapper::DownloadAndDestroyCodedBuffer(VABufferID buffer, On 2014/06/17 14:33:12, wuchengli wrote: > buffer_id for consistency Done. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... File content/common/gpu/media/vaapi_wrapper.h (right): https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_wrapper.h:91: // Execute job in hardware on target |va_surface_id} and destroy pending On 2014/06/17 14:33:13, wuchengli wrote: > |va_surface_id} => |va_surface_id| Done. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/vaapi_wrapper.h:127: bool DownloadAndDestroyCodedBuffer(VABufferID buffer, On 2014/06/17 14:33:13, wuchengli wrote: > buffer_id for consistency Done. https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/video_encode_accelerator_unittest.cc:29: #include "content/common/gpu/media/v4l2_video_encode_accelerator.h" On 2014/06/18 03:34:14, wuchengli wrote: > This should be the first include. Move this before "base/at_exit.h". > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Names_and_Orde... "Exception: sometimes, system-specific code needs conditional includes. Such code can put conditional includes after other includes. " https://chromiumcodereview.appspot.com/333253002/diff/20001/content/common/gp... content/common/gpu/media/video_encode_accelerator_unittest.cc:294: ASSERT_EQ(h264_parser_.ParseSliceHeader(nalu, &shdr), On 2014/06/18 03:34:13, wuchengli wrote: > The expectation (media::H264Parser::kOk) should be the first argument. Same > below. Done. https://chromiumcodereview.appspot.com/333253002/diff/20001/media/media.gyp File media/media.gyp (right): https://chromiumcodereview.appspot.com/333253002/diff/20001/media/media.gyp#n... media/media.gyp:663: ['target_arch != "arm" and chromeos == 1 and use_x11 == 1', { On 2014/06/16 15:51:52, wuchengli wrote: > 'target_arch=="ia32" or target_arch=="x64"'? What if chromeos has arm64 or mips > in the future? > What is this related to use_x11? This is for VAVEA, so it uses the same set of rules.
Finished the review of vaapi_video_encode_accelerator.h/cc. I'm continuing to review the rest. Is anyone familiar with H264 that can review this CL? https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:232: if (curr_pic_.frame_num % i_period_ == 0 || force_keyframe) nit: add () around curr_pic_.frame_num % i_period_ == 0 seems easier to read https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:263: ref_pic_list0_.push_front(curr_encode_job_->recon_surface); reconstructed_surface is easier to understand
PTAL
Finally finished the review. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:170: return false; On 2014/06/18 07:05:07, Pawel Osciak wrote: > On 2014/06/18 03:34:13, wuchengli wrote: > > Move encoder_thread creation before vaapi_wrapper_ creation so we don't need > to > > worry about any cleanup. > > This is intentional and defensive so that I don't have to check for > vaapi_wrapper_ anywhere else. > Moreover creating a thread is less likely to fail than the wrapper > initialization so it goes last. > Also, what cleanup do you have in mind? I was thinking about vaapi_wrapper cleanup. I just looked and its destructor will clean up everything. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:208: base::Bind(&Client::RequireBitstreamBuffers, On 2014/06/18 07:05:07, Pawel Osciak wrote: > On 2014/06/18 03:34:12, wuchengli wrote: > > Can this run at the end of VaapiVideoEncodeAccelerator::Initialize but before > > PostTask? > > No, we need to be ready to accept encode requests after this. If we need to be ready to accept encode request after this, SetState(kEncoding) should be done before this. Right? https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:1004: : coded_buffer(VA_INVALID_ID), keyframe(false) { On 2014/06/18 07:05:07, Pawel Osciak wrote: > On 2014/06/18 03:34:12, wuchengli wrote: > > initialize reference_surfaces? > > Initialize with? I misread the code. Ignore this. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_video_encode_accelerator.h (right): https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.h:54: enum { On 2014/06/18 07:05:08, Pawel Osciak wrote: > On 2014/06/18 03:34:13, wuchengli wrote: > > Do other chrome classes also define different types of non-continuous > constants > > in enum? This looks strange. Use static const int? Also chromium style guide > > says enums use all capital style. > > http://www.chromium.org/developers/coding-style > > There are other examples of this, > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/limits.... > is one, > https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu... > is another. > I can switch to const ints, but it's going to be soooo verbose. In C++11, static const int can be defined here and the usage has no restriction. I just tried and static const int in header seemed to work. I also saw other files define static const in header files. Anyway. Let's not spend time here. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.h:228: // All of the members below may only be accessed on the encoder_thread_, On 2014/06/18 07:05:08, Pawel Osciak wrote: > On 2014/06/18 03:34:13, wuchengli wrote: > > s/may/must/? remove extra "," at the end? > > 'may only' == 'must'. You are right. My English sucks... But you added an extra y to must... > > But I can do s/may only/must/ > > The ',' is intentional. https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:199: base::Unretained(this))); Can you add comment to explain why Unretained(this) is safe? All vaapi calls run on encoder thread? va_surface_release_cb_ will be posted from the encoder thread to encoder thread? When is a VASurface destroyed? Is it possible a VASurface is destroyed by "delete this" in Destroy() and trigger a |va_surface_release_cb_|? https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_video_encode_accelerator.h (right): https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.h:298: // to the encoder thread is safe, because this always outlives the encoder s/this/|this|/ https://codereview.chromium.org/333253002/diff/40001/media/filters/h264_bitst... File media/filters/h264_bitstream_buffer.cc (right): https://codereview.chromium.org/333253002/diff/40001/media/filters/h264_bitst... media/filters/h264_bitstream_buffer.cc:36: CHECK(data_) << "Failed growing the buffer"; This is not a programming error and shouldn't use CHECK.
https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:208: base::Bind(&Client::RequireBitstreamBuffers, On 2014/06/18 15:57:43, wuchengli wrote: > On 2014/06/18 07:05:07, Pawel Osciak wrote: > > On 2014/06/18 03:34:12, wuchengli wrote: > > > Can this run at the end of VaapiVideoEncodeAccelerator::Initialize but > before > > > PostTask? > > > > No, we need to be ready to accept encode requests after this. > If we need to be ready to accept encode request after this, SetState(kEncoding) > should be done before this. Right? Does not matter, because the encode task will be run on this thread, so as long as we do this before returning, it's ok. https://codereview.chromium.org/333253002/diff/20001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:263: ref_pic_list0_.push_front(curr_encode_job_->recon_surface); On 2014/06/18 07:32:51, wuchengli wrote: > reconstructed_surface is easier to understand The comment in *.h should be enough. I don't think reconstructed gives enough information without reading *.h anyway. https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:199: base::Unretained(this))); On 2014/06/18 15:57:44, wuchengli wrote: > Can you add comment to explain why Unretained(this) is safe? All vaapi calls run > on encoder thread? va_surface_release_cb_ will be posted from the encoder thread > to encoder thread? When is a VASurface destroyed? > > Is it possible a VASurface is destroyed by "delete this" in Destroy() and > trigger a |va_surface_release_cb_|? This is explained in the header, I'd prefer not to repeat it here. The callback is posted to the encoder_thread_, and "Posting from the ChildThread using base::Unretained to the encoder thread is safe, because |this| always outlives the encoder thread (it's a member of this class).". It does not matter from where a task is posted to a thread, as long as it is posted to/executes on the right thread. As explained in the header, all va resources are owned and destroyed by the vaapi_wrapper_. When delete this is called, the callback cannot be executed anymore, because the encoder thread is Stop()ped. But it doesn't matter, VASurface just stores ints and does not manage the underlying surfaces in any way, it's just to indicate what is in use and what is available for reuse. https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_video_encode_accelerator.h (right): https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.h:298: // to the encoder thread is safe, because this always outlives the encoder On 2014/06/18 15:57:44, wuchengli wrote: > s/this/|this|/ Done. https://codereview.chromium.org/333253002/diff/40001/media/filters/h264_bitst... File media/filters/h264_bitstream_buffer.cc (right): https://codereview.chromium.org/333253002/diff/40001/media/filters/h264_bitst... media/filters/h264_bitstream_buffer.cc:36: CHECK(data_) << "Failed growing the buffer"; On 2014/06/18 15:57:44, wuchengli wrote: > This is not a programming error and shouldn't use CHECK. It actually would be. Allocations in Chrome cannot fail (i.e. we should crash if an allocation fails), by design of our allocator.
I'm going to review the CL again. Here are the comments so far. https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:199: base::Unretained(this))); On 2014/06/19 01:31:11, Pawel Osciak wrote: > On 2014/06/18 15:57:44, wuchengli wrote: > > Can you add comment to explain why Unretained(this) is safe? All vaapi calls > run > > on encoder thread? va_surface_release_cb_ will be posted from the encoder > thread > > to encoder thread? When is a VASurface destroyed? > > > > Is it possible a VASurface is destroyed by "delete this" in Destroy() and > > trigger a |va_surface_release_cb_|? > > This is explained in the header, I'd prefer not to repeat it here. The callback > is posted to the encoder_thread_, and "Posting from the ChildThread using > base::Unretained to the encoder thread is safe, because |this| always outlives > the encoder thread (it's a member of this class).". > > It does not matter from where a task is posted to a thread, as long as it is > posted to/executes on the right thread. > > As explained in the header, all va resources are owned and destroyed by the > vaapi_wrapper_. When delete this is called, the callback cannot be executed > anymore, because the encoder thread is Stop()ped. But it doesn't matter, > VASurface just stores ints and does not manage the underlying surfaces in any > way, it's just to indicate what is in use and what is available for reuse. I don't understand why the release callback is posted from the ChildThread. It looks like VASurface can be destructed from ref_pic_list0_.pop_back() of EndFrame() or from the end of TryToReturnBitstreamBuffer(). Is that right? Please show me the exact location where the destructor of VASurface is called. https://codereview.chromium.org/333253002/diff/40001/media/filters/h264_bitst... File media/filters/h264_bitstream_buffer.cc (right): https://codereview.chromium.org/333253002/diff/40001/media/filters/h264_bitst... media/filters/h264_bitstream_buffer.cc:36: CHECK(data_) << "Failed growing the buffer"; On 2014/06/19 01:31:11, Pawel Osciak wrote: > On 2014/06/18 15:57:44, wuchengli wrote: > > This is not a programming error and shouldn't use CHECK. > > It actually would be. Allocations in Chrome cannot fail (i.e. we should crash if > an allocation fails), by design of our allocator. Interesting. You mean we should CHECK the result of every malloc and realloc to see if allocator has a bug? I find some code CHECK the result and some test if the result is NULL. https://code.google.com/p/chromium/codesearch#chromium/src/base/pickle.cc&q=r... https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/native_clien... https://code.google.com/p/chromium/codesearch#chromium/src/net/base/io_buffer...
https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:199: base::Unretained(this))); On 2014/06/19 04:28:34, wuchengli wrote: > On 2014/06/19 01:31:11, Pawel Osciak wrote: > > On 2014/06/18 15:57:44, wuchengli wrote: > > > Can you add comment to explain why Unretained(this) is safe? All vaapi calls > > run > > > on encoder thread? va_surface_release_cb_ will be posted from the encoder > > thread > > > to encoder thread? When is a VASurface destroyed? > > > > > > Is it possible a VASurface is destroyed by "delete this" in Destroy() and > > > trigger a |va_surface_release_cb_|? > > > > This is explained in the header, I'd prefer not to repeat it here. The > callback > > is posted to the encoder_thread_, and "Posting from the ChildThread using > > base::Unretained to the encoder thread is safe, because |this| always outlives > > the encoder thread (it's a member of this class).". > > > > It does not matter from where a task is posted to a thread, as long as it is > > posted to/executes on the right thread. > > > > As explained in the header, all va resources are owned and destroyed by the > > vaapi_wrapper_. When delete this is called, the callback cannot be executed > > anymore, because the encoder thread is Stop()ped. But it doesn't matter, > > VASurface just stores ints and does not manage the underlying surfaces in any > > way, it's just to indicate what is in use and what is available for reuse. > I don't understand why the release callback is posted from the ChildThread. It > looks like VASurface can be destructed from ref_pic_list0_.pop_back() of > EndFrame() or from the end of TryToReturnBitstreamBuffer(). Is that right? > Please show me the exact location where the destructor of VASurface is called. You misunderstood me, I never said it was posted from ChildThread. I only said it was posted to encoder_thread_. And that it does not matter from which task it is posted, only where it is posted to/where it runs on. It's always run on the encoder_thread_, which will never outlive |this|. The whole beauty of this whole setup that I don't have to worry about this at all for the above reason. And again, the actual resources are managed by the wrapper, which will outlive encoder_thread_ as well. https://codereview.chromium.org/333253002/diff/40001/media/filters/h264_bitst... File media/filters/h264_bitstream_buffer.cc (right): https://codereview.chromium.org/333253002/diff/40001/media/filters/h264_bitst... media/filters/h264_bitstream_buffer.cc:36: CHECK(data_) << "Failed growing the buffer"; On 2014/06/19 04:28:34, wuchengli wrote: > On 2014/06/19 01:31:11, Pawel Osciak wrote: > > On 2014/06/18 15:57:44, wuchengli wrote: > > > This is not a programming error and shouldn't use CHECK. > > > > It actually would be. Allocations in Chrome cannot fail (i.e. we should crash > if > > an allocation fails), by design of our allocator. > Interesting. You mean we should CHECK the result of every malloc and realloc to > see if allocator has a bug? I find some code CHECK the result and some test if > the result is NULL. > No, I'm just overly fastidious. Check out https://code.google.com/p/chromium/codesearch#chromium/src/base/process/memor.... This is a test that verifies we die if realloc() fails.
https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:199: base::Unretained(this))); On 2014/06/19 04:57:42, Pawel Osciak wrote: > On 2014/06/19 04:28:34, wuchengli wrote: > > On 2014/06/19 01:31:11, Pawel Osciak wrote: > > > On 2014/06/18 15:57:44, wuchengli wrote: > > > > Can you add comment to explain why Unretained(this) is safe? All vaapi > calls > > > run > > > > on encoder thread? va_surface_release_cb_ will be posted from the encoder > > > thread > > > > to encoder thread? When is a VASurface destroyed? > > > > > > > > Is it possible a VASurface is destroyed by "delete this" in Destroy() and > > > > trigger a |va_surface_release_cb_|? > > > > > > This is explained in the header, I'd prefer not to repeat it here. The > > callback > > > is posted to the encoder_thread_, and "Posting from the ChildThread using > > > base::Unretained to the encoder thread is safe, because |this| always > outlives > > > the encoder thread (it's a member of this class).". > > > > > > It does not matter from where a task is posted to a thread, as long as it is > > > posted to/executes on the right thread. > > > > > > As explained in the header, all va resources are owned and destroyed by the > > > vaapi_wrapper_. When delete this is called, the callback cannot be executed > > > anymore, because the encoder thread is Stop()ped. But it doesn't matter, > > > VASurface just stores ints and does not manage the underlying surfaces in > any > > > way, it's just to indicate what is in use and what is available for reuse. > > I don't understand why the release callback is posted from the ChildThread. It > > looks like VASurface can be destructed from ref_pic_list0_.pop_back() of > > EndFrame() or from the end of TryToReturnBitstreamBuffer(). Is that right? > > Please show me the exact location where the destructor of VASurface is called. > > You misunderstood me, I never said it was posted from ChildThread. I only said > it was posted to encoder_thread_. > And that it does not matter from which task it is posted, only where it is > posted to/where it runs on. > It's always run on the encoder_thread_, which will never outlive |this|. > > The whole beauty of this whole setup that I don't have to worry about this at > all for the above reason. > And again, the actual resources are managed by the wrapper, which will outlive > encoder_thread_ as well. I see. vaapi_wrapper and the release callback is always accessed on encoder thread. So there is no need to post and media::BindToCurrentLoop can be removed. Right?
https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:199: base::Unretained(this))); On 2014/06/19 05:29:31, wuchengli wrote: > On 2014/06/19 04:57:42, Pawel Osciak wrote: > > On 2014/06/19 04:28:34, wuchengli wrote: > > > On 2014/06/19 01:31:11, Pawel Osciak wrote: > > > > On 2014/06/18 15:57:44, wuchengli wrote: > > > > > Can you add comment to explain why Unretained(this) is safe? All vaapi > > calls > > > > run > > > > > on encoder thread? va_surface_release_cb_ will be posted from the > encoder > > > > thread > > > > > to encoder thread? When is a VASurface destroyed? > > > > > > > > > > Is it possible a VASurface is destroyed by "delete this" in Destroy() > and > > > > > trigger a |va_surface_release_cb_|? > > > > > > > > This is explained in the header, I'd prefer not to repeat it here. The > > > callback > > > > is posted to the encoder_thread_, and "Posting from the ChildThread using > > > > base::Unretained to the encoder thread is safe, because |this| always > > outlives > > > > the encoder thread (it's a member of this class).". > > > > > > > > It does not matter from where a task is posted to a thread, as long as it > is > > > > posted to/executes on the right thread. > > > > > > > > As explained in the header, all va resources are owned and destroyed by > the > > > > vaapi_wrapper_. When delete this is called, the callback cannot be > executed > > > > anymore, because the encoder thread is Stop()ped. But it doesn't matter, > > > > VASurface just stores ints and does not manage the underlying surfaces in > > any > > > > way, it's just to indicate what is in use and what is available for reuse. > > > I don't understand why the release callback is posted from the ChildThread. > It > > > looks like VASurface can be destructed from ref_pic_list0_.pop_back() of > > > EndFrame() or from the end of TryToReturnBitstreamBuffer(). Is that right? > > > Please show me the exact location where the destructor of VASurface is > called. > > > > You misunderstood me, I never said it was posted from ChildThread. I only said > > it was posted to encoder_thread_. > > And that it does not matter from which task it is posted, only where it is > > posted to/where it runs on. > > It's always run on the encoder_thread_, which will never outlive |this|. > > > > The whole beauty of this whole setup that I don't have to worry about this at > > all for the above reason. > > And again, the actual resources are managed by the wrapper, which will outlive > > encoder_thread_ as well. > I see. vaapi_wrapper and the release callback is always accessed on encoder > thread. So there is no need to post and media::BindToCurrentLoop can be removed. > Right? For now, yes, the plan is though for some of the operations (namely upload and download, and possibly sync) to be managed on separate threads, but I removed that from this CL to make it simpler for now.
LGTM (for the parts not related to H264 spec) https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:571: current_encode_job_->reference_surfaces.push_back(*iter); current_encode_job_->reference_surfaces = ref_pic_list0; should be enough. https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_video_encode_accelerator.h (right): https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.h:72: kMinSurfacesToEncode * (kNumInputBuffers - 1), Is it simpler to use "kNumSurfaces = kMaxNumReferenceFrames + kMinSurfacesToEncode * kNumInputBuffers"? It looks like we need kMaxNumReferenceFrames and every input buffer needs kMinSurfacesToEncode. https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.h:112: // to them here, because we may discard some of them from ref_pic_list* You mean |ref_pic_list0_|?
https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:571: current_encode_job_->reference_surfaces.push_back(*iter); On 2014/06/19 07:31:48, wuchengli wrote: > current_encode_job_->reference_surfaces = ref_pic_list0; should be enough. Done. https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_video_encode_accelerator.h (right): https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.h:72: kMinSurfacesToEncode * (kNumInputBuffers - 1), On 2014/06/19 07:31:48, wuchengli wrote: > Is it simpler to use "kNumSurfaces = kMaxNumReferenceFrames + > kMinSurfacesToEncode * kNumInputBuffers"? It looks like we need > kMaxNumReferenceFrames and every input buffer needs kMinSurfacesToEncode. I felt this was easier to understand, because this way it matches the description above exactly. https://codereview.chromium.org/333253002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.h:112: // to them here, because we may discard some of them from ref_pic_list* On 2014/06/19 07:31:48, wuchengli wrote: > You mean |ref_pic_list0_|? No, this is intentional. I want to suggest that when/if we add ref_pic_list1, it should also be included here.
isherman@: please histograms piman@: please switches in chrome_restart_request.cc, render_process_host_impl.cc, content_switches.*, also please content gypi. Thank you.
jorgelo@/jln@: please bpf_gpu_policy_linux.cc Thanks.
I have to admit that I don't understand most of the CL :) I skimmed the code and only have a few nits. Otherwise rslgtm! https://codereview.chromium.org/333253002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/login/chrome_restart_request.cc (right): https://codereview.chromium.org/333253002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/login/chrome_restart_request.cc:163: ::switches::kEnableVaapiAcceleratedVideoEncode, why put it here? most of the list is sorted except for some grouped by #ifdefs. https://codereview.chromium.org/333253002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/333253002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_encode_accelerator.cc:30: static inline size_t RoundUpToPowerOf2(size_t value, size_t alignment) { Add a comment? It's not obvious what it's doing... https://codereview.chromium.org/333253002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_encode_accelerator.cc:52: struct VaapiVideoEncodeAccelerator::BitstreamBufferRef { This takes the ownership of |shm|, why it's called "Ref"? https://codereview.chromium.org/333253002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_encode_accelerator.cc:193: base::Unretained(this))); Here and below. You already have |weak_this_|, why not use it here? https://codereview.chromium.org/333253002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_encode_accelerator.h (right): https://codereview.chromium.org/333253002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_encode_accelerator.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: s/(c)// https://codereview.chromium.org/333253002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_encode_accelerator.h:43: virtual void Destroy() OVERRIDE; nit: Empty line after this. https://codereview.chromium.org/333253002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_encode_accelerator.h:92: }; These should really be "const int" in the .cc file than a enum here. https://codereview.chromium.org/333253002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_encode_accelerator.h:244: unsigned int cpb_size_; nit: Can we just use uint32 for these then we don't need the base::checked_cast<> in the .cc file. https://codereview.chromium.org/333253002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://codereview.chromium.org/333253002/diff/120001/content/common/gpu/medi... content/common/gpu/media/video_encode_accelerator_unittest.cc:590: #elif defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY) how about defined(USE_X11)? https://codereview.chromium.org/333253002/diff/120001/content/public/common/c... File content/public/common/content_switches.h (right): https://codereview.chromium.org/333253002/diff/120001/content/public/common/c... content/public/common/content_switches.h:273: CONTENT_EXPORT extern const char kEnableVaapiAcceleratedVideoEncode[]; should this be guarded by defined(ARCH_CPU_X86_FAMILY) as well? https://codereview.chromium.org/333253002/diff/120001/media/filters/h264_bits... File media/filters/h264_bitstream_buffer.h (right): https://codereview.chromium.org/333253002/diff/120001/media/filters/h264_bits... media/filters/h264_bitstream_buffer.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. nit: ditto about (c) https://codereview.chromium.org/333253002/diff/120001/media/filters/h264_bits... media/filters/h264_bitstream_buffer.h:23: class MEDIA_EXPORT H264BitstreamBuffer { add include for MEDIA_EXPORT https://codereview.chromium.org/333253002/diff/120001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/333253002/diff/120001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:45454: + <int value="0" label="VAAPI_ERROR"/> If you don't report success cases, how do you calculate the failure rate?
https://chromiumcodereview.appspot.com/333253002/diff/120001/content/common/g... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/333253002/diff/120001/content/common/g... content/common/gpu/media/vaapi_video_encode_accelerator.cc:30: static inline size_t RoundUpToPowerOf2(size_t value, size_t alignment) { On 2014/06/20 07:34:39, xhwang wrote: > Add a comment? It's not obvious what it's doing... Done. https://chromiumcodereview.appspot.com/333253002/diff/120001/content/common/g... content/common/gpu/media/vaapi_video_encode_accelerator.cc:52: struct VaapiVideoEncodeAccelerator::BitstreamBufferRef { On 2014/06/20 07:34:39, xhwang wrote: > This takes the ownership of |shm|, why it's called "Ref"? Mostly historical reasons, we use a similar struct in V4L2VideoEncodeAccelerator. I'm finding it hard to find a better name, but the main reason is want to keep it the same as the one in V4L2VEA for "consistency". https://chromiumcodereview.appspot.com/333253002/diff/120001/content/common/g... content/common/gpu/media/vaapi_video_encode_accelerator.cc:193: base::Unretained(this))); On 2014/06/20 07:34:39, xhwang wrote: > Here and below. You already have |weak_this_|, why not use it here? |weak_this_| is for use from ChildThread. This is posted to the encoder thread. https://chromiumcodereview.appspot.com/333253002/diff/120001/content/common/g... File content/common/gpu/media/vaapi_video_encode_accelerator.h (right): https://chromiumcodereview.appspot.com/333253002/diff/120001/content/common/g... content/common/gpu/media/vaapi_video_encode_accelerator.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/06/20 07:34:39, xhwang wrote: > nit: s/(c)// Done. https://chromiumcodereview.appspot.com/333253002/diff/120001/content/common/g... content/common/gpu/media/vaapi_video_encode_accelerator.h:43: virtual void Destroy() OVERRIDE; On 2014/06/20 07:34:39, xhwang wrote: > nit: Empty line after this. Done. https://chromiumcodereview.appspot.com/333253002/diff/120001/content/common/g... content/common/gpu/media/vaapi_video_encode_accelerator.h:92: }; On 2014/06/20 07:34:39, xhwang wrote: > These should really be "const int" in the .cc file than a enum here. Done. https://chromiumcodereview.appspot.com/333253002/diff/120001/content/common/g... content/common/gpu/media/vaapi_video_encode_accelerator.h:244: unsigned int cpb_size_; On 2014/06/20 07:34:39, xhwang wrote: > nit: Can we just use uint32 for these then we don't need the > base::checked_cast<> in the .cc file. Well, technically, I don't really have to use checked_cast there. https://chromiumcodereview.appspot.com/333253002/diff/120001/content/common/g... File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): https://chromiumcodereview.appspot.com/333253002/diff/120001/content/common/g... content/common/gpu/media/video_encode_accelerator_unittest.cc:590: #elif defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY) On 2014/06/20 07:34:39, xhwang wrote: > how about defined(USE_X11)? Done. https://chromiumcodereview.appspot.com/333253002/diff/120001/content/public/c... File content/public/common/content_switches.h (right): https://chromiumcodereview.appspot.com/333253002/diff/120001/content/public/c... content/public/common/content_switches.h:273: CONTENT_EXPORT extern const char kEnableVaapiAcceleratedVideoEncode[]; On 2014/06/20 07:34:39, xhwang wrote: > should this be guarded by defined(ARCH_CPU_X86_FAMILY) as well? It could, but honestly I just didn't want to sprinkle those around. This flag is not exposed in about:flags, and it can't enable anything anyway if ARCH_CPU_X86_FAMILY is not defined. And it will go away soon. I can add this if people feel strongly about it though. https://chromiumcodereview.appspot.com/333253002/diff/120001/media/filters/h2... File media/filters/h264_bitstream_buffer.h (right): https://chromiumcodereview.appspot.com/333253002/diff/120001/media/filters/h2... media/filters/h264_bitstream_buffer.h:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/06/20 07:34:39, xhwang wrote: > nit: ditto about (c) Done. https://chromiumcodereview.appspot.com/333253002/diff/120001/media/filters/h2... media/filters/h264_bitstream_buffer.h:23: class MEDIA_EXPORT H264BitstreamBuffer { On 2014/06/20 07:34:39, xhwang wrote: > add include for MEDIA_EXPORT Done. https://chromiumcodereview.appspot.com/333253002/diff/120001/tools/metrics/hi... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/333253002/diff/120001/tools/metrics/hi... tools/metrics/histograms/histograms.xml:45454: + <int value="0" label="VAAPI_ERROR"/> On 2014/06/20 07:34:39, xhwang wrote: > If you don't report success cases, how do you calculate the failure rate? The idea is this should be 0. Anything else is bad. We have UMA for total encoder instantiaion rates though (Media.RTCVideoEncoderInitEncodeSuccess), which could be used for that.
On 2014/06/20 10:53:16, Pawel Osciak wrote: > https://chromiumcodereview.appspot.com/333253002/diff/120001/content/common/g... > File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): > > https://chromiumcodereview.appspot.com/333253002/diff/120001/content/common/g... > content/common/gpu/media/vaapi_video_encode_accelerator.cc:30: static inline > size_t RoundUpToPowerOf2(size_t value, size_t alignment) { > On 2014/06/20 07:34:39, xhwang wrote: > > Add a comment? It's not obvious what it's doing... > > Done. > > https://chromiumcodereview.appspot.com/333253002/diff/120001/content/common/g... > content/common/gpu/media/vaapi_video_encode_accelerator.cc:52: struct > VaapiVideoEncodeAccelerator::BitstreamBufferRef { > On 2014/06/20 07:34:39, xhwang wrote: > > This takes the ownership of |shm|, why it's called "Ref"? > > Mostly historical reasons, we use a similar struct in > V4L2VideoEncodeAccelerator. > I'm finding it hard to find a better name, but the main reason is want to keep > it the same as the one in V4L2VEA for "consistency". > > https://chromiumcodereview.appspot.com/333253002/diff/120001/content/common/g... > content/common/gpu/media/vaapi_video_encode_accelerator.cc:193: > base::Unretained(this))); > On 2014/06/20 07:34:39, xhwang wrote: > > Here and below. You already have |weak_this_|, why not use it here? > > |weak_this_| is for use from ChildThread. This is posted to the encoder thread. > > https://chromiumcodereview.appspot.com/333253002/diff/120001/content/common/g... > File content/common/gpu/media/vaapi_video_encode_accelerator.h (right): > > https://chromiumcodereview.appspot.com/333253002/diff/120001/content/common/g... > content/common/gpu/media/vaapi_video_encode_accelerator.h:1: // Copyright (c) > 2014 The Chromium Authors. All rights reserved. > On 2014/06/20 07:34:39, xhwang wrote: > > nit: s/(c)// > > Done. > > https://chromiumcodereview.appspot.com/333253002/diff/120001/content/common/g... > content/common/gpu/media/vaapi_video_encode_accelerator.h:43: virtual void > Destroy() OVERRIDE; > On 2014/06/20 07:34:39, xhwang wrote: > > nit: Empty line after this. > > Done. > > https://chromiumcodereview.appspot.com/333253002/diff/120001/content/common/g... > content/common/gpu/media/vaapi_video_encode_accelerator.h:92: }; > On 2014/06/20 07:34:39, xhwang wrote: > > These should really be "const int" in the .cc file than a enum here. > > Done. > > https://chromiumcodereview.appspot.com/333253002/diff/120001/content/common/g... > content/common/gpu/media/vaapi_video_encode_accelerator.h:244: unsigned int > cpb_size_; > On 2014/06/20 07:34:39, xhwang wrote: > > nit: Can we just use uint32 for these then we don't need the > > base::checked_cast<> in the .cc file. > > Well, technically, I don't really have to use checked_cast there. > > https://chromiumcodereview.appspot.com/333253002/diff/120001/content/common/g... > File content/common/gpu/media/video_encode_accelerator_unittest.cc (right): > > https://chromiumcodereview.appspot.com/333253002/diff/120001/content/common/g... > content/common/gpu/media/video_encode_accelerator_unittest.cc:590: #elif > defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY) > On 2014/06/20 07:34:39, xhwang wrote: > > how about defined(USE_X11)? > > Done. > > https://chromiumcodereview.appspot.com/333253002/diff/120001/content/public/c... > File content/public/common/content_switches.h (right): > > https://chromiumcodereview.appspot.com/333253002/diff/120001/content/public/c... > content/public/common/content_switches.h:273: CONTENT_EXPORT extern const char > kEnableVaapiAcceleratedVideoEncode[]; > On 2014/06/20 07:34:39, xhwang wrote: > > should this be guarded by defined(ARCH_CPU_X86_FAMILY) as well? > > It could, but honestly I just didn't want to sprinkle those around. This flag is > not exposed in about:flags, and it can't enable anything anyway if > ARCH_CPU_X86_FAMILY is not defined. And it will go away soon. > I can add this if people feel strongly about it though. > > https://chromiumcodereview.appspot.com/333253002/diff/120001/media/filters/h2... > File media/filters/h264_bitstream_buffer.h (right): > > https://chromiumcodereview.appspot.com/333253002/diff/120001/media/filters/h2... > media/filters/h264_bitstream_buffer.h:1: // Copyright (c) 2014 The Chromium > Authors. All rights reserved. > On 2014/06/20 07:34:39, xhwang wrote: > > nit: ditto about (c) > > Done. > > https://chromiumcodereview.appspot.com/333253002/diff/120001/media/filters/h2... > media/filters/h264_bitstream_buffer.h:23: class MEDIA_EXPORT H264BitstreamBuffer > { > On 2014/06/20 07:34:39, xhwang wrote: > > add include for MEDIA_EXPORT > > Done. > > https://chromiumcodereview.appspot.com/333253002/diff/120001/tools/metrics/hi... > File tools/metrics/histograms/histograms.xml (right): > > https://chromiumcodereview.appspot.com/333253002/diff/120001/tools/metrics/hi... > tools/metrics/histograms/histograms.xml:45454: + <int value="0" > label="VAAPI_ERROR"/> > On 2014/06/20 07:34:39, xhwang wrote: > > If you don't report success cases, how do you calculate the failure rate? > > The idea is this should be 0. Anything else is bad. > We have UMA for total encoder instantiaion rates though > (Media.RTCVideoEncoderInitEncodeSuccess), which could be used for that. bpf_gpu_policy_linux.cc lgtm
lgtm++ Thanks!
https://chromiumcodereview.appspot.com/333253002/diff/140001/content/browser/... File content/browser/renderer_host/render_process_host_impl.cc (right): https://chromiumcodereview.appspot.com/333253002/diff/140001/content/browser/... content/browser/renderer_host/render_process_host_impl.cc:1254: #endif I don't see where this is used in the renderer. https://chromiumcodereview.appspot.com/333253002/diff/140001/content/common/g... File content/common/gpu/media/gpu_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/333253002/diff/140001/content/common/g... content/common/gpu/media/gpu_video_encode_accelerator.cc:193: #endif Should this be gated on the flag? Rationale: don't expose new security surface until we're confident enough with the code to enable it.
https://chromiumcodereview.appspot.com/333253002/diff/140001/content/common/g... File content/common/gpu/media/vaapi_video_encode_accelerator.h (right): https://chromiumcodereview.appspot.com/333253002/diff/140001/content/common/g... content/common/gpu/media/vaapi_video_encode_accelerator.h:52: }; Can this be defined in the anonymous namespace instead? https://chromiumcodereview.appspot.com/333253002/diff/140001/tools/metrics/hi... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/333253002/diff/140001/tools/metrics/hi... tools/metrics/histograms/histograms.xml:45454: + <int value="0" label="VAAPI_ERROR"/> Is there really only a single failure type? What information do you hope to draw from this histogram? If it's an error rate, what is the baseline that you're planning to compare to? Can that baseline be built into the histogram instead?
https://chromiumcodereview.appspot.com/333253002/diff/140001/content/browser/... File content/browser/renderer_host/render_process_host_impl.cc (right): https://chromiumcodereview.appspot.com/333253002/diff/140001/content/browser/... content/browser/renderer_host/render_process_host_impl.cc:1254: #endif On 2014/06/20 17:10:22, piman wrote: > I don't see where this is used in the renderer. Yeah, this is a bit convoluted. We need a way to be able to tell which codecs/profiles are supported by a hw encoder from the renderer on request from libjingle, before we make a decision to instantiate the codec in GPU process. The method used to retrieve the list of supported codecs is currently a hack, a hardcoded in each accelerator VEA::GetSupportedProfiles, which is actually a static method in HW accelerators executed in renderer from RTCVideoEncoderFactory (https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...) and then used to decide if we would be able to fulfill webrtc's request for a HW codec that would support given profile (https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...). We are working on a proper solution for this (crbug.com/350197) which will add codec feature autodetection, this is temporary. Until that is ready however, the flag is checked in GetSupportedProfiles in HW encoders when called from renderer and is used to decide what can be added to the list of supported profiles. In VaapiVideoEncodeAccelerator::GetSupportedProfiles, the flag gates everything, so no profiles will be added to the list if it's not on. Not having any profiles on the list will result in that we'll not match any requested codecs in RTCVideoEncoderFactory and VaapiVideoEncodeAccelerator will never be instantiated. https://chromiumcodereview.appspot.com/333253002/diff/140001/content/common/g... File content/common/gpu/media/gpu_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/333253002/diff/140001/content/common/g... content/common/gpu/media/gpu_video_encode_accelerator.cc:193: #endif On 2014/06/20 17:10:22, piman wrote: > Should this be gated on the flag? Rationale: don't expose new security surface > until we're confident enough with the code to enable it. It actually is, just in a convoluted way. As I mentioned in the comment in render_process_host_impl.cc, the codec will not get instantiated (we'll not get here from the renderer) if the list of supported profiles is empty in the renderer when we call the static GetSupportedProfiles. And the list will be empty if the flag is not on. I know it's far from perfect, but the flag has to be there in the renderer in GetSupportedProfiles, it's to late to do this here in GPU process, because we can't say we'd match a profile requested there and fail here as things are right now iirc. I could add it here, but it felt superficial, as we'd never have gotten this far anyway. https://chromiumcodereview.appspot.com/333253002/diff/140001/content/common/g... File content/common/gpu/media/vaapi_video_encode_accelerator.h (right): https://chromiumcodereview.appspot.com/333253002/diff/140001/content/common/g... content/common/gpu/media/vaapi_video_encode_accelerator.h:52: }; On 2014/06/20 20:04:30, Ilya Sherman wrote: > Can this be defined in the anonymous namespace instead? Sure, just curious why would that be preferred though? https://chromiumcodereview.appspot.com/333253002/diff/140001/tools/metrics/hi... File tools/metrics/histograms/histograms.xml (right): https://chromiumcodereview.appspot.com/333253002/diff/140001/tools/metrics/hi... tools/metrics/histograms/histograms.xml:45454: + <int value="0" label="VAAPI_ERROR"/> On 2014/06/20 20:04:30, Ilya Sherman wrote: > Is there really only a single failure type? What information do you hope to > draw from this histogram? If it's an error rate, what is the baseline that > you're planning to compare to? Can that baseline be built into the histogram > instead? The class I'm adding reuses VaapiWrapper, which is also used by another class for HW decode. VaapiWrapper takes a callback for reporting errors. That decode class has a longer list of failures (Media.VAVDA.DecoderFailure, https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...), which we use to understand the usage of unimplemented features, as well as whether vaapi backend is working fine (VAAPI_ERROR is one of the error codes in the decode class' histogram). The baseline for this uma in this encoder class is the number of successful instantiations of RTCVideoEncoder, which is the Media.RTCVideoEncoderInitEncodeSuccess. Ideally, the number of VAAPI_ERROR should be zero if things are working properly. A higher number would indicate a platform/driver problems in the wild that we'd have to investigate.
Histograms LGTM https://chromiumcodereview.appspot.com/333253002/diff/140001/content/common/g... File content/common/gpu/media/vaapi_video_encode_accelerator.h (right): https://chromiumcodereview.appspot.com/333253002/diff/140001/content/common/g... content/common/gpu/media/vaapi_video_encode_accelerator.h:52: }; On 2014/06/20 22:28:11, Pawel Osciak wrote: > On 2014/06/20 20:04:30, Ilya Sherman wrote: > > Can this be defined in the anonymous namespace instead? > > Sure, just curious why would that be preferred though? In general, if you don't need to expose something in a header file, it's preferable not to expose it. Doubly so for the public API of a class -- if clients of the class don't need to know about an implementation detail, it's best to keep that hidden. This both creates a smaller, easier to understand API; and reduces the chances of somebody depending on something they shouldn't depend on.
https://chromiumcodereview.appspot.com/333253002/diff/140001/content/common/g... File content/common/gpu/media/vaapi_video_encode_accelerator.h (right): https://chromiumcodereview.appspot.com/333253002/diff/140001/content/common/g... content/common/gpu/media/vaapi_video_encode_accelerator.h:52: }; On 2014/06/20 22:33:30, Ilya Sherman wrote: > On 2014/06/20 22:28:11, Pawel Osciak wrote: > > On 2014/06/20 20:04:30, Ilya Sherman wrote: > > > Can this be defined in the anonymous namespace instead? > > > > Sure, just curious why would that be preferred though? > > In general, if you don't need to expose something in a header file, it's > preferable not to expose it. Doubly so for the public API of a class -- if > clients of the class don't need to know about an implementation detail, it's > best to keep that hidden. This both creates a smaller, easier to understand > API; and reduces the chances of somebody depending on something they shouldn't > depend on. Ah of course, good point. Thanks. Moved.
https://chromiumcodereview.appspot.com/333253002/diff/140001/content/common/g... File content/common/gpu/media/gpu_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/333253002/diff/140001/content/common/g... content/common/gpu/media/gpu_video_encode_accelerator.cc:193: #endif On 2014/06/20 22:28:11, Pawel Osciak wrote: > On 2014/06/20 17:10:22, piman wrote: > > Should this be gated on the flag? Rationale: don't expose new security surface > > until we're confident enough with the code to enable it. > > It actually is, just in a convoluted way. As I mentioned in the comment in > render_process_host_impl.cc, the codec will not get instantiated (we'll not get > here from the renderer) if the list of supported profiles is empty in the > renderer when we call the static GetSupportedProfiles. And the list will be > empty if the flag is not on. > I know it's far from perfect, but the flag has to be there in the renderer in > GetSupportedProfiles, it's to late to do this here in GPU process, because we > can't say we'd match a profile requested there and fail here as things are right > now iirc. I could add it here, but it felt superficial, as we'd never have > gotten this far anyway. The idea is that the gpu process is privileged, it can't trust the renderer. So the idea would be to fail if the flag isn't set, so that a compromised renderer (or malicious nacl plugin) doesn't try to access it anyway.
https://chromiumcodereview.appspot.com/333253002/diff/140001/content/common/g... File content/common/gpu/media/gpu_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/333253002/diff/140001/content/common/g... content/common/gpu/media/gpu_video_encode_accelerator.cc:193: #endif On 2014/06/21 01:27:06, piman wrote: > On 2014/06/20 22:28:11, Pawel Osciak wrote: > > On 2014/06/20 17:10:22, piman wrote: > > > Should this be gated on the flag? Rationale: don't expose new security > surface > > > until we're confident enough with the code to enable it. > > > > It actually is, just in a convoluted way. As I mentioned in the comment in > > render_process_host_impl.cc, the codec will not get instantiated (we'll not > get > > here from the renderer) if the list of supported profiles is empty in the > > renderer when we call the static GetSupportedProfiles. And the list will be > > empty if the flag is not on. > > I know it's far from perfect, but the flag has to be there in the renderer in > > GetSupportedProfiles, it's to late to do this here in GPU process, because we > > can't say we'd match a profile requested there and fail here as things are > right > > now iirc. I could add it here, but it felt superficial, as we'd never have > > gotten this far anyway. > > > The idea is that the gpu process is privileged, it can't trust the renderer. So > the idea would be to fail if the flag isn't set, so that a compromised renderer > (or malicious nacl plugin) doesn't try to access it anyway. Good point. Done. This makes we want to do the same for other accelerators and decode too (in follow-up CLs).
lgtm
The CQ bit was checked by posciak@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/posciak@chromium.org/333253002/300001
Message was sent while issue was closed.
Change committed as 279650
Message was sent while issue was closed.
On 2014/06/25 07:08:11, I haz the power (commit-bot) wrote: > Change committed as 279650 This broke the Chrome-on-Chrome OS builder: http://master.chrome.corp.google.com:8011/builders/cros%20trunk/builds/21417 FAILED: c++ -MMD -MF obj/media/filters/media_unittests.h264_bitstream_buffer_unittest.o.d -DV8_DEPRECATION_WARNINGS -DBLINK_SCALE_FILTERS_AT_RECORD_TIME -D_FILE_OFFSET_BITS=64 -DGOOGLE_CHROME_BUILD -DENABLE_RLZ -DTOOLKIT_VIEWS=1 -DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1 -DUSE_ASH=1 -DUSE_CAIRO=1 -DUSE_GLIB=1 -DUSE_DEFAULT_RENDER_THEME=1 -DUSE_X11=1 -DUSE_XI2_MT=2 -DIMAGE_LOADER_EXTENSION=1 -DENABLE_REMOTING=1 -DENABLE_WEBRTC=1 -DUSE_PROPRIETARY_CODECS -DENABLE_PEPPER_CDMS -DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1 -DUSE_UDEV -DENABLE_EGLIMAGE=1 -DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGINS=1 -DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_AUTOFILL_DIALOG=1 -DENABLE_PROD_WALLET_SERVICE=1 -DENABLE_BACKGROUND=1 -DENABLE_GOOGLE_NOW=1 -DCLD_VERSION=2 -DCLD_DATA_FROM_STATIC -DENABLE_FULL_PRINTING=1 -DENABLE_PRINTING=1 -DENABLE_SPELLCHECK=1 -DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_MANAGED_USERS=1 -DENABLE_MDNS=1 -DENABLE_SERVICE_DISCOVERY=1 -DUSE_NEON -DUSE_ALSA -DUSE_PULSEAUDIO -DGTEST_HAS_POSIX_RE=0 -DSK_ENABLE_INST_COUNT=0 -DSK_SUPPORT_GPU=1 '-DGR_GL_CUSTOM_SETUP_HEADER="GrGLConfig_chrome.h"' -DSK_ENABLE_LEGACY_API_ALIASING=1 -DSK_ATTR_DEPRECATED=SK_NOTHING_ARG1 -DGR_GL_IGNORE_ES3_MSAA=0 -DSK_WILL_NEVER_DRAW_PERSPECTIVE_TEXT -DSK_SUPPORT_LEGACY_GETTOPDEVICE -DSK_SUPPORT_LEGACY_BITMAP_CONFIG -DSK_SUPPORT_LEGACY_DEVICE_VIRTUAL_ISOPAQUE -DSK_SUPPORT_LEGACY_N32_NAME -DSK_SUPPORT_LEGACY_SETCONFIG -DSK_IGNORE_ETC1_SUPPORT -DSK_IGNORE_GPU_DITHER -DSK_SUPPORT_LEGACY_GETTOTALCLIP -DSK_USE_POSIX_THREADS -DSK_DEFERRED_CANVAS_USES_FACTORIES=1 -DUNIT_TEST -DGTEST_HAS_RTTI=0 -DU_USING_ICU_NAMESPACE=0 -DU_STATIC_IMPLEMENTATION -D__STDC_CONSTANT_MACROS -DUSE_NSS=1 -DOS_CHROMEOS=1 -D__STDC_FORMAT_MACROS -DNDEBUG -DOFFICIAL_BUILD -DNVALGRIND -DDYNAMIC_ANNOTATIONS_ENABLED=0 -D_FORTIFY_SOURCE=2 -Igen -I../../third_party/khronos -I../../gpu -I../.. -I../../skia/config -I../../third_party/skia/src/core -I../../third_party/skia/include/core -I../../third_party/skia/include/effects -I../../third_party/skia/include/pdf -I../../third_party/skia/include/gpu -I../../third_party/skia/include/lazy -I../../third_party/skia/include/pathops -I../../third_party/skia/include/pipe -I../../third_party/skia/include/ports -I../../third_party/skia/include/utils -I../../skia/ext -I../../testing/gmock/include -I../../testing/gtest/include -I../../third_party/icu/source/i18n -I../../third_party/icu/source/common -Igen/ffmpeg -I../../third_party/ffmpeg/chromium/config/ChromeOS/linux/ia32 -I../../third_party/ffmpeg -Werror -pthread -fno-exceptions -fno-strict-aliasing -Wall -Wno-unused-parameter -Wno-missing-field-initializers -fvisibility=hidden -pipe -fPIC -g -pthread -I/usr/include/glib-2.0 -I/usr/lib/i386-linux-gnu/glib-2.0/include -msse2 -mfpmath=sse -mmmx -m32 -O2 -fno-ident -fdata-sections -ffunction-sections -fno-unwind-tables -fno-asynchronous-unwind-tables -fno-rtti -fno-threadsafe-statics -fvisibility-inlines-hidden -Wsign-compare -c ../../media/filters/h264_bitstream_buffer_unittest.cc -o obj/media/filters/media_unittests.h264_bitstream_buffer_unittest.o ../../media/filters/h264_bitstream_buffer_unittest.cc: In function ‘testing::internal::ParamGenerator<unsigned int> media::gtest_AppendNumBitsH264BitstreamBufferAppendBitsTest_EvalGenerator_()’: ../../media/filters/h264_bitstream_buffer_unittest.cc:52:1: error: could not convert ‘testing::Range(T, T) [with T = long unsigned int](65ul)’ from ‘testing::internal::ParamGenerator<long unsigned int>’ to ‘testing::internal::ParamGenerator<unsigned int>’ ../../media/filters/h264_bitstream_buffer_unittest.cc:52:1: error: control reaches end of non-void function [-Werror=return-type] Reverted at r279733 using Drover since the codereview tool isn't working See crbug.com/388778 |
