791164 - Hardware video decoding requires IPC privileges

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[cajbir (:cajbir)]

When playing videos in the video player or web browser the hardware decoder fails to instantiate and falls back to software.

If the file ipc/glue/GeckoChildProcessHost.cpp is modified so that base::PRIVILEGES_INHERIT is used instead of base::PRIVILEGES_UNPRIVILEGED in GeckoChildProcessHost::PerformAsyncLaunchInternal then the hardware codec is successfully instantiated.

STR:

1) Start browser on Otoro device
2) Go to http://cd.pn/b2
3) Click play

Expected result:

Playback occurs using hardware decoder

Actual result:

Playback fails with error message due to software decoder not supporting the H.264 profile level.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The lack of communication around this work has been very frustrating.  (Not blaming you specifically.)  Let's make sure that gets better going forward.

If unprivileged processes were able to use the decoder in an earlier kernel update, then the permissions must have changed.  But AFAIK we don't know that.  If the permissions changed, it was either an accident or a bugfix.  If the latter, we need to find out what the change is protecting.  I'll kick that to our friends.

In the meantime, we can hack around this for the video app, like we did for camera.  But it sucks more for video.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Workaround

If we give the video app "deprecated-hwvideo" permissions, this should work.  But sigh.

Comment 3

[cajbir (:cajbir)]

Note that the STR in comment 0 uses the browser, not the video app. The patch had no effect on browser playback. I was also unable to get the video app to work with this patch.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

That's right.  That's why this is a terrible hack :(.

You would need to add a "deprecated-hwvideo" permission bit to the video app's manifest in gaia, then reflash gaia.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Gaia patch for the workaround

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Re: blocking nom.  Settling this blocks, but the work here doesn't, necessarily.  Need more data from our friends.

Comment 7

[Andrew Overholt [:overholt]]

Blocking until we know what we're doing.

Comment 8

[Andrew Overholt [:overholt]]

Giving this to you, cjones, to make the call.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Workaround #2

$ cd $gaia/gonk-misc
$ git apply video-perms.patch

Then push b2g.sh to /system/bin/ or reflash.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The approach we take will be built on the prototype patches above.  (I.e., only video app gets hwdecoder permission.)

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 661154 [details] [diff] [review]
Gaia patch for the workaround

I'm not sure who really "owns" permissions right now.

So the problem here is, we need to temporarily create a "virtual" permission to work around a lack of platform capability in v1 that will be fixed in v2.  This isn't quite the same case as raw-camera-stream, which is a long-term permission that we simply interpret differently (and hand out more stingily) in v1 because of lack of platform capability.

Here's my proposal, but feel free to offer alternate suggestions / bikeshed.

Comment 12

[cajbir (:cajbir)]

I confirm these workarounds result in H.264 video playback from the video app if the workaround in bug 791167 is also applied.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Does this mean that web content and other apps won't be able to use hardware decoding for displaying video? Or that the gallery app won't be able to display hw decoded video?

What's the security implications of giving this permission to an app?

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

system privileges (~root) are required to use the hardware decoder.  We can't hand that out to uncertified content, it's not even on the table.

Ping me privately for more specifics about the decoder interface itself.

In addition to the workaround #1 here, we can kick out videos we can't decode on the CPU to the video app, either through an overlay activity (which is probably what we want for youtube) or just switching to the video app.

Comment 15

[Sotaro Ikeda [:sotaro]]

It is not easy to solve this problem within gecko. But it might be solved by using android framework. In the past, I moved OMX instance within B2G process from Andrea's request. If it moves back within mediaserver process, normal content process could used hw codec via Binder IPC, I think.

To do this, it is necessary to enable Binder IPC thread within a procees by calling following function like in attachment 592577 [details] [diff] [review].

> ProcessState::self()->startThreadPool();

Same thing could be said to camera hw.

Comment 16

[Justin Lebar (not reading bugmail)]

How hard would it be to create a process which runs as root and does the decoding?  We can communicate its results down to the unprivileged child using shmem...

I suspect that's what we're going to have to do if we ever want unprivileged access to these resources; just curious whether it's easy, medium, or whoa-nelly.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

That would be a fundamental change to gecko's video architecture.  Hacking something in with the plugin interface is feasible, but large-effort and we wouldn't get zero-copy with it.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

sicking: f? ping.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 661154 [details] [diff] [review]
Gaia patch for the workaround

Review of attachment 661154 [details] [diff] [review]:
-----------------------------------------------------------------

Sadness that we can't do hardware decoding in other apps. But I think it's the right call to make for v1 unfortunately.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Don't we need to give this permission to the gallery app too? I thought the intent was that we'd be playing videos there too?

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

It's extreme sadness.

(In reply to Jonas Sicking (:sicking) from comment #20)
> Don't we need to give this permission to the gallery app too? I thought the
> intent was that we'd be playing videos there too?

First I've heard of that.

Comment 22

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Filed bug 793034 for the FIXME/XXXXX comment in this patch.

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a05fdefc0214

Comment 24

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Gaia pull request for the video app

https://github.com/mozilla-b2g/gaia/pull/4991

Comment 25

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Real PR

https://github.com/mozilla-b2g/gaia/pull/4993

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a05fdefc0214