Issue 603153002: vaapi: detect supported profiles in runtime.

Issue 603153002: vaapi: detect supported profiles in runtime. (Closed)

Created:
6 years, 2 months ago by wuchengli

Modified:
6 years, 2 months ago

Reviewers:
kcwu, Pawel Osciak

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, henryhsu

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Project:
chromium

Visibility:
Public.

More Reviews

Description

vaapi: detect supported profiles in runtime. BUG=350197, 402182 TEST=Run vainfo and VEA unittest on various Intel platforms. Committed: https://crrev.com/e0e2bda200a1474e5d1930659b7066776f84d8ae Cr-Commit-Position: refs/heads/master@{#296925}

Patch Set 1 : #

Total comments: 12

Patch Set 2 : address review comments #

Patch Set 3 : fix a bug in IsProfileSupported #

Patch Set 4 : fix a comment #

Total comments: 1

Patch Set 5 : change IsProfileSupported back to IsEntrypointSupported and IsAttribSupported #

Patch Set 6 : rename to AreAttribsSupported and add va_lock_ to TryToSetVADisplayAttributeToLocalGPU #

Created: 6 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+141 lines, -63 lines)			Patch
M	content/common/gpu/media/vaapi_video_encode_accelerator.cc	View	1	1 chunk	+9 lines, -11 lines	0 comments	Download
M	content/common/gpu/media/vaapi_wrapper.h	View	1 2 3 4 5	2 chunks	+13 lines, -1 line	0 comments	Download
M	content/common/gpu/media/vaapi_wrapper.cc	View	1 2 3 4 5	7 chunks	+119 lines, -51 lines	0 comments	Download

Messages

Total messages: 18 (5 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Pawel Osciak

https://chromiumcodereview.appspot.com/603153002/diff/20001/content/common/gpu/media/vaapi_video_encode_accelerator.cc File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://chromiumcodereview.appspot.com/603153002/diff/20001/content/common/gpu/media/vaapi_video_encode_accelerator.cc#newcode113 content/common/gpu/media/vaapi_video_encode_accelerator.cc:113: std::vector<media::VideoCodecProfile> codecs = maybe: s/codecs/hw_profiles/ ? https://chromiumcodereview.appspot.com/603153002/diff/20001/content/common/gpu/media/vaapi_wrapper.cc File content/common/gpu/media/vaapi_wrapper.cc ...

6 years, 2 months ago (2014-09-26 07:15:12 UTC) #4

kcwu

https://codereview.chromium.org/603153002/diff/20001/content/common/gpu/media/vaapi_video_encode_accelerator.cc File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/603153002/diff/20001/content/common/gpu/media/vaapi_video_encode_accelerator.cc#newcode119 content/common/gpu/media/vaapi_video_encode_accelerator.cc:119: profile.max_resolution.SetSize(1920, 1088); Is this detectable? https://codereview.chromium.org/603153002/diff/20001/content/common/gpu/media/vaapi_wrapper.cc File content/common/gpu/media/vaapi_wrapper.cc (right): ...

6 years, 2 months ago (2014-09-26 07:50:46 UTC) #5

Pawel Osciak

6 years, 2 months ago (2014-09-26 07:52:27 UTC) #6

wuchengli

PTAL. https://codereview.chromium.org/603153002/diff/20001/content/common/gpu/media/vaapi_video_encode_accelerator.cc File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right): https://codereview.chromium.org/603153002/diff/20001/content/common/gpu/media/vaapi_video_encode_accelerator.cc#newcode113 content/common/gpu/media/vaapi_video_encode_accelerator.cc:113: std::vector<media::VideoCodecProfile> codecs = On 2014/09/26 07:15:12, Pawel Osciak ...

6 years, 2 months ago (2014-09-26 09:02:29 UTC) #8

PTAL.

https://codereview.chromium.org/603153002/diff/20001/content/common/gpu/media...
File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right):

https://codereview.chromium.org/603153002/diff/20001/content/common/gpu/media...
content/common/gpu/media/vaapi_video_encode_accelerator.cc:113:
std::vector<media::VideoCodecProfile> codecs =
On 2014/09/26 07:15:12, Pawel Osciak wrote:
> maybe:
> 
> s/codecs/hw_profiles/ ?
Done.

The construction of VEA::SupportedProfiles is done here because I need to use
kDefaultFramerate. If it's OK to duplicate kDefaultFramerate, the naming (codecs
vs profiles) will be less confusing.

https://codereview.chromium.org/603153002/diff/20001/content/common/gpu/media...
File content/common/gpu/media/vaapi_wrapper.cc (right):

https://codereview.chromium.org/603153002/diff/20001/content/common/gpu/media...
content/common/gpu/media/vaapi_wrapper.cc:162: media::VideoCodecProfile
profiles[3] = {media::H264PROFILE_BASELINE,
On 2014/09/26 07:15:12, Pawel Osciak wrote:
> Ideally we'd like to use all va_profiles and use something like
> VAProfileToVideoCodecProfile, but the baseline thing is making it difficult.
> But I'd like to eliminate this hardcoded list from here at least, we have it
in
> ProfileToVaProfile already. How about we iterate over all VideoCodecProfile
> instead? It'd be a bit inefficient to do this for 20 profiles instead of 3,
but
> maybe worth it in order to contain this list to ProfileToVaProfile only?
GetSupportedProfiles is called when GPU process starts, so it has to be as fast
as possible. I combined the hardcoded list here and in ProfileToVaProfile.

https://codereview.chromium.org/603153002/diff/20001/content/common/gpu/media...
content/common/gpu/media/vaapi_wrapper.cc:175: VAProfile va_profile =
ProfileToVAProfile(profiles[i], va_profiles);
On 2014/09/26 07:15:12, Pawel Osciak wrote:
> We have this chunk of code duplicated here and in Initialize. Can we wrap this
> in something like
> 
> bool IsProfileSupported(VideoCodecProfile, CodecMode) ?
Done.

vaCreateConfig needs VA profile, entrypoint, and required attribs. So I changed
the prototype. Also, it's more efficient not to call vaQueryConfigProfiles
multiple times because GetSupportedProfiles is called when GPU process starts.

https://codereview.chromium.org/603153002/diff/20001/content/common/gpu/media...
content/common/gpu/media/vaapi_wrapper.cc:328: VAStatus va_res =
vaCreateConfig(va_display_,
On 2014/09/26 07:50:45, kcwu wrote:
> va_lock_ for this?

Done. Thanks for catching this.

https://codereview.chromium.org/603153002/diff/20001/content/common/gpu/media...
File content/common/gpu/media/vaapi_wrapper.h (right):

https://codereview.chromium.org/603153002/diff/20001/content/common/gpu/media...
content/common/gpu/media/vaapi_wrapper.h:54: static
std::vector<media::VideoCodecProfile> GetSupportedProfiles(
On 2014/09/26 07:15:12, Pawel Osciak wrote:
> GetSupportedEncodeProfiles

Done.

kcwu

https://codereview.chromium.org/603153002/diff/100001/content/common/gpu/media/vaapi_wrapper.cc File content/common/gpu/media/vaapi_wrapper.cc (right): https://codereview.chromium.org/603153002/diff/100001/content/common/gpu/media/vaapi_wrapper.cc#newcode71 content/common/gpu/media/vaapi_wrapper.cc:71: {media::H264PROFILE_HIGH, VAProfileH264High}}; how about }; in next line

6 years, 2 months ago (2014-09-26 10:06:14 UTC) #9

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/603153002/150001

6 years, 2 months ago (2014-09-26 11:15:30 UTC) #16

commit-bot: I haz the power

Committed patchset #6 (id:150001) as d76b62016af4b1d35f1d372154b8a91b00a2751e

6 years, 2 months ago (2014-09-26 12:27:03 UTC) #17

commit-bot: I haz the power

6 years, 2 months ago (2014-09-26 12:28:12 UTC) #18

Message was sent while issue was closed.

Patchset 6 (id:??) landed as
https://crrev.com/e0e2bda200a1474e5d1930659b7066776f84d8ae
Cr-Commit-Position: refs/heads/master@{#296925}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages