1205927 - [FireFox OS 2.2][Multimedia] FFOS support EVRC record Inquiry

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[sangyeol]

User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/27.0.1453.110 Safari/537.36 CoolNovo/2.0.9.20

Comment 1

[sangyeol]

In latest Gecko version, Mozilla only supports 3GPP(AMR) and OGG types for audio recording.
(Please check “gecko/dom/media/encoder/MediaEncoder.cpp” file.)
please check EVRC Record is supported.
if not supported, is it possible to add this API for EVRC record?

Comment 2

[sangyeol]

Currently, EVRC codec is QC H/W Codec

Comment 3

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Blake, could you provide some initial inputs?

Comment 4

[Alfredo Yang (:alfredo)]

If it supports OpenMax IL interface, that'll be easier to do it; otherwise, we need to implement a new encoder.

Comment 5

[John Lin [:jhlin][:jolin]]

(In reply to Alfredo Yang (:alfredo) from comment #4)
> If it supports OpenMax IL interface, that'll be easier to do it; otherwise,
> we need to implement a new encoder.

 /system/etc/media_codecs.xml lists all exposed codecs on device. For example,
 
  Flame(KK) has EVRC decoder, but no encoder:
    <MediaCodec name="OMX.qcom.audio.decoder.evrc" type="audio/evrc" >

  Z3C(KK) has both:
    <MediaCodec name="OMX.qcom.audio.encoder.evrc" type="audio/evrc" >
    <MediaCodec name="OMX.qcom.audio.decoder.evrc" type="audio/evrc" >

 And android::MediaCodecList::findCodecByType("audio/evrc", /* encoder? */ true) can be used to check the availability at runtime.

Comment 6

[John Lin [:jhlin][:jolin]]

Also, IMO new API is not necessary since in latest spec, MediaRecorder constructor (optionally) accepts mimeType parameter[1] and has canRecordMimeType() method[2] already.

[1] http://w3c.github.io/mediacapture-record/MediaRecorder.html#widl-ctor-MediaRecorder--MediaStream-stream-DOMString-mimeType
[2] http://w3c.github.io/mediacapture-record/MediaRecorder.html#widl-MediaRecorder-canRecordMimeType-DOMString-DOMString-mimeType

Comment 7

[Jaemin Song]

(In reply to John Lin [:jolin][:jhlin] from comment #6)
> Also, IMO new API is not necessary since in latest spec, MediaRecorder
> constructor (optionally) accepts mimeType parameter[1] and has
> canRecordMimeType() method[2] already.
> 
> [1]
> http://w3c.github.io/mediacapture-record/MediaRecorder.html#widl-ctor-
> MediaRecorder--MediaStream-stream-DOMString-mimeType
> [2]
> http://w3c.github.io/mediacapture-record/MediaRecorder.html#widl-
> MediaRecorder-canRecordMimeType-DOMString-DOMString-mimeType

Dear John,

According your comments,
if EVRC encoder is available in media_codec.xml, EVRC recording is available without the need to add API.
Is that right?

If latest spec you mentioned([1],[2]) is implemented in FFOS2.2 version,
please let we know directory and file name.

Thanks.

Comment 8

[John Lin [:jhlin][:jolin]]

(In reply to Jaemin Song from comment #7)
> (In reply to John Lin [:jolin][:jhlin] from comment #6)
> > Also, IMO new API is not necessary since in latest spec, MediaRecorder
> > constructor (optionally) accepts mimeType parameter[1] and has
> > canRecordMimeType() method[2] already.
> > 
> > [1]
> > http://w3c.github.io/mediacapture-record/MediaRecorder.html#widl-ctor-
> > MediaRecorder--MediaStream-stream-DOMString-mimeType
> > [2]
> > http://w3c.github.io/mediacapture-record/MediaRecorder.html#widl-
> > MediaRecorder-canRecordMimeType-DOMString-DOMString-mimeType
> 
> Dear John,
> 
> According your comments,
> if EVRC encoder is available in media_codec.xml, EVRC recording is available
> without the need to add API.
> Is that right?

 What I meant is that you don't have to add new API in MediaRecorder spec.
 There's still Gecko work to do to support EVRC (like what :rlin had done for 3GPP/AMR in bug 959490).

> 
> If latest spec you mentioned([1],[2]) is implemented in FFOS2.2 version,
> please let we know directory and file name.
> 
> Thanks.

[1] is implemented in bug 959490 (files under dom/media/).
[2] is not implemented in Gecko yet and you can file a new bug for it.

Comment 9

[Jaemin Song]

(In reply to John Lin [:jolin][:jhlin] from comment #8)
> There's still Gecko work to do to support EVRC (like what :rlin had done for 3GPP/AMR in bug 959490).

For EVRC recording, could you add EVRC format like 3GPP/AMR(bug 959490)?

Thanks.

Comment 10

[Blake Wu [:bwu][:blakewu]]

Munro, 
Could you help on this?

Comment 11

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

sangyeol,

May I know more detail about the use case of EVRC?
And could you share us
1) some sample clips with evrc codec
2) any desktop player which can play this format.

Comment 12

[sangyeol]

Dear Munro

1) This is for EVRC recording, would you let me know why you need an evrc sample file?
(EVRC Playback already checked O.K.)
2) Any player like VLC Player

Comment 13

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

I am searching a player to verify the b2g recorded evrc clip.
I tried to use VLC player to play a evrc audio sample from http://download.wavetlan.com/SVV/Media/HTTP/http-3gp-audio.htm but it failed. I am not sure if VLC player cannot support this format or the audio sample doesn't meet spec.
Is that possible you can provide us some samples for reference?

Comment 14

[sangyeol]

Attachment: EVRC_Audio_SAMPLE_With_MPEG4_Video.3g2

uploaded EVRC sample audio.
kindly check this sample.

Comment 15

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Hi Sangyeol,

Is this required for 2.2R, and why?
Please be note that either feature-b2g:2.2r+ or blocking-b2g:2.2r+ is needed to get patches landed to 2.2R.

And we also won't focus on non-required items given the schedule concern.

Comment 16

[sangyeol]

(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #15)
> Hi Sangyeol,
> 
> Is this required for 2.2R, and why?
- Yes, This is required for 2.2R 
- Because of Verizon Wireless's requirement which is based on 2.2R
> Please be note that either feature-b2g:2.2r+ or blocking-b2g:2.2r+ is needed
> to get patches landed to 2.2R.
> 
> And we also won't focus on non-required items given the schedule concern.

Comment 17

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

calling out to PM to confirm the requirements with partner (also ni TAM to help out)

Comment 18

[Sandip Kamat]

It is a duplicate of 1205926?

Comment 19

[sangyeol]

1205926(In reply to Sandip Kamat from comment #18)
> It is a duplicate of 1205926?
Hi Sandip,
Yes, This one(for EVRC) is as a duplication of 1205926
But Please note that 1205926 is about PCM Record.
Both of these recording APIs are needed.

Comment 20

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Sandip, could you confirm if we need to support this feature? Thanks

Comment 21

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

I've been told these are mandatory.
Will you also be able to take bug 1205925 and bug 1205926?

Comment 22

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: WIP-Bug-1205927-Part-1-MediaEncoder-Support-.3g2-with-EV.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1205927
User impact if declined: cannot pass carrier lab test
Testing completed: vlc player can play the evrc clip recorded by flame
Risk to taking this patch (and alternatives if risky): no
String or UUID changes made by this patch: no

Comment 23

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: WIP-Bug-1205927-Part-2-Add-audio-capture-3gpp2-perimissi.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 1205927
User impact if declined: cannot pass carrier lab entry test
Testing completed: vlc player can play the evrc clip recorded by flame
Risk to taking this patch (and alternatives if risky): no
String or UUID changes made by this patch: no

Comment 24

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8678710 [details] [diff] [review]
WIP-Bug-1205927-Part-1-MediaEncoder-Support-.3g2-with-EV.patch

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

I am fine with adding EVRC encoding on B2G.

Alfredo, or someone with more experience with MediaEncoder code than I, should review this.

Comment 25

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8678712 [details] [diff] [review]
WIP-Bug-1205927-Part-2-Add-audio-capture-3gpp2-perimissi.patch

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

I am fine with adding EVRC encoding on B2G.

Alfredo, or someone with more experience with MediaEncoder code than I, should review this.

Comment 26

[Alfredo Yang (:alfredo)]

Comment on attachment 8678710 [details] [diff] [review]
WIP-Bug-1205927-Part-1-MediaEncoder-Support-.3g2-with-EV.patch

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

+Jolin for TrackEncoder review.

::: dom/media/encoder/fmp4_muxer/ISOMediaBoxes.cpp
@@ +12,5 @@
>  #include "ISOTrackMetadata.h"
>  #include "MP4ESDS.h"
>  #include "AMRBox.h"
>  #include "AVCBox.h"
> +#include "EVRCBox.h"

I can't find this file. Do you forget to add it?

Comment 27

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: WIP-Bug-1205927-Part-1-MediaEncoder-Support-.3g2-with-EV.patch

Thanks for the reminding!
Please help review the new WIP

Comment 28

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: WIP-Bug-1205927-Part-2-Add-audio-capture-3gpp2-perimissi.patch

Comment 29

[Alfredo Yang (:alfredo)]

Comment on attachment 8679288 [details] [diff] [review]
WIP-Bug-1205927-Part-1-MediaEncoder-Support-.3g2-with-EV.patch

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

Leave OmxTrackEncoder for John to review. r+ for others.

Comment 30

[Alfredo Yang (:alfredo)]

Comment on attachment 8679289 [details] [diff] [review]
WIP-Bug-1205927-Part-2-Add-audio-capture-3gpp2-perimissi.patch

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

LGTM.

Comment 31

[John Lin [:jhlin][:jolin]]

Comment on attachment 8679288 [details] [diff] [review]
WIP-Bug-1205927-Part-1-MediaEncoder-Support-.3g2-with-EV.patch

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

Overall looks good to me. There are some place that can be refactored though, IMHO.
- OmxAudioTrackEncoder::AppendEncodedFrames() checks for codec (IOW subclass) type to determine the frame type. That could be replaced with virtual method call.
- OMXCodecWrapper::mAMRCSDProvided could be used to handle all "no CSD from encoder" cases.

Comment 32

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Thanks for the comment.
I have created bug 1219100 for refactoring.

Comment 33

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=978105806a39

Comment 34

[Carsten Book [:Tomcat]]

Hi, this has conflicts when trying to checkin to 2.2r 

1 out of 1 hunks FAILED -- saving rejects to file dom/media/MediaRecorder.cpp.rej
patching file dom/media/encoder/OmxTrackEncoder.cpp
Hunk #1 FAILED at 178
Hunk #2 FAILED at 337
2 out of 2 hunks FAILED -- saving rejects to file dom/media/encoder/OmxTrackEncoder.cpp.rej
patching file dom/media/encoder/OmxTrackEncoder.h
Hunk #1 FAILED at 84
1 out of 1 hunks FAILED -- saving rejects to file dom/media/encoder/OmxTrackEncoder.h.rej
patching file dom/media/encoder/fmp4_muxer/ISOControl.cpp
Hunk #2 FAILED at 162
1 out of 2 hunks FAILED -- saving rejects to file dom/media/encoder/fmp4_muxer/ISOControl.cpp.rej
patching file dom/media/encoder/fmp4_muxer/ISOMediaBoxes.cpp
Hunk #2 FAILED at 661
1 out of 3 hunks FAILED -- saving rejects to file dom/media/encoder/fmp4_muxer/ISOMediaBoxes.cpp.rej
patching file dom/media/encoder/fmp4_muxer/ISOMediaWriter.cpp
Hunk #1 FAILED at 98
1 out of 2 hunks FAILED -- saving rejects to file dom/media/encoder/fmp4_muxer/ISOMediaWriter.cpp.rej
patching file dom/media/encoder/fmp4_muxer/ISOTrackMetadata.h
Hunk #1 succeeded at 96 with fuzz 2 (offset 0 lines).
patching file dom/media/encoder/fmp4_muxer/moz.build
Hunk #1 succeeded at 8 with fuzz 1 (offset 0 lines).
patching file dom/media/omx/OMXCodecWrapper.h
Hunk #3 FAILED at 202
1 out of 3 hunks FAILED -- saving rejects to file dom/media/omx/OMXCodecWrapper.h.rej
patching file netwerk/mime/nsMimeTypes.h
Hunk #1 FAILED at 78
1 out of 1 hunks FAILED -- saving rejects to file netwerk/mime/nsMimeTypes.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh WIP-Bug-1205927-Part-1-MediaEncoder-Support-.3g2-with-EV.patch

could you take a look, thanks!

Comment 35

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: WIP-2_2r-Bug-1205927-Part-1-MediaEncoder-Support-.3g2-with-EV.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 1205927
User impact if declined: cannot pass VZW lab test
Testing completed: vlc player can play the clip recorded with Flame
Risk to taking this patch (and alternatives if risky): no
String or UUID changes made by this patch: no

Comment 36

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Attachment: WIP-2_2r-Bug-1205927-Part-2-Add-audio-capture-3gpp2-perimissi.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 1205927
User impact if declined: cannot pass VZW lab test
Testing completed: vlc player can play the clip recorded with Flame
Risk to taking this patch (and alternatives if risky): no
String or UUID changes made by this patch: no

Comment 37

[Carsten Book [:Tomcat]]

landed as remote:   https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/d2c31956955d
remote:   https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/ac3ed9649442

Comment 38

[Carsten Book [:Tomcat]]

Hi Munro, removing checkin-needed because this need first approval from mahe for 2.2

Comment 39

[Mahendra Potharaju [:mahe]]

Comment on attachment 8679908 [details] [diff] [review]
WIP-2_2r-Bug-1205927-Part-1-MediaEncoder-Support-.3g2-with-EV.patch

Approved to land on 2.2R. 

Thanks

Comment 40

[Mahendra Potharaju [:mahe]]

Comment on attachment 8679912 [details] [diff] [review]
WIP-2_2r-Bug-1205927-Part-2-Add-audio-capture-3gpp2-perimissi.patch

Approved to land on 2.2R

Thanks

Comment 41

[Mahendra Potharaju [:mahe]]

Munro, Did you intend to have this patch land on 2.2 or 2.2R?

Tomcat got this to my notice as its already landed on 2.2R

Comment 42

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

I intend to have this patch on 2.2R and central.
attachment 8679288 [details] [diff] [review] and attachment 8679289 [details] [diff] [review] -> central
attachment 8679908 [details] [diff] [review] and attachment 8679912 [details] [diff] [review] -> 2.2R (already done)

Comment 43

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/888a6bae2a4b
https://hg.mozilla.org/integration/b2g-inbound/rev/65995f4d5183

Comment 44

[Munro Mengjue Chiang [:mchiang] Be Mozillian since 1/19/2018]

Sangyeol,

We have landed a patch which supports MediaRecorder with mime type "audio/3gpp2".
Please notice that only privileged or certified apps can access.

Comment 45

[Mahendra Potharaju [:mahe]]

Munro, Thank you for confirming

Comment 46

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

remote:   https://hg.mozilla.org/mozilla-central/rev/888a6bae2a4b
remote:   https://hg.mozilla.org/mozilla-central/rev/65995f4d5183

Comment 47

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/888a6bae2a4b
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/65995f4d5183

Comment 48

[Josh Cheng [:josh]]

Comment on attachment 8679288 [details] [diff] [review]
WIP-Bug-1205927-Part-1-MediaEncoder-Support-.3g2-with-EV.patch

Not maintaining 2.2 anymore

Comment 49

[Josh Cheng [:josh]]

Comment on attachment 8679289 [details] [diff] [review]
WIP-Bug-1205927-Part-2-Add-audio-capture-3gpp2-perimissi.patch

Not maintaining 2.2 anymore