Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
872623002:
VaapiVEA: Get maximum resolution from libva (Closed)
|
Created:
5 years, 11 months ago by henryhsu Modified:
5 years, 9 months ago CC:
chromium-reviews, posciak+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, wjia+watch_chromium.org, robert.bradford Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionVaapiVEA: Get maximum resolution from libva
Add GetMaxResolutionForVAConfigID function to get hardware supported maximum
resolution for video encode and decode. This is also the preparation of
removing --ignore-resolution-limits-for-accelerated-video-decode flag.
Because this function uses va_config_id as parameter, we refactor
vaapi_wrapper and add LazyInstance to initialize supported profile
infos once and reuse it in Initialize and GetSupportedEncodeProfiles
functions.
BUG=350197
TEST=Test on squawks, and lumpy. Make sure this function works well.
Committed: https://crrev.com/a598f3adabbe02f427e2233239cdead68cf21d4f
Cr-Commit-Position: refs/heads/master@{#319842}
Patch Set 1 #
Total comments: 1
Patch Set 2 : refactor vaapi_wrapper #Patch Set 3 : modify description #
Total comments: 29
Patch Set 4 : Address review comments #
Total comments: 33
Patch Set 5 : address review comments #Patch Set 6 : fix nits #
Total comments: 64
Patch Set 7 : address all review comments #
Total comments: 23
Patch Set 8 : address patch set 7 review comments #
Total comments: 11
Patch Set 9 : address comments #
Total comments: 6
Patch Set 10 : add comments #
Total comments: 25
Patch Set 11 : address all review comments #
Total comments: 2
Patch Set 12 : #
Total comments: 11
Patch Set 13 : fix nits #Patch Set 14 : #
Messages
Total messages: 42 (4 generated)
henryhsu@chromium.org changed reviewers: + posciak@chromium.org, wuchengli@chromium.org
PTAL
https://codereview.chromium.org/872623002/diff/1/third_party/libva/va/va_back... File third_party/libva/va/va_backend.h (right): https://codereview.chromium.org/872623002/diff/1/third_party/libva/va/va_back... third_party/libva/va/va_backend.h:395: VAStatus (*vaGetCodecResolution) ( We should call this vaGetProfileResolution. Functions of va.h uses profile. For example, vaQueryConfigProfiles, VAProfile, and etc. This function doesn't have VAProfile or other argument. What's the prerequisite of calling this function? How does the caller know if the returned resolution is for H264 or VP8? For decode or encode? Should we pass VAProfile, VAEntrypoint, and VAConfigAttrib to this function?
PTAL
wuchengli@chromium.org changed reviewers: + kcwu@chromium.org
Kuang-che. Please also help review this. https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:259: static ProfileConfig supported_profiles_[media::VIDEO_CODEC_PROFILE_MAX * 2]; Chromium way to do this is using base::LazyInstance or Singleton. http://google-styleguide.googlecode.com/svn/trunk/cppguide.html?showone=Stati... http://www.chromium.org/developers/coding-style/important-abstractions-and-da...
https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... File content/common/gpu/media/va.sigs (right): https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/va.sigs:34: VAStatus vaQuerySurfaceAttributes(VADisplay dpy, VAConfigID config, VASurfaceAttrib *attrib_list, unsigned int *num_attribs); sort https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:61: VaapiWrapper::supported_profiles_[media::VIDEO_CODEC_PROFILE_MAX * 2] = MAX*2+1 https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:62: {{media::VIDEO_CODEC_PROFILE_UNKNOWN}}; add comment that UNKNOWN is guard. https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:189: scoped_ptr<VaapiWrapper> VaapiWrapper::Create( you need to rebase since I modified these functions recently. https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:256: for (size_t i = 0; i < arraysize(kProfileMap); i++) { DCHECK_LT(arraysize(kProfileMap) * modes.size(), arraysize(supported_profiles_)); https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:277: GetVaCodecMaxResolution(config_id, didn't check return value? https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:443: return true; Is it guaranteed that there are always VaSurfaceAttribMaxWidth and VaSurfaceAttribMaxHeight in attributes? https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:259: static ProfileConfig supported_profiles_[media::VIDEO_CODEC_PROFILE_MAX * 2]; Why not vector? if not vector, the size should be MAX*2+1. another idea: supported_profiles_[kCodecModeMax][media::VIDEO_CODEC_PROFILE_MAX+1]; Then you can save one loop later.
https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:443: return true; On 2015/02/02 10:11:02, kcwu wrote: > Is it guaranteed that there are always VaSurfaceAttribMaxWidth and > VaSurfaceAttribMaxHeight in attributes? No. If attributes don't include MaxWidth and MaxHeight, resolution should be (0, 0). Hmm... I'll add a check in this function because we expect the attributes should have these value. https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:259: static ProfileConfig supported_profiles_[media::VIDEO_CODEC_PROFILE_MAX * 2]; On 2015/02/02 10:11:02, kcwu wrote: > Why not vector? if not vector, the size should be MAX*2+1. > > another idea: > supported_profiles_[kCodecModeMax][media::VIDEO_CODEC_PROFILE_MAX+1]; > Then you can save one loop later. Static variables cannot use class object.
https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:120: profile.max_resolution.SetSize(1920, 1088); remove https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:248: if (is_initialize_profile_) Originally this class is thread-safe. You need |va_lock_| be thread safe. But using Singleton or LazyInstance should be able to get rid of this. https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:49: media::VideoCodecProfile profile; It should be better to store VaProfile in this class. For example, what's the max decode resolution of Jpeg? https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:63: static std::vector<media::VideoCodecProfile> GetSupportedProfiles( Now that we have to return both profile and resolution, make this return VideoEncodeAccelerator::SupportedProfile. That is, combine GetSupportedProfiles and GetCodecMaxResolution. https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:64: CodecMode mode); No need to add CodecMode parameter in this CL. Decode is not used now. Change this back to GetSupportedEncodeProfiles.
all done. PTAL https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... File content/common/gpu/media/va.sigs (right): https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/va.sigs:34: VAStatus vaQuerySurfaceAttributes(VADisplay dpy, VAConfigID config, VASurfaceAttrib *attrib_list, unsigned int *num_attribs); On 2015/02/02 10:11:01, kcwu wrote: > sort Done. https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:120: profile.max_resolution.SetSize(1920, 1088); On 2015/02/03 03:17:33, wuchengli wrote: > remove Done. https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:61: VaapiWrapper::supported_profiles_[media::VIDEO_CODEC_PROFILE_MAX * 2] = On 2015/02/02 10:11:01, kcwu wrote: > MAX*2+1 Not used anymore https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:62: {{media::VIDEO_CODEC_PROFILE_UNKNOWN}}; On 2015/02/02 10:11:01, kcwu wrote: > add comment that UNKNOWN is guard. Not used anymore https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:189: scoped_ptr<VaapiWrapper> VaapiWrapper::Create( On 2015/02/02 10:11:01, kcwu wrote: > you need to rebase since I modified these functions recently. Done. https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:248: if (is_initialize_profile_) On 2015/02/03 03:17:33, wuchengli wrote: > Originally this class is thread-safe. You need |va_lock_| be thread safe. But > using Singleton or LazyInstance should be able to get rid of this. Done. https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:256: for (size_t i = 0; i < arraysize(kProfileMap); i++) { On 2015/02/02 10:11:01, kcwu wrote: > DCHECK_LT(arraysize(kProfileMap) * modes.size(), > arraysize(supported_profiles_)); Not used anymore https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:277: GetVaCodecMaxResolution(config_id, On 2015/02/02 10:11:02, kcwu wrote: > didn't check return value? Done. https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:49: media::VideoCodecProfile profile; On 2015/02/03 03:17:33, wuchengli wrote: > It should be better to store VaProfile in this class. For example, what's the > max decode resolution of Jpeg? Done. https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:63: static std::vector<media::VideoCodecProfile> GetSupportedProfiles( On 2015/02/03 03:17:33, wuchengli wrote: > Now that we have to return both profile and resolution, make this return > VideoEncodeAccelerator::SupportedProfile. That is, combine GetSupportedProfiles > and GetCodecMaxResolution. Done. https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:64: CodecMode mode); On 2015/02/03 03:17:33, wuchengli wrote: > No need to add CodecMode parameter in this CL. Decode is not used now. Change > this back to GetSupportedEncodeProfiles. Done. https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:259: static ProfileConfig supported_profiles_[media::VIDEO_CODEC_PROFILE_MAX * 2]; On 2015/02/02 09:01:07, wuchengli wrote: > Chromium way to do this is using base::LazyInstance or Singleton. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html?showone=Stati... > > http://www.chromium.org/developers/coding-style/important-abstractions-and-da... Done. https://codereview.chromium.org/872623002/diff/40001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:259: static ProfileConfig supported_profiles_[media::VIDEO_CODEC_PROFILE_MAX * 2]; On 2015/02/02 09:01:07, wuchengli wrote: > Chromium way to do this is using base::LazyInstance or Singleton. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html?showone=Stati... > > http://www.chromium.org/developers/coding-style/important-abstractions-and-da... Done.
https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:92: {media::H264PROFILE_BASELINE, VAProfileH264ConstrainedBaseline}, This is incorrect. You break the original behavior. https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:1034: supported_encode_profiles_.push_back(all_profile_configs[i]); How about define it something like array of vector? supported_profiles_[mode].push_back(...); the getter will be simpler, too. https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:1040: break; NOTREACHED
https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:92: {media::H264PROFILE_BASELINE, VAProfileH264ConstrainedBaseline}, On 2015/02/13 07:15:38, kcwu wrote: > This is incorrect. You break the original behavior. offline talked with henry. The code is good. Please modify comment to reflect how you deal this issue. And maybe exchange the field order of ProfileMap.
https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:92: {media::H264PROFILE_BASELINE, VAProfileH264ConstrainedBaseline}, On 2015/02/13 07:15:38, kcwu wrote: > This is incorrect. You break the original behavior. As discuss, the logic is correct. But I'll modify comments and swap the order of profile and va_profile. https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:1034: supported_encode_profiles_.push_back(all_profile_configs[i]); On 2015/02/13 07:15:38, kcwu wrote: > How about define it something like array of vector? > supported_profiles_[mode].push_back(...); > > the getter will be simpler, too. ok. I'll add kCodecMax and CHECK rule.
https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:119: profiles[i].max_framerate_numerator = kDefaultFramerate; Let's move setting framerate and make a copy of kDefaultFramerate to vaapi_wrapper.cc. VaapiWrapper should decide the capability though it's not supported yet. https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:163: DVLOG(1) << "Unsupported profile"; also print |va_profile| https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:185: DVLOG(1) << "Unsupported profile"; also print |profile| https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:244: if (va_profiles[j] != VAProfileNone && Why this can be VAProfileNone? https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:248: supported_profile.mode = modes[i]; move l247-248 to l259. No need to do this if it fails later. Also, the operation of |supported_profile| should be together. https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:259: &supported_profile.max_resolution)) This for loop is hard to read. Can you flatten the ifs in the loop? for (size_t j = 0; ...) { if (!IsEntrypointSupported(va_profiles[j], entrypoint)) continue; if (!AreAttribsSupported(va_profiles[j], entrypoint, required_attribs))) continue; ... VAStatus va_res = vaCreateConfig(); if (va_res != VA_STATUS_SUCCESS) continue; if (!GetVaCodecMaxResolution()) continue; supported_profiles.push_back(supported_profile); } https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:421: if (!resolution->height() || !resolution->width()) This shouldn't happen. Right? Add LOG(ERROR) https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:432: for (i = 0; i < profile_configs.size(); ++i) This for loops have two lines. Please add braces for readability. I was confused when I first look at this code. https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:436: return false; Move line 427-436 to a LazyProfileConfig::IsProfileSupported(CodecMode, VAProfile). https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:49: struct ProfileConfig { This name is not good. This is not a VAConfig. This is not a "configuration" in normal meaning, either. Maybe ProfileInfo? Also move this to private. https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:51: VaapiWrapper::CodecMode mode; This field is redundant. All |mode| of |supported_encode_profiles_| are kEncode and all |mode| of |supported_decode_profiles_| are kDecode. Remove this variable. https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:77: static std::vector<ProfileConfig> InitSupportedProfileConfigs(); this should be private. https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:275: class CONTENT_EXPORT LazyProfileConfig { This is not used by other classes. Move this to vaapi_wrapper.cc and remove CONTENT_EXPORT.
all done. PTAL https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_video_encode_accelerator.cc:119: profiles[i].max_framerate_numerator = kDefaultFramerate; On 2015/02/13 15:38:54, wuchengli wrote: > Let's move setting framerate and make a copy of kDefaultFramerate to > vaapi_wrapper.cc. VaapiWrapper should decide the capability though it's not > supported yet. Done. https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:92: {media::H264PROFILE_BASELINE, VAProfileH264ConstrainedBaseline}, On 2015/02/13 07:35:11, kcwu wrote: > On 2015/02/13 07:15:38, kcwu wrote: > > This is incorrect. You break the original behavior. > > offline talked with henry. The code is good. Please modify comment to reflect > how you deal this issue. > And maybe exchange the field order of ProfileMap. Done. https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:163: DVLOG(1) << "Unsupported profile"; On 2015/02/13 15:38:54, wuchengli wrote: > also print |va_profile| Done. https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:185: DVLOG(1) << "Unsupported profile"; On 2015/02/13 15:38:55, wuchengli wrote: > also print |profile| Done. https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:244: if (va_profiles[j] != VAProfileNone && On 2015/02/13 15:38:54, wuchengli wrote: > Why this can be VAProfileNone? oh...this is typo. In original implementation, we use ProfileToVAProfile to get correct va_profile. https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:248: supported_profile.mode = modes[i]; On 2015/02/13 15:38:54, wuchengli wrote: > move l247-248 to l259. No need to do this if it fails later. Also, the operation > of |supported_profile| should be together. Done. https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:259: &supported_profile.max_resolution)) On 2015/02/13 15:38:55, wuchengli wrote: > This for loop is hard to read. Can you flatten the ifs in the loop? > for (size_t j = 0; ...) { > if (!IsEntrypointSupported(va_profiles[j], entrypoint)) > continue; > if (!AreAttribsSupported(va_profiles[j], entrypoint, required_attribs))) > continue; > ... > VAStatus va_res = vaCreateConfig(); > if (va_res != VA_STATUS_SUCCESS) > continue; > if (!GetVaCodecMaxResolution()) > continue; > supported_profiles.push_back(supported_profile); > } > > Done. https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:421: if (!resolution->height() || !resolution->width()) On 2015/02/13 15:38:54, wuchengli wrote: > This shouldn't happen. Right? Add LOG(ERROR) Done. https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:432: for (i = 0; i < profile_configs.size(); ++i) On 2015/02/13 15:38:54, wuchengli wrote: > This for loops have two lines. Please add braces for readability. I was confused > when I first look at this code. Done. https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.cc:436: return false; On 2015/02/13 15:38:55, wuchengli wrote: > Move line 427-436 to a LazyProfileConfig::IsProfileSupported(CodecMode, > VAProfile). Done. https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:49: struct ProfileConfig { On 2015/02/13 15:38:55, wuchengli wrote: > This name is not good. This is not a VAConfig. This is not a "configuration" in > normal meaning, either. Maybe ProfileInfo? > > Also move this to private. Done. https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:51: VaapiWrapper::CodecMode mode; On 2015/02/13 15:38:55, wuchengli wrote: > This field is redundant. All |mode| of |supported_encode_profiles_| are kEncode > and all |mode| of |supported_decode_profiles_| are kDecode. Remove this > variable. InitSupportedProfileConfigs returns a vector of ProfileConfig including encode and decode. In lasyinstance we dispatch the vector to supported_profiles[mode]. For LasyInstance, this field is redundant. But for VaapiWrapper, this field is required. I prefer to keep this field. https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:77: static std::vector<ProfileConfig> InitSupportedProfileConfigs(); On 2015/02/13 15:38:55, wuchengli wrote: > this should be private. Done. https://codereview.chromium.org/872623002/diff/60001/content/common/gpu/media... content/common/gpu/media/vaapi_wrapper.h:275: class CONTENT_EXPORT LazyProfileConfig { On 2015/02/13 15:38:55, wuchengli wrote: > This is not used by other classes. Move this to vaapi_wrapper.cc and remove > CONTENT_EXPORT. Done.
https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:60: const int kDefaultFramerate = 30; s/kDefaultFramerate/kMaxFramerate/. in VaapiVDA, it's the default (initial) frame rate. Here it's the max framerate in media::VideoEncodeAccelerator::SupportedProfile. Add variable comments. libva doesn't have an API for max framerate. Right? https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:196: std::vector<ProfileInfo> encode_profile_info = s/encode_profile_info/encode_profile_infos/ https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:216: std::vector<VaapiWrapper::ProfileInfo> InitSupportedProfileInfos should be renamed. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:217: VaapiWrapper::InitSupportedProfileInfos() { How about adding CodecMode parameter to InitSupportedProfileInfos and GetSupportedProfileInfos? Then ProfileInfo struct won't need CodecMode. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:239: std::vector<ProfileInfo> supported_profiles; s/supported_profiles/supported_profile_infos/. Let's be consistent with the naming. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:418: switch (attrib_list[i].type) { nit: using if clause can have less code. Up to you. if (attrib_list[i].type == VASurfaceAttribMaxWidth) resolution->set_width(attrib_list[i].value.value.i); else if (attrib_list[i].type == VASurfaceAttribMaxHeight) resolution->set_height(attrib_list[i].value.value.i); https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:430: LOG(ERROR) << "Codec resolution cannot be zero."; also print resolution->ToString(). https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:1045: CHECK(mode < VaapiWrapper::kCodecModeMax); Use DCHECK. See http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED- https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:195: class LazyProfileInfo { s/LazyProfileInfo/LazyProfileInfos/ to distinguish between ProfileInfo and LazyProfileInfo. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:211: static std::vector<ProfileInfo> InitSupportedProfileInfos(); Move this near PostSandboxInitialization(). Static functions should be together. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:221: bool GetVaCodecMaxResolution(VAConfigID config, gfx::Size* resolution); Add function comments. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:241: // Get all supported profile configs. s/configs/infos/ https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:287: static base::LazyInstance<LazyProfileInfo> profile_info_; s/profile_info/profile_infos_/. Add variable comments.
https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:96: {VAProfileH264ConstrainedBaseline, media::H264PROFILE_BASELINE}, Move this below VAProfileH264Baseline so two H264PROFILE_BASELINE are together. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:202: media::VideoCodecProfile hw_profile = VAProfileToProfile( As discussed, this can add two media::H264PROFILE_BASELINE to |profiles|. Do not add it to |profiles| if |profile.profile| already exists.
https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_encode_accelerator.cc:111: if (cmd_line->HasSwitch(switches::kDisableVaapiAcceleratedVideoEncode)) I think we should move this to VaapiWrapper::GetSupportedEncodeProfiles() https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:11: #include "base/lazy_instance.h" Already in the header. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:60: const int kDefaultFramerate = 30; Please add a comment saying that this is an arbitrary limit (not something that we actually took from the HW documentation, etc.). Also, I don't think this is the case with decoder and I would prefer not suggesting an artificial limit there. Perhaps kMaxEncoderFramerate ? https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:96: {VAProfileH264ConstrainedBaseline, media::H264PROFILE_BASELINE}, How does fallback to Constrained work now? https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:189: DVLOG(1) << "Unsupported profile: " << profile; This may also fail if profile is supported, but vaCreateConfig failed. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:403: VAStatus va_res; Please define each variable just before use. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:405: va_res = vaQuerySurfaceAttributes( Please add a comment why we are calling this twice. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:409: std::vector<VASurfaceAttrib> attrib_list( Probably better to check if num_attribs != 0? https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:1043: VaapiWrapper::LazyProfileInfo::GetSupportedProfileInfos( GetSupportedProfileInfosForCodecMode https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:1045: CHECK(mode < VaapiWrapper::kCodecModeMax); On 2015/03/02 07:10:12, wuchengli wrote: > Use DCHECK. See > http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED- I'd prefer if. Also, better to compare against supported_profiles.size(). https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:1051: for (size_t i = 0; i < supported_profiles_[mode].size(); ++i) { for (const auto& profile : supported_profiles_[mode]) if (profile.va_profile == va_profile) return true; Also similarly in other loops in this file. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:51: // Return an instance of VaapiWrapper and initialize by |va_profile| and s/and initialize by/initialized for/ https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:191: VaapiWrapper::CodecMode mode; s/VaapiWrapper::// ? https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:199: std::vector<VaapiWrapper::ProfileInfo> GetSupportedProfileInfos( Are VaapiWrapper:: prefixes needed? https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:221: bool GetVaCodecMaxResolution(VAConfigID config, gfx::Size* resolution); s/config/va_config_id/ Could we just return empty (0,0) gfx::Size on failure? Also, GetMaxResolutionForVAConfigID() perhaps?
https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:429: if (!resolution->height() || !resolution->width()) { resolution->IsEmpty() https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:1033: for (size_t i = 0; i < all_profile_info.size(); ++i) { how about for (const auto& info : all_profile_info)
all done. PTAL https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_video_encode_accelerator.cc:111: if (cmd_line->HasSwitch(switches::kDisableVaapiAcceleratedVideoEncode)) On 2015/03/02 11:00:20, Pawel Osciak wrote: > I think we should move this to VaapiWrapper::GetSupportedEncodeProfiles() Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:11: #include "base/lazy_instance.h" On 2015/03/02 11:00:20, Pawel Osciak wrote: > Already in the header. Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:60: const int kDefaultFramerate = 30; On 2015/03/02 07:10:12, wuchengli wrote: > s/kDefaultFramerate/kMaxFramerate/. in VaapiVDA, it's the default (initial) > frame rate. Here it's the max framerate in > media::VideoEncodeAccelerator::SupportedProfile. > > Add variable comments. > > libva doesn't have an API for max framerate. Right? Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:60: const int kDefaultFramerate = 30; On 2015/03/02 11:00:20, Pawel Osciak wrote: > Please add a comment saying that this is an arbitrary limit (not something that > we actually took from the HW documentation, etc.). > > Also, I don't think this is the case with decoder and I would prefer not > suggesting an artificial limit there. Perhaps kMaxEncoderFramerate ? Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:96: {VAProfileH264ConstrainedBaseline, media::H264PROFILE_BASELINE}, On 2015/03/02 07:28:32, wuchengli wrote: > Move this below VAProfileH264Baseline so two H264PROFILE_BASELINE are together. Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:96: {VAProfileH264ConstrainedBaseline, media::H264PROFILE_BASELINE}, On 2015/03/02 11:00:20, Pawel Osciak wrote: > How does fallback to Constrained work now? Change back to original way. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:189: DVLOG(1) << "Unsupported profile: " << profile; On 2015/03/02 11:00:20, Pawel Osciak wrote: > This may also fail if profile is supported, but vaCreateConfig failed. Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:196: std::vector<ProfileInfo> encode_profile_info = On 2015/03/02 07:10:12, wuchengli wrote: > s/encode_profile_info/encode_profile_infos/ Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:202: media::VideoCodecProfile hw_profile = VAProfileToProfile( On 2015/03/02 07:28:32, wuchengli wrote: > As discussed, this can add two media::H264PROFILE_BASELINE to |profiles|. Do not > add it to |profiles| if |profile.profile| already exists. Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:216: std::vector<VaapiWrapper::ProfileInfo> On 2015/03/02 07:10:12, wuchengli wrote: > InitSupportedProfileInfos should be renamed. Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:217: VaapiWrapper::InitSupportedProfileInfos() { On 2015/03/02 07:10:12, wuchengli wrote: > How about adding CodecMode parameter to InitSupportedProfileInfos and > GetSupportedProfileInfos? Then ProfileInfo struct won't need CodecMode. Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:239: std::vector<ProfileInfo> supported_profiles; On 2015/03/02 07:10:12, wuchengli wrote: > s/supported_profiles/supported_profile_infos/. Let's be consistent with the > naming. Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:403: VAStatus va_res; On 2015/03/02 11:00:20, Pawel Osciak wrote: > Please define each variable just before use. Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:405: va_res = vaQuerySurfaceAttributes( On 2015/03/02 11:00:20, Pawel Osciak wrote: > Please add a comment why we are calling this twice. Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:409: std::vector<VASurfaceAttrib> attrib_list( On 2015/03/02 11:00:20, Pawel Osciak wrote: > Probably better to check if num_attribs != 0? Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:418: switch (attrib_list[i].type) { On 2015/03/02 07:10:12, wuchengli wrote: > nit: using if clause can have less code. Up to you. > > if (attrib_list[i].type == VASurfaceAttribMaxWidth) > resolution->set_width(attrib_list[i].value.value.i); > else if (attrib_list[i].type == VASurfaceAttribMaxHeight) > resolution->set_height(attrib_list[i].value.value.i); Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:429: if (!resolution->height() || !resolution->width()) { On 2015/03/02 14:02:18, kcwu wrote: > resolution->IsEmpty() Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:430: LOG(ERROR) << "Codec resolution cannot be zero."; On 2015/03/02 07:10:12, wuchengli wrote: > also print resolution->ToString(). Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:1033: for (size_t i = 0; i < all_profile_info.size(); ++i) { On 2015/03/02 14:02:18, kcwu wrote: > how about > for (const auto& info : all_profile_info) Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:1043: VaapiWrapper::LazyProfileInfo::GetSupportedProfileInfos( On 2015/03/02 11:00:20, Pawel Osciak wrote: > GetSupportedProfileInfosForCodecMode Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:1045: CHECK(mode < VaapiWrapper::kCodecModeMax); On 2015/03/02 07:10:12, wuchengli wrote: > Use DCHECK. See > http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED- Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:1045: CHECK(mode < VaapiWrapper::kCodecModeMax); On 2015/03/02 11:00:20, Pawel Osciak wrote: > On 2015/03/02 07:10:12, wuchengli wrote: > > Use DCHECK. See > > > http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED- > > I'd prefer if. Also, better to compare against supported_profiles.size(). supported_profiles_ is an array instead of class object and it is initialized with kCodecModeMax elements. So I think checking kCodecModeMax is enough. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:1051: for (size_t i = 0; i < supported_profiles_[mode].size(); ++i) { On 2015/03/02 11:00:20, Pawel Osciak wrote: > for (const auto& profile : supported_profiles_[mode]) > if (profile.va_profile == va_profile) > return true; > > Also similarly in other loops in this file. Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:51: // Return an instance of VaapiWrapper and initialize by |va_profile| and On 2015/03/02 11:00:21, Pawel Osciak wrote: > s/and initialize by/initialized for/ Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:191: VaapiWrapper::CodecMode mode; On 2015/03/02 11:00:20, Pawel Osciak wrote: > s/VaapiWrapper::// ? Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:195: class LazyProfileInfo { On 2015/03/02 07:10:12, wuchengli wrote: > s/LazyProfileInfo/LazyProfileInfos/ to distinguish between ProfileInfo and > LazyProfileInfo. Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:199: std::vector<VaapiWrapper::ProfileInfo> GetSupportedProfileInfos( On 2015/03/02 11:00:21, Pawel Osciak wrote: > Are VaapiWrapper:: prefixes needed? Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:211: static std::vector<ProfileInfo> InitSupportedProfileInfos(); On 2015/03/02 07:10:12, wuchengli wrote: > Move this near PostSandboxInitialization(). Static functions should be together. Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:221: bool GetVaCodecMaxResolution(VAConfigID config, gfx::Size* resolution); On 2015/03/02 07:10:12, wuchengli wrote: > Add function comments. Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:221: bool GetVaCodecMaxResolution(VAConfigID config, gfx::Size* resolution); On 2015/03/02 11:00:20, Pawel Osciak wrote: > s/config/va_config_id/ > > Could we just return empty (0,0) gfx::Size on failure? > > Also, GetMaxResolutionForVAConfigID() perhaps? I prefer to return boolean value to consist with other functions which do VA APIs. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:241: // Get all supported profile configs. On 2015/03/02 07:10:12, wuchengli wrote: > s/configs/infos/ Done. https://codereview.chromium.org/872623002/diff/100001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:287: static base::LazyInstance<LazyProfileInfo> profile_info_; On 2015/03/02 07:10:12, wuchengli wrote: > s/profile_info/profile_infos_/. Add variable comments. Done.
https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:152: VAProfile VaapiWrapper::CheckConstrainedBaselineWorkaround( The name is not descriptive. This function does many things and it's hard to name it. This function also checks if a profile is supported. This should be merged back to ProfileToVAProfile. https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:167: DVLOG(1) << "Falling back to constrained baseline profile."; s/Falling/Fall/ https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:202: return vaapi_wrapper.Pass(); Normally I would expect the end of the function is the success case, not the failure case. Also line 196 early returns the failure case, which is the opposite of line 201. I prefer the original code of returning nullptr here. https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:217: media::VideoEncodeAccelerator::SupportedProfile profile; Move this declaration near where it's first used -- line 225. https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:266: ProfileInfo profile_info; Move this declaration before the first use (line 285) so it's easy to understand the code. https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:276: VAStatus va_res = vaCreateConfig( need va_lock_ for this call. https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:283: if (va_res != VA_STATUS_SUCCESS) Print a DVLOG here. When can this fail? If this should never fail, use LOG(ERROR) instead. https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:1049: for (const auto& mode : modes) { Better to iterate from 0 to kCodecModeMax - 1 because the size of supported_profiles_ is kCodecModeMax. And we don't need to declare {kDecode, kEncode} again, which duplicates the order of enum. https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:1061: if (mode < kCodecModeMax) No need to have this check. GetSupportedProfileInfosForCodecMode is internal to vaapi_wrapper.cc. All calls to this function clearly won't be out-of-bound. It's the same that we don't validate |mode| in IsProfileSupported. https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:73: // and it is not supported. We try constrained baseline. s/supported. We/supported, we/ https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:74: static VAProfile CheckConstrainedBaselineWorkaround( This should be moved to private.
https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:152: VAProfile VaapiWrapper::CheckConstrainedBaselineWorkaround( On 2015/03/04 04:03:18, wuchengli wrote: > The name is not descriptive. This function does many things and it's hard to > name it. This function also checks if a profile is supported. This should be > merged back to ProfileToVAProfile. For example, this function also checks if a profile is supported and returns VAProfileNone if unsupported.
https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:152: VAProfile VaapiWrapper::CheckConstrainedBaselineWorkaround( On 2015/03/04 04:03:18, wuchengli wrote: > The name is not descriptive. This function does many things and it's hard to > name it. This function also checks if a profile is supported. This should be > merged back to ProfileToVAProfile. Done. https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:167: DVLOG(1) << "Falling back to constrained baseline profile."; On 2015/03/04 04:03:18, wuchengli wrote: > s/Falling/Fall/ Done. https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:202: return vaapi_wrapper.Pass(); On 2015/03/04 04:03:18, wuchengli wrote: > Normally I would expect the end of the function is the success case, not the > failure case. Also line 196 early returns the failure case, which is the > opposite of line 201. I prefer the original code of returning nullptr here. Done. https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:217: media::VideoEncodeAccelerator::SupportedProfile profile; On 2015/03/04 04:03:18, wuchengli wrote: > Move this declaration near where it's first used -- line 225. Done. https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:266: ProfileInfo profile_info; On 2015/03/04 04:03:18, wuchengli wrote: > Move this declaration before the first use (line 285) so it's easy to understand > the code. Done. https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:276: VAStatus va_res = vaCreateConfig( On 2015/03/04 04:03:18, wuchengli wrote: > need va_lock_ for this call. Done. https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:283: if (va_res != VA_STATUS_SUCCESS) On 2015/03/04 04:03:18, wuchengli wrote: > Print a DVLOG here. When can this fail? If this should never fail, use > LOG(ERROR) instead. Done. https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:1049: for (const auto& mode : modes) { On 2015/03/04 04:03:18, wuchengli wrote: > Better to iterate from 0 to kCodecModeMax - 1 because the size of > supported_profiles_ is kCodecModeMax. And we don't need to declare {kDecode, > kEncode} again, which duplicates the order of enum. Done. https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:1061: if (mode < kCodecModeMax) On 2015/03/04 04:03:18, wuchengli wrote: > No need to have this check. GetSupportedProfileInfosForCodecMode is internal to > vaapi_wrapper.cc. All calls to this function clearly won't be out-of-bound. It's > the same that we don't validate |mode| in IsProfileSupported. Done. https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:73: // and it is not supported. We try constrained baseline. On 2015/03/04 04:03:18, wuchengli wrote: > s/supported. We/supported, we/ Done. https://codereview.chromium.org/872623002/diff/120001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:74: static VAProfile CheckConstrainedBaselineWorkaround( On 2015/03/04 04:03:18, wuchengli wrote: > This should be moved to private. Done.
Looking good. Only some small comments. https://codereview.chromium.org/872623002/diff/140001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/872623002/diff/140001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:263: base::AutoLock auto_lock(va_lock_); We cannot hold |va_lock_| when calling the functions that will try getting |va_lock_| -- IsEntrypointSupported and AreAttribsSupported. Better to move vaCreateConfig to GetMaxResolutionForVAConfigID. Or make IsEntrypointSupported, AreAttribsSupported, and GetMaxResolutionForVAConfigID locked functions like InitializeVpp_Locked. https://codereview.chromium.org/872623002/diff/140001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:1047: VaapiWrapper::GetSupportedProfileInfosForCodecMode( It's confusing both VaapiWrapper and LazyProfileInfos have GetSupportedProfileInfosForCodecMode. VaapiWrapper calls LazyProfileInfos::GetSupportedProfileInfosForCodecMode and LazyProfileInfos calls VaapiWrapper::GetSupportedProfileInfosForCodecMode. But I cannot think of a better way. https://codereview.chromium.org/872623002/diff/140001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/872623002/diff/140001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. The CL title is "Add GetMaxResolutionForVAConfigID in vaapi". I think the main thing this CL does is removing the hard-coded max encoder resolution and getting it from libva. GetMaxResolutionForVAConfigID is internal to vappi wrapper and doesn't provide much context. How about using "VaapiVEA: get max encode resolution from libva" for CL title? https://codereview.chromium.org/872623002/diff/140001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:239: // Get all supported profile infos. Update the comments. It's not all anymore. Same for line 247. https://codereview.chromium.org/872623002/diff/140001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:252: // Maps Profile enum values to VaProfile values.This function is including s/Maps/Map/ to be consistent with other comments. s/Profile/VideoCodecProfile/ s/VaProfile/VAProfile/ s/values.This/values. This/ s/is including/includes a workaround for/ https://codereview.chromium.org/872623002/diff/140001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:256: media::VideoCodecProfile profile, line 256 and 257 should fit in one line. Run git cl format.
https://codereview.chromium.org/872623002/diff/140001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/872623002/diff/140001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:263: base::AutoLock auto_lock(va_lock_); On 2015/03/04 08:45:55, wuchengli wrote: > We cannot hold |va_lock_| when calling the functions that will try getting > |va_lock_| -- IsEntrypointSupported and AreAttribsSupported. Better to move > vaCreateConfig to GetMaxResolutionForVAConfigID. Or make IsEntrypointSupported, > AreAttribsSupported, and GetMaxResolutionForVAConfigID locked functions like > InitializeVpp_Locked. Done. https://codereview.chromium.org/872623002/diff/140001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/872623002/diff/140001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2015/03/04 08:45:56, wuchengli wrote: > The CL title is "Add GetMaxResolutionForVAConfigID in vaapi". I think the main > thing this CL does is removing the hard-coded max encoder resolution and getting > it from libva. GetMaxResolutionForVAConfigID is internal to vappi wrapper and > doesn't provide much context. How about using "VaapiVEA: get max encode > resolution from libva" for CL title? Done. https://codereview.chromium.org/872623002/diff/140001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:239: // Get all supported profile infos. On 2015/03/04 08:45:56, wuchengli wrote: > Update the comments. It's not all anymore. Same for line 247. Done. https://codereview.chromium.org/872623002/diff/140001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:252: // Maps Profile enum values to VaProfile values.This function is including On 2015/03/04 08:45:56, wuchengli wrote: > s/Maps/Map/ to be consistent with other comments. > s/Profile/VideoCodecProfile/ > s/VaProfile/VAProfile/ > s/values.This/values. This/ > s/is including/includes a workaround for/ Done. https://codereview.chromium.org/872623002/diff/140001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:256: media::VideoCodecProfile profile, On 2015/03/04 08:45:56, wuchengli wrote: > line 256 and 257 should fit in one line. Run git cl format. Done.
https://codereview.chromium.org/872623002/diff/160001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/872623002/diff/160001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:212: bool IsEntrypointSupported(VAProfile va_profile, VAEntrypoint entrypoint); Add a comment to say |va_lock_| must be held on entry. http://www.chromium.org/developers/lock-and-condition-variable Each routine should have a comment indicating which mutexes must and must not be held on entry. This allows implementors to edit routines without examining their call sites, and allows clients to use routines without reading their bodies. https://codereview.chromium.org/872623002/diff/160001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:213: bool AreAttribsSupported(VAProfile va_profile, Add a comment to say |va_lock_| must be held on entry. https://codereview.chromium.org/872623002/diff/160001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:218: // |resolution| is the maximum resolution. Add a comment to say |va_lock_| must be held on entry.
https://codereview.chromium.org/872623002/diff/160001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/872623002/diff/160001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:212: bool IsEntrypointSupported(VAProfile va_profile, VAEntrypoint entrypoint); On 2015/03/05 03:40:28, wuchengli wrote: > Add a comment to say |va_lock_| must be held on entry. > > http://www.chromium.org/developers/lock-and-condition-variable > Each routine should have a comment indicating which mutexes must and must not be > held on entry. This allows implementors to edit routines without examining their > call sites, and allows clients to use routines without reading their bodies. Done. https://codereview.chromium.org/872623002/diff/160001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:213: bool AreAttribsSupported(VAProfile va_profile, On 2015/03/05 03:40:29, wuchengli wrote: > Add a comment to say |va_lock_| must be held on entry. Done. https://codereview.chromium.org/872623002/diff/160001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:218: // |resolution| is the maximum resolution. On 2015/03/05 03:40:28, wuchengli wrote: > Add a comment to say |va_lock_| must be held on entry. Done.
lgtm https://codereview.chromium.org/872623002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/872623002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:213: // Check |va_profile| support |entrypoint| or not. |va_lock_| must be held on s/Check |va_profile| support/Check if |va_profile| supports/ https://codereview.chromium.org/872623002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:217: // Check |va_profile| support |required_attribs| or not. |va_lock_| must be Checks if |va_profile| / |entrypoint| pair supports
https://chromiumcodereview.appspot.com/872623002/diff/180001/content/common/g... File content/common/gpu/media/vaapi_wrapper.cc (right): https://chromiumcodereview.appspot.com/872623002/diff/180001/content/common/g... content/common/gpu/media/vaapi_wrapper.cc:150: DVLOG(1) << "Unsupported va profile: " << va_profile; Initialize() may fail for other reasons too. Perhaps move this log and one at l.168 to Initialize() itself? https://chromiumcodereview.appspot.com/872623002/diff/180001/content/common/g... content/common/gpu/media/vaapi_wrapper.cc:198: } Could we break here? https://chromiumcodereview.appspot.com/872623002/diff/180001/content/common/g... content/common/gpu/media/vaapi_wrapper.cc:282: if (!GetMaxResolutionForVAConfigID(config_id, &profile_info.max_resolution)) I think we should LOG(ERROR) here, failure here may make us silently ignore some profiles, which we would've supported otherwise. This may be important if more profiles are added, but we don't add their resolutions to libva. https://chromiumcodereview.appspot.com/872623002/diff/180001/content/common/g... content/common/gpu/media/vaapi_wrapper.cc:424: va_display_, va_config_id, NULL, &num_attribs); s/NULL/nullptr/ https://chromiumcodereview.appspot.com/872623002/diff/180001/content/common/g... content/common/gpu/media/vaapi_wrapper.cc:437: for (size_t i = 0; i < num_attribs; i++) { for (const auto& attrib : attrib_list) https://chromiumcodereview.appspot.com/872623002/diff/180001/content/common/g... content/common/gpu/media/vaapi_wrapper.cc:1045: for (size_t i = 0; i < kCodecModeMax; ++i) { Ok, but let's at least do static_assert(arraysize(supported_profiles_), kCodecModeMax) https://chromiumcodereview.appspot.com/872623002/diff/180001/content/common/g... File content/common/gpu/media/vaapi_wrapper.h (right): https://chromiumcodereview.appspot.com/872623002/diff/180001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:215: bool IsEntrypointSupported(VAProfile va_profile, VAEntrypoint entrypoint); s/IsEntrypointSupported/IsEntrypointSupported_Locked/ https://chromiumcodereview.appspot.com/872623002/diff/180001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:219: bool AreAttribsSupported(VAProfile va_profile, s/AreAttribsSupported/AreAttribsSupported_Locked/ https://chromiumcodereview.appspot.com/872623002/diff/180001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:223: // Get maximum resolution of a profile by |config|. If return value is true, s/by |config|/for |va_config_id|/ https://chromiumcodereview.appspot.com/872623002/diff/180001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:255: // GetSupportedProfileInfosForCodecModeInternal for static usage. It looks like GetSupportedProfileInfosForCodecModeInternal() is never called outside of GetSupportedProfileInfosForCodecMode(), so the body of Internal could be put back into GetSupportedProfileInfosForCodecMode()?
all done. https://codereview.chromium.org/872623002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/872623002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:150: DVLOG(1) << "Unsupported va profile: " << va_profile; On 2015/03/08 00:58:24, Pawel Osciak wrote: > Initialize() may fail for other reasons too. Perhaps move this log and one at > l.168 to Initialize() itself? Done. https://codereview.chromium.org/872623002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:198: } On 2015/03/08 00:58:24, Pawel Osciak wrote: > Could we break here? Done. https://codereview.chromium.org/872623002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:282: if (!GetMaxResolutionForVAConfigID(config_id, &profile_info.max_resolution)) On 2015/03/08 00:58:24, Pawel Osciak wrote: > I think we should LOG(ERROR) here, failure here may make us silently ignore some > profiles, which we would've supported otherwise. This may be important if more > profiles are added, but we don't add their resolutions to libva. Done. https://codereview.chromium.org/872623002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:424: va_display_, va_config_id, NULL, &num_attribs); On 2015/03/08 00:58:24, Pawel Osciak wrote: > s/NULL/nullptr/ Done. https://codereview.chromium.org/872623002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:437: for (size_t i = 0; i < num_attribs; i++) { On 2015/03/08 00:58:24, Pawel Osciak wrote: > for (const auto& attrib : attrib_list) Done. https://codereview.chromium.org/872623002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:1045: for (size_t i = 0; i < kCodecModeMax; ++i) { On 2015/03/08 00:58:24, Pawel Osciak wrote: > Ok, but let's at least do static_assert(arraysize(supported_profiles_), > kCodecModeMax) Done. https://codereview.chromium.org/872623002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/872623002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:213: // Check |va_profile| support |entrypoint| or not. |va_lock_| must be held on On 2015/03/05 16:24:21, wuchengli wrote: > s/Check |va_profile| support/Check if |va_profile| supports/ Done. https://codereview.chromium.org/872623002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:215: bool IsEntrypointSupported(VAProfile va_profile, VAEntrypoint entrypoint); On 2015/03/08 00:58:24, Pawel Osciak wrote: > s/IsEntrypointSupported/IsEntrypointSupported_Locked/ Done. https://codereview.chromium.org/872623002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:217: // Check |va_profile| support |required_attribs| or not. |va_lock_| must be On 2015/03/05 16:24:21, wuchengli wrote: > Checks if |va_profile| / |entrypoint| pair supports Done. https://codereview.chromium.org/872623002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:219: bool AreAttribsSupported(VAProfile va_profile, On 2015/03/08 00:58:24, Pawel Osciak wrote: > s/AreAttribsSupported/AreAttribsSupported_Locked/ Done. https://codereview.chromium.org/872623002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:223: // Get maximum resolution of a profile by |config|. If return value is true, On 2015/03/08 00:58:24, Pawel Osciak wrote: > s/by |config|/for |va_config_id|/ Done. https://codereview.chromium.org/872623002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:255: // GetSupportedProfileInfosForCodecModeInternal for static usage. On 2015/03/08 00:58:24, Pawel Osciak wrote: > It looks like GetSupportedProfileInfosForCodecModeInternal() is never called > outside of GetSupportedProfileInfosForCodecMode(), so the body of Internal could > be put back into GetSupportedProfileInfosForCodecMode()? I prefer not. Because GetSupportedProfileInfosForCodecMode is static function. If we put the body of Internal to the function, all used functions should be static function as well.
https://codereview.chromium.org/872623002/diff/180001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/872623002/diff/180001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:255: // GetSupportedProfileInfosForCodecModeInternal for static usage. On 2015/03/09 03:56:52, henryhsu wrote: > On 2015/03/08 00:58:24, Pawel Osciak wrote: > > It looks like GetSupportedProfileInfosForCodecModeInternal() is never called > > outside of GetSupportedProfileInfosForCodecMode(), so the body of Internal > could > > be put back into GetSupportedProfileInfosForCodecMode()? > > I prefer not. Because GetSupportedProfileInfosForCodecMode is static function. > If we put the body of Internal to the function, all used functions should be > static function as well. Can you move the body of GetSupportedProfileInfosForCodecMode to the contrustor of LazyProfileInfos? Then GetSupportedProfileInfosForCodecModeInternal can be renamed to GetSupportedProfileInfosForCodecMode.
https://codereview.chromium.org/872623002/diff/200001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/872623002/diff/200001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:268: VAStatus va_res = vaCreateConfig( Pawel and I discussed. vaCreateConfig should be moved into GetMaxResolutionForVAConfigID because all other code in GetSupportedProfileInfosForCodecModeInternal does not call va functions directly. s/GetMaxResolutionForVAConfigID/GetMaxResolution_Locked/
https://codereview.chromium.org/872623002/diff/200001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/872623002/diff/200001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:268: VAStatus va_res = vaCreateConfig( On 2015/03/09 05:17:30, wuchengli wrote: > Pawel and I discussed. vaCreateConfig should be moved into > GetMaxResolutionForVAConfigID because all other code in > GetSupportedProfileInfosForCodecModeInternal does not call va functions > directly. > > s/GetMaxResolutionForVAConfigID/GetMaxResolution_Locked/ Done.
lgtm and one small comment https://codereview.chromium.org/872623002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/872623002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:230: std::vector<VAConfigAttrib>& required_attribs, add const
lgtm % nits https://chromiumcodereview.appspot.com/872623002/diff/220001/content/common/g... File content/common/gpu/media/vaapi_wrapper.cc (right): https://chromiumcodereview.appspot.com/872623002/diff/220001/content/common/g... content/common/gpu/media/vaapi_wrapper.cc:263: LOG(ERROR) << "GetMaxResolution failed by va_profile " << va_profile; s/by/for/ Also please add entrypoint to the message so that we know if it was encoder or decoder failing. https://chromiumcodereview.appspot.com/872623002/diff/220001/content/common/g... content/common/gpu/media/vaapi_wrapper.cc:404: va_res = vaCreateConfig( Nit: VAStatus va_res = ... https://chromiumcodereview.appspot.com/872623002/diff/220001/content/common/g... File content/common/gpu/media/vaapi_wrapper.h (right): https://chromiumcodereview.appspot.com/872623002/diff/220001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:218: // Check if |va_profile| supports |required_attribs| or not. |va_lock_| must Return true if |va_profile| for |entrypoint| with |required_attribs| is supported. https://chromiumcodereview.appspot.com/872623002/diff/220001/content/common/g... content/common/gpu/media/vaapi_wrapper.h:225: // Get maximum resolution of |va_profile|. If return value is true, s/of |va_profile|/for |va_profile| and |entrypoint| with |required_attribs|/
https://codereview.chromium.org/872623002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/872623002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:263: LOG(ERROR) << "GetMaxResolution failed by va_profile " << va_profile; On 2015/03/10 03:41:14, Pawel Osciak wrote: > s/by/for/ > Also please add entrypoint to the message so that we know if it was encoder or > decoder failing. Done. https://codereview.chromium.org/872623002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.cc:404: va_res = vaCreateConfig( On 2015/03/10 03:41:14, Pawel Osciak wrote: > Nit: VAStatus va_res = ... Done. https://codereview.chromium.org/872623002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/872623002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:218: // Check if |va_profile| supports |required_attribs| or not. |va_lock_| must On 2015/03/10 03:41:14, Pawel Osciak wrote: > Return true if |va_profile| for |entrypoint| with |required_attribs| is > supported. Done. https://codereview.chromium.org/872623002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:225: // Get maximum resolution of |va_profile|. If return value is true, On 2015/03/10 03:41:14, Pawel Osciak wrote: > s/of |va_profile|/for |va_profile| and |entrypoint| with |required_attribs|/ Done. https://codereview.chromium.org/872623002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:230: std::vector<VAConfigAttrib>& required_attribs, On 2015/03/09 11:14:53, wuchengli wrote: > add const Done.
The CQ bit was checked by henryhsu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wuchengli@chromium.org, posciak@chromium.org Link to the patchset: https://codereview.chromium.org/872623002/#ps260001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/872623002/260001
lgtm https://codereview.chromium.org/872623002/diff/220001/content/common/gpu/medi... File content/common/gpu/media/vaapi_wrapper.h (right): https://codereview.chromium.org/872623002/diff/220001/content/common/gpu/medi... content/common/gpu/media/vaapi_wrapper.h:230: std::vector<VAConfigAttrib>& required_attribs, On 2015/03/10 05:36:53, henryhsu wrote: > On 2015/03/09 11:14:53, wuchengli wrote: > > add const > > Done. Discussed with Henry. vaCreateConfig needs non-const. So this cannot be const.
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/a598f3adabbe02f427e2233239cdead68cf21d4f Cr-Commit-Position: refs/heads/master@{#319842} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
