Closed
Bug 1111135
Opened 10 years ago
Closed 10 years ago
[MediaEncoder] Support *.3gp with AMR-NB audio format in privileged applications
Categories
(Core :: Audio/Video: Recording, defect)
Tracking
()
RESOLVED
FIXED
mozilla37
People
(Reporter: erahm, Assigned: erahm)
References
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
erahm
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
erahm
:
review+
|
Details | Diff | Splinter Review |
+++ 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.
Comment 2•10 years ago
|
||
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.
Comment 3•10 years ago
|
||
(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. ;-)
Comment 4•10 years ago
|
||
Allowing AMR in certified Apps sounds fine to me.
Comment 5•10 years ago
|
||
(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!
Assignee | ||
Comment 6•10 years ago
|
||
(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)
Comment 8•10 years ago
|
||
(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.
Assignee | ||
Comment 10•10 years ago
|
||
(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 | ||
Updated•10 years ago
|
Assignee: nobody → erahm
Comment 11•10 years ago
|
||
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
Assignee | ||
Comment 12•10 years ago
|
||
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)
Assignee | ||
Comment 13•10 years ago
|
||
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)
Attachment #8538759 -
Flags: review?(roc) → review+
Assignee | ||
Comment 14•10 years ago
|
||
Assignee | ||
Comment 15•10 years ago
|
||
Assignee | ||
Comment 16•10 years ago
|
||
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.
Assignee | ||
Comment 18•10 years ago
|
||
(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 19•10 years ago
|
||
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+
Assignee | ||
Comment 20•10 years ago
|
||
(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)
Comment 21•10 years ago
|
||
(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)
Assignee | ||
Comment 22•10 years ago
|
||
Both backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/eb6f3e7f5650 for build bustage:
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=5090443&repo=mozilla-inbound
Flags: needinfo?(erahm)
Assignee | ||
Comment 24•10 years ago
|
||
add r=fabrice
Assignee | ||
Updated•10 years ago
|
Attachment #8538757 -
Attachment is obsolete: true
Attachment #8538757 -
Flags: review?(ptheriault)
Attachment #8538757 -
Flags: review?(jonas)
Assignee | ||
Comment 25•10 years ago
|
||
Fix unified bustage, add r=roc
Assignee | ||
Updated•10 years ago
|
Attachment #8538759 -
Attachment is obsolete: true
Assignee | ||
Comment 26•10 years ago
|
||
Comment on attachment 8544268 [details] [diff] [review]
Part 1: Add audio-capture:3gpp perimission
Carrying forward r+
Attachment #8544268 -
Flags: review+
Assignee | ||
Comment 27•10 years ago
|
||
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+
Assignee | ||
Comment 28•10 years ago
|
||
Added a |#include "nsIPermissionManager.h"|, should fix unified bustage.
Flags: needinfo?(erahm)
Assignee | ||
Comment 29•10 years ago
|
||
Assignee | ||
Comment 30•10 years ago
|
||
Comment 31•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/78d67ab239d2
https://hg.mozilla.org/mozilla-central/rev/eb47315f8acb
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
You need to log in
before you can comment on or make changes to this bug.
Description
•