[MediaEncoder] Support *.3gp with AMR-NB audio format in privileged applications

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: erahm, Assigned: erahm)

Tracking

unspecified
mozilla37
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

+++ This bug was initially created as a clone of Bug #959490 +++

It would be useful to loosen the restriction [1] of AMR recording from certified-only to privileged apps as well.

[1] http://hg.mozilla.org/mozilla-central/annotate/28fdae830289/dom/media/MediaRecorder.cpp#l561
If we're going to do this, then we should definitely not expose it to all privileged apps, but instead require some permission.

That said, I'll defer to the media team about what we want to do here.
It may also be useful to be able to receive the data without the 3GP container.  Marco, Pin, and Yuan know more than me about the format that would be the most useful.  cc:ing them.
(In reply to Jonas Sicking (:sicking) from comment #1)
> If we're going to do this, then we should definitely not expose it to all
> privileged apps, but instead require some permission.
> 
> That said, I'll defer to the media team about what we want to do here.
How about the permssion of "audio-capture-3gpp" or "audio-capture-amr"?

BTW, we already requrie the "audio-capture" permission to perform audio recording. So it won't expose to privileged apps without that permission in advance. The "audio-capture" permission might be enough. ;-)
Allowing AMR in certified Apps sounds fine to me.
(In reply to Chris Pearce (:cpearce) from comment #4)
> Allowing AMR in certified Apps sounds fine to me.

Erm, did you mean privileged?  Certified apps already have access to AMR, per bug 959490.  This bug is about extending that capability to privileged apps!
(In reply to Yuan Xulei [:yxl] from comment #3)
> BTW, we already requrie the "audio-capture" permission to perform audio
> recording. So it won't expose to privileged apps without that permission in
> advance. The "audio-capture" permission might be enough. ;-)

Jonas, do you feel like the existing "audio-capture" permission is good enough?
Flags: needinfo?(jonas)
No. We should have a separate way to enable codecs separate from the ability to record using the standard web codecs.
Flags: needinfo?(jonas)
(In reply to Myk Melez [:myk] [@mykmelez] from comment #5)
> (In reply to Chris Pearce (:cpearce) from comment #4)
> > Allowing AMR in certified Apps sounds fine to me.
> 
> Erm, did you mean privileged?  Certified apps already have access to AMR,
> per bug 959490.  This bug is about extending that capability to privileged
> apps!

I don't understand the difference, but as long as it's not exposed to regular web content inside the browser app, I'm not bothered by AMR working in other apps.
There's a lot of privileged apps. So just because we only expose it to privileged apps doesn't mean that won't have to deal with compatibility issues.

I'm still fine with exposing additional codecs to privileged apps, but I'd like to require an explicit permission for it.
(In reply to Jonas Sicking (:sicking) from comment #9)
> There's a lot of privileged apps. So just because we only expose it to
> privileged apps doesn't mean that won't have to deal with compatibility
> issues.
> 
> I'm still fine with exposing additional codecs to privileged apps, but I'd
> like to require an explicit permission for it.

Sounds reasonable, let's go with Yuan's suggestion of "audio-capture-3gpp" as that maps to the mime type the encoder is using.

Myk's request to also support raw AMR seems like it might be a bit more work as the existing encoder implementation only supports 3gpp. Perhaps we could split out a separate bug for that and limit this to adding the permission and loosening the restriction.

I'm happy to implement this, although I don't know exactly how we add permissions (and check them from C++). If someone can point me to an example in our codebase that would be helpful. Or if someone from the media teas wants to do this, that would be fine too.
Assignee: nobody → erahm
The permissions are defined in `PermissionsTable.jsm`(http://dxr.mozilla.org/mozilla-central/source/dom/apps/PermissionsTable.jsm#267) and you could new permission in this file.

You can possibly check the permission via document principal:
http://hg.mozilla.org/mozilla-central/file/28fdae830289/dom/media/MediaManager.cpp#l1747
This simply adds the 'audio-capture:3gpp' permission. A ':' was necessary due to the way the permissions table sets up a reverse lookup for sub-permissions.
Attachment #8538757 - Flags: review?(jonas)
This patch checks for the 3GPP permission when the 3GPP mimetype is specified. Certified apps still do not require this permission.
Attachment #8538759 - Flags: review?(roc)
Comment on attachment 8538757 [details] [diff] [review]
Part 1: Add audio-capture:3gpp perimission

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

Handing over to Fabrice.
Attachment #8538757 - Flags: review?(jonas) → review?(fabrice)
Which part of the code gets confused if you use "-" rather than ":"? I don't see any code that treats the values as anything but opaque strings. That said, I don't care strongly either way, mainly curious and worried about that there might be some bigger problems going on here.
(In reply to Jonas Sicking (:sicking) from comment #17)
> Which part of the code gets confused if you use "-" rather than ":"? I don't
> see any code that treats the values as anything but opaque strings. That
> said, I don't care strongly either way, mainly curious and worried about
> that there might be some bigger problems going on here.

I assumed it had something to do with the reverse lookup table [1], honestly I didn't try to track it down more than "huh that doesn't work and returns audio-capture's permission instead."

[1] http://hg.mozilla.org/mozilla-central/annotate/021b09e92d30/dom/apps/PermissionsTable.jsm#l625
Comment on attachment 8538757 [details] [diff] [review]
Part 1: Add audio-capture:3gpp perimission

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

r=me on the patch, but flagging Paul and Jonas to make sure that's fine from a security policy standpoint.
Attachment #8538757 - Flags: review?(ptheriault)
Attachment #8538757 - Flags: review?(jonas)
Attachment #8538757 - Flags: review?(fabrice)
Attachment #8538757 - Flags: review+
(In reply to Fabrice Desré [:fabrice] from comment #19)
> Comment on attachment 8538757 [details] [diff] [review]
> Part 1: Add audio-capture:3gpp perimission
> 
> Review of attachment 8538757 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me on the patch, but flagging Paul and Jonas to make sure that's fine from
> a security policy standpoint.

Fabrice, do you have a specific concern? Jonas seemed OK with the general idea of this in comment 9, but maybe you're concerned about the actual code changes?
Flags: needinfo?(fabrice)
(In reply to Eric Rahm [:erahm] from comment #20)
> (In reply to Fabrice Desré [:fabrice] from comment #19)
> > Comment on attachment 8538757 [details] [diff] [review]
> > Part 1: Add audio-capture:3gpp perimission
> > 
> > Review of attachment 8538757 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > r=me on the patch, but flagging Paul and Jonas to make sure that's fine from
> > a security policy standpoint.
> 
> Fabrice, do you have a specific concern? Jonas seemed OK with the general
> idea of this in comment 9, but maybe you're concerned about the actual code
> changes?

Oh right, I missed this comment. Go ahead then!
Flags: needinfo?(fabrice)
add r=fabrice
Attachment #8538757 - Attachment is obsolete: true
Attachment #8538757 - Flags: review?(ptheriault)
Attachment #8538757 - Flags: review?(jonas)
Fix unified bustage, add r=roc
Attachment #8538759 - Attachment is obsolete: true
Comment on attachment 8544268 [details] [diff] [review]
Part 1: Add audio-capture:3gpp perimission

Carrying forward r+
Attachment #8544268 - Flags: review+
Comment on attachment 8544269 [details] [diff] [review]
Part 2: Check for 3gpp permission

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

Carrying forward r+
Attachment #8544269 - Flags: review+
Added a |#include "nsIPermissionManager.h"|, should fix unified bustage.
Flags: needinfo?(erahm)
https://hg.mozilla.org/mozilla-central/rev/78d67ab239d2
https://hg.mozilla.org/mozilla-central/rev/eb47315f8acb
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.