847809 - [MMS] Support AMR media type on Firefox OS

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect, P1)

Description

[Shelly Lin [:shelly]]

Attachment: Add audio/amr to the supported decoder type of OMX.

In require to the spec of MMS, adding audio/amr to the supported media type on Firefox OS.

Comment 1

[Shelly Lin [:shelly]]

Test case:
http://people.mozilla.org/~slin/test-amr.html

Comment 2

[Shelly Lin [:shelly]]

Attachment: Add audio/amr to the supported decoder type of OMX.

Comment 3

[Joe Cheng [:jcheng] (please needinfo)]

this is needed to support MMS user stories. Leo+

Comment 4

[Joe Cheng [:jcheng] (please needinfo)]

Shelly, since you already working on this. assign to you. thanks

Comment 5

[Joe Cheng [:jcheng] (please needinfo)]

Hi Shelly, is the patch ready for review? Thanks

Comment 6

[cajbir (:cajbir)]

Please ensure that AMR files cannot be played or used generally from the web. The use of AMR (and other B2G specific formats) should be restricted to internal B2G apps somehow.

Comment 7

[Chris Lee [:clee]]

Should we mark this P1 given it's new feature work?

Comment 8

[Kai-Chih Hu [:khu]]

Shelly, do you have any idea about the possible effort on this case? Thanks.

Comment 9

[Kai-Chih Hu [:khu]]

It looks like we only need to enable the support for AMR, and it's already done. Shelly also did the testing.

Comment 10

[Dietrich Ayala (:dietrich)]

Per partner and release-driver discussions, marking blocking- until all MMS functionality in bug 849867 is complete, allowing it all to be uplifted at once to avoid SMS bustage.

Comment 11

[Shelly Lin [:shelly]]

Attachment: Support amr audio type

Add audio/amr to the supported decoder type of OMX, and to the list of external mimetype.

Comment 12

[Shelly Lin [:shelly]]

Attachment: Support amr audio type

Add audio/amr to the supported decoder type of OMX, and to the list of external mimetype.

Comment 13

[Shelly Lin [:shelly]]

Comment on attachment 723787 [details] [diff] [review]
Support amr audio type

Hi Chris, this has ensure that we only support amr files on B2G, could you review the patch at your convenience? or could you direct me to someone else who can take the reviewing job? Thanks!

Comment 14

[Shelly Lin [:shelly]]

Comment on attachment 723787 [details] [diff] [review]
Support amr audio type

Hi :roc, since this is a p1 issue and I haven't heard from Chris, could you review this patch? Thanks.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What Chris asked for in comment #6 was that on B2G, only B2G apps should be able to play AMR. Web sites loaded in the Web browser should not be able to play AMR. We need to discourage Web and app developers from using AMR and encourage them to use Opus, MP3 or AAC instead.

It would be even better if we can limit the apps which can use AMR to some set of privileged apps.

The same is true for 3gpp.

The way I suggest you do this is to add an nsIPrincipal* parameter to IsOmxSupportedType, defaulting to null. All our current callers would pass null except nsHTMLMediaElement::CreateDecoder, which would pass this->NodePrincipal(). IsOmxSupportedType should only support AMR and 3gpp when the principal is not null and corresponds to the right sort of app. Maybe we should check nsIPrincipal::GetAppStatus is APP_STATUS_PRIVILEGED or above? I don't know enough about the B2G app permissions system to be sure.

Comment 16

[cajbir (:cajbir)]

Comment on attachment 723787 [details] [diff] [review]
Support amr audio type

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

Sorry, missed the feedback request due to email issues.

It all looks good for supporting AMR at compile time but the issue is this enables AMR support at runtime for all web properties. This means the browser can visit sites with AMR files and play them. We shouldn't allow these formats to spread into the open web and should limit them to 'on device' playback only.

Comment 17

[Shelly Lin [:shelly]]

Attachment: v2: Support amr audio type

Thanks for your feedback! I think it works becuase a Tab has an appStatus of "APP_STATUS_INSTALLED".

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 725344 [details] [diff] [review]
v2: Support amr audio type

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

::: content/media/DecoderTraits.cpp
@@ +416,5 @@
>  #endif
>  #ifdef MOZ_WIDGET_GONK
>    if (IsOmxSupportedType(aType)) {
> +    if (aType.EqualsASCII("audio/amr") || aType.EqualsASCII("video/3gpp")) {
> +      nsHTMLMediaElement* aElement = aOwner->GetMediaElement();

This might return null. If it's null, we should not support this format.

@@ +418,5 @@
>    if (IsOmxSupportedType(aType)) {
> +    if (aType.EqualsASCII("audio/amr") || aType.EqualsASCII("video/3gpp")) {
> +      nsHTMLMediaElement* aElement = aOwner->GetMediaElement();
> +      nsIPrincipal* aPrincipal = aElement->NodePrincipal();
> +      if (!aPrincipal) return decoder.forget();

just return nullptr. Also, the return goes on its own line.

@@ +420,5 @@
> +      nsHTMLMediaElement* aElement = aOwner->GetMediaElement();
> +      nsIPrincipal* aPrincipal = aElement->NodePrincipal();
> +      if (!aPrincipal) return decoder.forget();
> +      if (aPrincipal->GetAppStatus() < nsIPrincipal::APP_STATUS_PRIVILEGED) {
> +        return decoder.forget();

same as above.

Comment 19

[Shelly Lin [:shelly]]

Attachment: v2: Support amr audio type

Hi :roc, Could you review the patch at your convenience? I've fixed the patch per feedback and added some comments to the change. Thank you very much!

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 725977 [details] [diff] [review]
v2: Support amr audio type

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

::: content/media/DecoderTraits.cpp
@@ +419,5 @@
> +    // AMR audio is enabled for MMS, but we are discouraging Web and App
> +    // developers from using AMR, thus we only allow AMR to be played on WebApps.
> +    if (aType.EqualsASCII("audio/amr") || aType.EqualsASCII("video/3gpp")) {
> +      nsHTMLMediaElement* aElement = aOwner->GetMediaElement();
> +      nsIPrincipal* aPrincipal = aElement->NodePrincipal();

Don't call these aElement/aPrincipal. Names starting with "a" are reserved for parameters. Call them "element" and "principal".

"element" could be null, so you need to check for that here.

Comment 21

[Shelly Lin [:shelly]]

Attachment: v2: Support amr audio type

Thanks :roc, here is the fixed :)

Comment 22

[Shelly Lin [:shelly]]

Try server results:

https://tbpl.mozilla.org/?tree=Try&rev=f5a8f75172cd
https://tbpl.mozilla.org/?tree=Try&rev=997735e3920a

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/46519785800d

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Backed out for build bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9fb17feaa70

https://tbpl.mozilla.org/php/getParsedLog.php?id=20930318&tree=Mozilla-Inbound

/builds/slave/m-in-ics_a7_g-0000000000000000/build/content/media/DecoderTraits.cpp: In static member function 'static already_AddRefed<mozilla::MediaDecoder> mozilla::DecoderTraits::CreateDecoder(const nsACString_internal&, mozilla::MediaDecoderOwner*)':
/builds/slave/m-in-ics_a7_g-0000000000000000/build/content/media/DecoderTraits.cpp:407: error: 'nsHTMLMediaElement' was not declared in this scope
/builds/slave/m-in-ics_a7_g-0000000000000000/build/content/media/DecoderTraits.cpp:407: error: 'element' was not declared in this scope
make[6]: *** [DecoderTraits.o] Error 1

Comment 25

[Shelly Lin [:shelly]]

Attachment: Support amr audio type

Carry r+.

Comment 26

[Shelly Lin [:shelly]]

https://tbpl.mozilla.org/?tree=Try&rev=c10e6f4e9509
https://tbpl.mozilla.org/?tree=Try&rev=a994f4802ee0

(It was very weird that I can even pass the building stage and try server...)

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d61e5165928

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3d61e5165928

Comment 29

[Marco Chen [:mchen]]

Bug 847006 added amr support into DeviceStorage.

Comment 30

[Dylan Oliver [:doliver]]

triage: blocking in support of MMS

Comment 31

[Marco Chen [:mchen]]

To change the status-b2g18 to affected, so the sheriff will scan it and help to uplift.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

(In reply to Marco Chen [:mchen] from comment #31)
> To change the status-b2g18 to affected, so the sheriff will scan it and help
> to uplift.

All I need is the leo+ to find it :). That said, this needs a b2g18-specific patch.

Comment 33

[Shelly Lin [:shelly]]

Attachment: Support amr audio type (b2g18)

I found out that we have a refactor for media decoder, file DecoderTraits.cpp is newly added and is not in the code base of b2g18. Does it needs another review?

Comment 34

[Shelly Lin [:shelly]]

Comment on attachment 747271 [details] [diff] [review]
Support amr audio type (b2g18)

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

::: content/html/content/src/nsHTMLMediaElement.cpp
@@ +2590,5 @@
> +      nsIPrincipal* principal = NodePrincipal();
> +      if (!principal) {
> +        return nullptr;
> +      }
> +      if (principal->GetAppStatus() < nsIPrincipal::APP_STATUS_PRIVILEGED) {

Actually, the app status of a web tab is APP_STATUS_NOT_INSTALLED, and is APP_STATUS_INSTALLED on a test app (not approved by Firefox Marketplace). Should I lower the level check to the following?
|if (principal->GetAppStatus() < nsIPrincipal::APP_STATUS_INSTALLED)|

Comment 35

[Shelly Lin [:shelly]]

Hi Chris, any thoughts about comment 34?

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 747271 [details] [diff] [review]
Support amr audio type (b2g18)

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

The patch looks OK.

Let's land this. The question of whether installed-but-not-privileged apps should have AMR access can be answered separately. I'd say that if we don't have a compelling need for that, we should avoid it.

Comment 37

[cajbir (:cajbir)]

(In reply to Shelly Lin [:shelly] from comment #35)
> Hi Chris, any thoughts about comment 34?

Stick with privileged for now and worry about installed apps later. Easier to drop down than up if needed I think,

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/76ff4dcebb91

Comment 39

[Sandip Kamat]

To clarify, what does the current patch include? All points Roc mentioned in comment #15 or some?

1. AMR for "on device" play only and not on web
2. limit the apps which can use AMR to some set (or all?) of privileged apps.

Comment 40

[Shelly Lin [:shelly]]

1. Yes, AMR can only be loaded and played on "on device" web apps, not on web pages.
2. Yes, even for "on device" web apps are limited to privileged apps.