|
|
Created:
7 years, 6 months ago by haltonhuo Modified:
7 years, 6 months ago Reviewers:
Ami GONE FROM CHROMIUM CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, apatrick_chromium, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionH.264 decoder should dlopen SONAME of libva and libva-x11.
libva.so and libva-x11.so normally belongs to -dev package, should use
SONAME instead.
vaSyncSurface resides in libva, so use vaapi_handle as handle.
BUG=244364
TEST=ppapi_example_video_decode
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203386
Patch Set 1 #Patch Set 2 : remove RTLD_GLOBAL and modify log #Patch Set 3 : rework after based on r203343 #Messages
Total messages: 30 (0 generated)
No reviewers yet.
One more comment about the change not mentioned in changelog - VAAPI_DLSYM(SyncSurface, vaapi_x11_handle); + VAAPI_DLSYM(SyncSurface, vaapi_handle); It is because vaSyncSurface reside in libva.so.1, not in libva-x11.so.1. $readelf --dyn-sym /usr/lib/x86_64-linux-gnu/libva.so.1|grep vaSyncSurface 109: 00000000000042a0 108 FUNC GLOBAL DEFAULT 11 vaSyncSurface
I don't think this CL should be committed, for reasons listed below. 1) As https://code.google.com/p/chromium/wiki/LinuxHWVideoDecode describes, running a chromeos=1 build on linux to test VAAPI integration is a developer tool, not a reasonable thing to do on end-user systems. As such, requiring a -dev package be installed is seems fine to me. In particular note that VAAPI is _not_ supported on linux hosts (https://code.google.com/p/chromium/issues/detail?id=137247). 2) The RTLD_GLOBAL change looks like a hack; I suspect the real fix is to build libva-x11.so.1 so that it declares its dependency on libva.so.1 explicitly. 3) I suspect that the change of which lib to load SyncSurface from will break all shipping cros/x86 platforms. How was this tested?
On 2013/05/28 16:56:07, Ami Fischman wrote: > I don't think this CL should be committed, for reasons listed below. > > 1) As https://code.google.com/p/chromium/wiki/LinuxHWVideoDecode describes, > running a chromeos=1 build on linux to test VAAPI integration is a developer > tool, not a reasonable thing to do on end-user systems. As such, requiring a > -dev package be installed is seems fine to me. In particular note that VAAPI is > _not_ supported on linux hosts > (https://code.google.com/p/chromium/issues/detail?id=137247). > I think we may want to switch to so.1. This is ok. > 2) The RTLD_GLOBAL change looks like a hack; I suspect the real fix is to build > libva-x11.so.1 so that it declares its dependency on libva.so.1 explicitly. > Not an expert on this, but reading man for it I'd agree with what you are saying. In any case, we build them both together, so I'm not sure if we need the dependency. > 3) I suspect that the change of which lib to load SyncSurface from will break > all shipping cros/x86 platforms. How was this tested? I'm actually surprised to see that the non-x11 version contains that symbol. This is weird, we must've had it in x11 at some point, although I can't find that in libva history. In any case, I'll test this later in the day and I think we should allow this.
On 2013/05/28 18:01:07, posciak wrote: > I think we may want to switch to so.1. This is ok. Thanks for understanding, I'll keep that change. > > > 2) The RTLD_GLOBAL change looks like a hack; I suspect the real fix is to > build > > libva-x11.so.1 so that it declares its dependency on libva.so.1 explicitly. > > > > Not an expert on this, but reading man for it I'd agree with what you are > saying. In any case, we build them both together, so I'm not sure if we need the > dependency. Run `ldd libva-x11.so.1 |grep libva` under following OS: ChromeOS-Vanilla-04028.0.2013_04_20_1810_r706c41144: Yes Ubuntu 13.04 64 bit: No So I'd like to report a bug for Ubuntu. For the time being, to allow developer build/run ChromeOS under Ubuntu 13.04, can we leave a FIXME here? > > > 3) I suspect that the change of which lib to load SyncSurface from will break > > all shipping cros/x86 platforms. How was this tested? > > I'm actually surprised to see that the non-x11 version contains that symbol. > This is weird, we must've had it in x11 at some point, although I can't find > that in libva history. In any case, I'll test this later in the day and I think > we should allow this. I tested on my Ubuntu 13.04/12.10. I was wishing to check whether SyncSurface reside in libva.so.1 on ChromeOS-Vanilla-04028.0.2013_04_20_1810_r706c41144, but I do not have readelf under shell. I'll try to copy a 32bit version readelf to give a try. Or you guys have better idea? And I will check other distros like Fedora, will come back later.
On Tue, May 28, 2013 at 7:23 PM, <halton.huo@intel.com> wrote: > On 2013/05/28 18:01:07, posciak wrote: > >> I think we may want to switch to so.1. This is ok. >> > Thanks for understanding, I'll keep that change. Halton/Pawel: can either of you flesh out the explanation for why it is right to depend on a specific version of the .so, and further why this particular version? IOW, why libva.so.1 and not libva.so.1.3200.0, or any other version ? > > 2) The RTLD_GLOBAL change looks like a hack; I suspect the real fix is >> to build >> > libva-x11.so.1 so that it declares its dependency on libva.so.1 >> explicitly. >> > Not an expert on this, but reading man for it I'd agree with what you are >> saying. In any case, we build them both together, so I'm not sure if we >> need the > > dependency. >> > Run `ldd libva-x11.so.1 |grep libva` under following OS: > ChromeOS-Vanilla-04028.0.2013_**04_20_1810_r706c41144: Yes > Ubuntu 13.04 64 bit: No > So I'd like to report a bug for Ubuntu. For the time being, to allow > developer > build/run ChromeOS under Ubuntu 13.04, can we leave a FIXME here? Developers can just as well build libva correctly for their dev systems. Since this is not a supported setup I'd rather not bake into chrome a workaround for an ubuntu bug. Is there a setup I'm missing where it's important to use the system-provided (buggy) libva? > 3) I suspect that the change of which lib to load SyncSurface from will >> break > > > all shipping cros/x86 platforms. How was this tested? > > I'm actually surprised to see that the non-x11 version contains that >> symbol. >> This is weird, we must've had it in x11 at some point, although I can't >> find >> that in libva history. In any case, I'll test this later in the day and I >> think > > we should allow this. >> > I tested on my Ubuntu 13.04/12.10. I was wishing to check whether > SyncSurface > reside in libva.so.1 on ChromeOS-Vanilla-04028.0.2013_**04_20_1810_r706c41144, > but > I do not have readelf under shell. I'll try to copy a 32bit version > readelf to > give a try. Or you guys have better idea? And I will check other distros > like > Fedora, will come back later. TBH "the symbol belongs in libfoo and not libbar AND is available in libfoo on cros/x86" would be far more convincing than any findings about Fedora. I'll defer to Pawel on the question of "belongs" since he's far more familiar than me with the breakdown of the libva sources.
On 2013/05/29 05:30:15, Ami Fischman wrote: > Halton/Pawel: can either of you flesh out the explanation for why it is > right to depend on a specific version of the .so, and further why this > particular version? > IOW, why libva.so.1 and not libva.so.1.3200.0, or any other version ? libva.so.1 is SONAME of libva.so.1.3200.0. And the non-version one(libva.so) is normally separate shipped with -dev pkg. My understanding and experience is upper executable or library should depend on SONAME, not the real library or non-version one. Refer link http://tldp.org/HOWTO/Program-Library-HOWTO/shared-libraries.html > > Developers can just as well build libva correctly for their dev systems. > Since this is not a supported setup I'd rather not bake into chrome a > workaround for an ubuntu bug. > Is there a setup I'm missing where it's important to use the > system-provided (buggy) libva? I've check Fedora 18, the libva-x11 explicitly linked with libva. So far it should Ubuntu only bug. Adding this hacking of RTLD_GLOBAL is more friendly to Ubuntu developers. (It won't need user to rebuild its own libva libraries). But I'm okay to keep dlopen("libva.so", RTLD_NOW) untouched if you insist. BTW, the ubuntu bug is reported at https://bugs.launchpad.net/ubuntu/+source/libva/+bug/1185298 > TBH "the symbol belongs in libfoo and not libbar AND is available in libfoo > on cros/x86" would be far more convincing than any findings about Fedora. > I'll defer to Pawel on the question of "belongs" since he's far more > familiar than me with the breakdown of the libva sources. I've installed readelf on ChromeOS-Vanilla-04028.0.2013_**04_20_1810_r706c41144. It shows me that vaSyncSurface resides in libva.so.1, not libva-x11.so.1. Same as Ubuntu and Fedora. So it should make sense dlsym vaSyncSurface by using vaapi_handle, not vaapi_x11_handle. This bug is hidden on chromeos is because that libva-x11 is explicitly linked to libva. This leads dlsym(vaapi_x11_handle, vaSyncSurface) calls successfully. But it still a hidden bug, we should fix it.
On Wed, May 29, 2013 at 1:11 AM, <halton.huo@intel.com> wrote: > On 2013/05/29 05:30:15, Ami Fischman wrote: > >> Halton/Pawel: can either of you flesh out the explanation for why it is >> right to depend on a specific version of the .so, and further why this >> particular version? >> IOW, why libva.so.1 and not libva.so.1.3200.0, or any other version ? > > libva.so.1 is SONAME of libva.so.1.3200.0. > This makes sense. We should depend on sonames. TBH "the symbol belongs in libfoo and not libbar AND is available in libfoo >> on cros/x86" would be far more convincing than any findings about Fedora. >> I'll defer to Pawel on the question of "belongs" since he's far more >> familiar than me with the breakdown of the libva sources. > > I've installed readelf on ChromeOS-Vanilla-04028.0.2013_** > **04_20_1810_r706c41144. > It shows me that vaSyncSurface resides in libva.so.1, not libva-x11.so.1. > Same > as Ubuntu and Fedora. > So it should make sense dlsym vaSyncSurface by using vaapi_handle, not > vaapi_x11_handle. > This bug is hidden on chromeos is because that libva-x11 is explicitly > linked to > libva. This leads dlsym(vaapi_x11_handle, vaSyncSurface) calls > successfully. But > it still a hidden bug, we should fix it. > Agreed. Your patchset #2 looks good but the CL description needs to be updated.
On 2013/05/29 18:22:04, Ami Fischman wrote: > Your patchset #2 looks good but the CL description needs to be updated. I use git commit --amend to modify the log, then git cl upload. But not sure why the description does not change. Any hint?
On 2013/05/30 02:46:14, haltonhuo wrote: > On 2013/05/29 18:22:04, Ami Fischman wrote: > > Your patchset #2 looks good but the CL description needs to be updated. > I use git commit --amend to modify the log, then git cl upload. But not sure why > the description does not change. Any hint? I've updated the description via the "Edit issue", I'm still a newbie for codereview.chromium.org. I do not find any argument when "git cl upload", if you know, please tell me.
LGTM & CQ'ing (you can land this even though you're not a committer by checking the commit-queue checkbox)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/halton.huo@intel.com/16042004/10001
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
On 2013/05/30 19:39:59, I haz the power (commit-bot) wrote: > Retried try job too often on linux_aura for step(s) browser_tests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura... Failed tests: ExtensionWebstorePrivateApiTest.InstallAccepted I do not think this failure is related with this commit.
Yeah, flaky tests are awesome. Just keep pushing the CQ button until it goes. On Fri, May 31, 2013 at 12:15 AM, <halton.huo@intel.com> wrote: > On 2013/05/30 19:39:59, I haz the power (commit-bot) wrote: > >> Retried try job too often on linux_aura for step(s) browser_tests >> > > http://build.chromium.org/p/**tryserver.chromium/** > buildstatus?builder=linux_**aura&number=45392<http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura&number=45392> > > Failed tests: > ExtensionWebstorePrivateApiTes**t.InstallAccepted > > I do not think this failure is related with this commit. > > https://codereview.chromium.**org/16042004/<https://codereview.chromium.org/1... >
On 2013/05/31 07:18:34, Ami Fischman wrote: > Yeah, flaky tests are awesome. Just keep pushing the CQ button until it > goes. I submit a linux_aura via this web page, but it show me "failure -1", seems the trybot is not triggered at all. Only a committer can do that or anything I'm doing wrong?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/halton.huo@intel.com/16042004/10001
Failed to apply patch for content/common/gpu/media/vaapi_h264_decoder.cc: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file content/common/gpu/media/vaapi_h264_decoder.cc Hunk #1 FAILED at 2388. Hunk #2 FAILED at 2412. 2 out of 2 hunks FAILED -- saving rejects to file content/common/gpu/media/vaapi_h264_decoder.cc.rej Patch: content/common/gpu/media/vaapi_h264_decoder.cc Index: content/common/gpu/media/vaapi_h264_decoder.cc diff --git a/content/common/gpu/media/vaapi_h264_decoder.cc b/content/common/gpu/media/vaapi_h264_decoder.cc index 0eac074b5e24fdc0d8e22ed65fb9fd28540b37a4..eabf9c0658f709de40b1a514462855bc5a1f4ef9 100644 --- a/content/common/gpu/media/vaapi_h264_decoder.cc +++ b/content/common/gpu/media/vaapi_h264_decoder.cc @@ -2388,8 +2388,8 @@ size_t VaapiH264Decoder::GetRequiredNumOfPictures() { // static void VaapiH264Decoder::PreSandboxInitialization() { DCHECK(!pre_sandbox_init_done_); - vaapi_handle = dlopen("libva.so", RTLD_NOW); - vaapi_x11_handle = dlopen("libva-x11.so", RTLD_NOW); + vaapi_handle = dlopen("libva.so.1", RTLD_NOW); + vaapi_x11_handle = dlopen("libva-x11.so.1", RTLD_NOW); pre_sandbox_init_done_ = vaapi_handle && vaapi_x11_handle; } @@ -2412,7 +2412,7 @@ bool VaapiH264Decoder::PostSandboxInitialization() { VAAPI_DLSYM(CreateContext, vaapi_handle); VAAPI_DLSYM(DestroyContext, vaapi_handle); VAAPI_DLSYM(PutSurface, vaapi_x11_handle); - VAAPI_DLSYM(SyncSurface, vaapi_x11_handle); + VAAPI_DLSYM(SyncSurface, vaapi_handle); VAAPI_DLSYM(BeginPicture, vaapi_handle); VAAPI_DLSYM(RenderPicture, vaapi_handle); VAAPI_DLSYM(EndPicture, vaapi_handle);
On https://codereview.chromium.org/16042004/ there's a checkbox named "Commit"; you can check it even when you're not a committer. I just checked it for you. On Fri, May 31, 2013 at 12:29 AM, <halton.huo@intel.com> wrote: > On 2013/05/31 07:18:34, Ami Fischman wrote: > >> Yeah, flaky tests are awesome. Just keep pushing the CQ button until it >> goes. >> > I submit a linux_aura via this web page, but it show me "failure -1", > seems the > trybot is not triggered at all. > > Only a committer can do that or anything I'm doing wrong? > > https://codereview.chromium.**org/16042004/<https://codereview.chromium.org/1... >
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/halton.huo@intel.com/16042004/10001
Failed to apply patch for content/common/gpu/media/vaapi_h264_decoder.cc: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file content/common/gpu/media/vaapi_h264_decoder.cc Hunk #1 FAILED at 2388. Hunk #2 FAILED at 2412. 2 out of 2 hunks FAILED -- saving rejects to file content/common/gpu/media/vaapi_h264_decoder.cc.rej Patch: content/common/gpu/media/vaapi_h264_decoder.cc Index: content/common/gpu/media/vaapi_h264_decoder.cc diff --git a/content/common/gpu/media/vaapi_h264_decoder.cc b/content/common/gpu/media/vaapi_h264_decoder.cc index 0eac074b5e24fdc0d8e22ed65fb9fd28540b37a4..eabf9c0658f709de40b1a514462855bc5a1f4ef9 100644 --- a/content/common/gpu/media/vaapi_h264_decoder.cc +++ b/content/common/gpu/media/vaapi_h264_decoder.cc @@ -2388,8 +2388,8 @@ size_t VaapiH264Decoder::GetRequiredNumOfPictures() { // static void VaapiH264Decoder::PreSandboxInitialization() { DCHECK(!pre_sandbox_init_done_); - vaapi_handle = dlopen("libva.so", RTLD_NOW); - vaapi_x11_handle = dlopen("libva-x11.so", RTLD_NOW); + vaapi_handle = dlopen("libva.so.1", RTLD_NOW); + vaapi_x11_handle = dlopen("libva-x11.so.1", RTLD_NOW); pre_sandbox_init_done_ = vaapi_handle && vaapi_x11_handle; } @@ -2412,7 +2412,7 @@ bool VaapiH264Decoder::PostSandboxInitialization() { VAAPI_DLSYM(CreateContext, vaapi_handle); VAAPI_DLSYM(DestroyContext, vaapi_handle); VAAPI_DLSYM(PutSurface, vaapi_x11_handle); - VAAPI_DLSYM(SyncSurface, vaapi_x11_handle); + VAAPI_DLSYM(SyncSurface, vaapi_handle); VAAPI_DLSYM(BeginPicture, vaapi_handle); VAAPI_DLSYM(RenderPicture, vaapi_handle); VAAPI_DLSYM(EndPicture, vaapi_handle);
This is not flake - you need to rebase your patch since posciak@ changed the file from under you. On Fri, May 31, 2013 at 12:36 AM, <commit-bot@chromium.org> wrote: > Failed to apply patch for content/common/gpu/media/** > vaapi_h264_decoder.cc: > While running patch -p1 --forward --force --no-backup-if-mismatch; > patching file content/common/gpu/media/**vaapi_h264_decoder.cc > Hunk #1 FAILED at 2388. > Hunk #2 FAILED at 2412. > 2 out of 2 hunks FAILED -- saving rejects to file > content/common/gpu/media/**vaapi_h264_decoder.cc.rej > > Patch: content/common/gpu/media/**vaapi_h264_decoder.cc > Index: content/common/gpu/media/**vaapi_h264_decoder.cc > diff --git a/content/common/gpu/media/**vaapi_h264_decoder.cc > b/content/common/gpu/media/**vaapi_h264_decoder.cc > index > 0eac074b5e24fdc0d8e22ed65fb9fd**28540b37a4..** > eabf9c0658f709de40b1a514462855**bc5a1f4ef9 > 100644 > --- a/content/common/gpu/media/**vaapi_h264_decoder.cc > +++ b/content/common/gpu/media/**vaapi_h264_decoder.cc > @@ -2388,8 +2388,8 @@ size_t VaapiH264Decoder::**GetRequiredNumOfPictures() > { > // static > void VaapiH264Decoder::**PreSandboxInitialization() { > DCHECK(!pre_sandbox_init_done_**); > - vaapi_handle = dlopen("libva.so", RTLD_NOW); > - vaapi_x11_handle = dlopen("libva-x11.so", RTLD_NOW); > + vaapi_handle = dlopen("libva.so.1", RTLD_NOW); > + vaapi_x11_handle = dlopen("libva-x11.so.1", RTLD_NOW); > pre_sandbox_init_done_ = vaapi_handle && vaapi_x11_handle; > } > > @@ -2412,7 +2412,7 @@ bool VaapiH264Decoder::**PostSandboxInitialization() > { > VAAPI_DLSYM(CreateContext, vaapi_handle); > VAAPI_DLSYM(DestroyContext, vaapi_handle); > VAAPI_DLSYM(PutSurface, vaapi_x11_handle); > - VAAPI_DLSYM(SyncSurface, vaapi_x11_handle); > + VAAPI_DLSYM(SyncSurface, vaapi_handle); > VAAPI_DLSYM(BeginPicture, vaapi_handle); > VAAPI_DLSYM(RenderPicture, vaapi_handle); > VAAPI_DLSYM(EndPicture, vaapi_handle); > > > https://chromiumcodereview.**appspot.com/16042004/<https://chromiumcodereview... >
On 2013/05/31 07:36:44, I haz the power (commit-bot) wrote: > Failed to apply patch for content/common/gpu/media/vaapi_h264_decoder.cc: > While running patch -p1 --forward --force --no-backup-if-mismatch; > patching file content/common/gpu/media/vaapi_h264_decoder.cc > Hunk #1 FAILED at 2388. > Hunk #2 FAILED at 2412. > 2 out of 2 hunks FAILED -- saving rejects to file > content/common/gpu/media/vaapi_h264_decoder.cc.rej https://chromiumcodereview.appspot.com/14914009 has rewrite vaapi_h264_video_decode.cc, i need verify whether this bug is still valid. If yes, i'll rework it.
On 2013/05/31 07:47:34, haltonhuo wrote: > https://chromiumcodereview.appspot.com/14914009 has rewrite > vaapi_h264_video_decode.cc, i need verify whether this bug is still valid. If > yes, i'll rework it. https://src.chromium.org/viewvc/chrome/trunk/src/content/common/gpu/media/vaa..., line 411,412, 446 this bug should be still valid. A rework patch set will be uploaded in a minute.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/halton.huo@intel.com/16042004/38002
Sorry for I got bad news for ya. Compile failed with a clobber build on ios_dbg_simulator. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/halton.huo@intel.com/16042004/38002
Sorry for I got bad news for ya. Compile failed with a clobber build on ios_dbg_simulator. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/halton.huo@intel.com/16042004/38002
Message was sent while issue was closed.
Change committed as 203386 |