Closed Bug 791164 Opened 7 years ago Closed 7 years ago

Hardware video decoding requires IPC privileges

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-basecamp:+)

RESOLVED FIXED
blocking-basecamp +

People

(Reporter: cajbir, Assigned: cjones)

References

Details

(Whiteboard: [LOE:S])

Attachments

(2 files, 1 obsolete file)

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.
Blocks: 765977
Blocks: 791167
blocking-basecamp: --- → ?
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.
Attached patch WorkaroundSplinter Review
If we give the video app "deprecated-hwvideo" permissions, this should work.  But sigh.
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.
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.
Re: blocking nom.  Settling this blocks, but the work here doesn't, necessarily.  Need more data from our friends.
Blocking until we know what we're doing.
blocking-basecamp: ? → +
Giving this to you, cjones, to make the call.
Assignee: nobody → jones.chris.g
Whiteboard: [caf:needs input]
Attached patch Workaround #2 (obsolete) — Splinter Review
$ cd $gaia/gonk-misc
$ git apply video-perms.patch

Then push b2g.sh to /system/bin/ or reflash.
Attachment #661429 - Attachment is obsolete: true
The approach we take will be built on the prototype patches above.  (I.e., only video app gets hwdecoder permission.)
Attachment #661140 - Flags: review?(justin.lebar+bug)
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.
Attachment #661154 - Flags: feedback?(jonas)
Attachment #661154 - Flags: feedback?(fabrice)
Attachment #661140 - Flags: review?(justin.lebar+bug) → review+
I confirm these workarounds result in H.264 video playback from the video app if the workaround in bug 791167 is also applied.
Whiteboard: [caf:needs input] → [caf:needs input][LOE:S]
Whiteboard: [caf:needs input][LOE:S] → [LOE:S]
Attachment #661154 - Flags: feedback?(fabrice) → feedback+
Blocks: 759506
No longer blocks: 765977
No longer blocks: 791167
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?
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.
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.
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.
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 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.
Attachment #661154 - Flags: feedback?(jonas) → feedback+
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?
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.
Filed bug 793034 for the FIXME/XXXXX comment in this patch.
https://hg.mozilla.org/mozilla-central/rev/a05fdefc0214
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Blocks: 798202
You need to log in before you can comment on or make changes to this bug.