959490 - [MediaEncoder] Support *.3gp with AMR-NB audio format on certificated application

Status: RESOLVED FIXED

(Core :: Audio/Video: Recording, defect)

Comment 1

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Hi Marvin, 
I heard this requirement comes form our partner, 
Could you confirm this and give us the user story and we can finish this feature.

Comment 2

[Marvin Khoo [:Marvin_Khoo]]

Hi Randy, as dicussed, i modified the REQ here.

Comment 3

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Hi Alfredo, 
Could you evolute how much work should we do for this task?

Comment 4

[Alfredo Yang (:alfredo)]

We need following new components:
1. AMR OMX encoder.
2. AMRSpecificBox in ISOMediaWriter.
And ISOMediaWriter also needs some 3GP specific headers.

Please notes that according to ETSI TS 126 244 V6.2.0, "In order to maintain backward compatibility with Release 4 and Release 5, it is not recommended to use movie fragments in 3GP files for MMS. ".
It's against the design of ISOMediaWriter.

Comment 5

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

If we want to support this one.
We should 
1. Rewrite a new 3gp muxer to package encoder data in .3gp format. (bug:891704 [MP4 writer] took around two months to land)
2. Produce amr-nb audio format through OMXWrapper. (May took two weeks)
3. Recorder API should has an API to control what kind of audio format can generate.
4. Convince Robert(:roc) to support this format.

Comment 6

[Marvin Khoo [:Marvin_Khoo]]

had ping Robert to this, will communicate with him.

Comment 7

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

(In reply to Randy Lin [:rlin] from comment #5)
> If we want to support this one.
> We should 
> 1. Rewrite a new 3gp muxer to package encoder data in .3gp format.
> (bug:891704 [MP4 writer] took around two months to land)
> 2. Produce amr-nb audio format through OMXWrapper. (May took two weeks)
> 3. Recorder API should has an API to control what kind of audio format can
> generate.
> 4. Convince Robert(:roc) to support this format.

You should do step 4 first. Otherwise you may waste work.  ;)

Comment 8

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

For point 3,
Media Encoder framework would keep all the encoder data in memory /temporary file and push out when application call the stop method.

For point 4, 
Only allow to produce this kind of format in certificated application.

Comment 9

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: wip

todo: resample

Comment 10

[Ivan Tsay (:ITsay)]

Correct dependency

Comment 11

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: wip2

Comment 12

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: wip2.1

Comment 13

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: wip v3

wip v3, add csd, fix sound problem.

Comment 14

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: wip v3

update to the correct one.

Comment 15

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v1

Hi John, 
Could you help to give FB for this one? 
Thanks.

Comment 16

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

Comment on attachment 8380503 [details] [diff] [review]
patch v1

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

::: content/media/encoder/OmxTrackEncoder.cpp
@@ +312,5 @@
> +  return meta.forget();
> +}
> +
> +nsresult
> +OmxAMRAudioTrackEncoder::AppendEncodedFrames(EncodedFrameContainer& aContainer)

This function looks nearly identical to OmxAACAudioTrackEncoder::AppendEncodedFrames(). Consider sharing code in one common function?

@@ +359,5 @@
> +    segment.AppendFrom(&mRawSegment);
> +  }
> +
> +  // AMR encoder won't provide csd
> +  if (!mCsdProvided) {

How about returning this "fake" AMR CSD in OMXAudioEncoder::AppendDecoderConfig()? This way the whole function will be identical to OmxAACAudioTrackEncoder::GetEncodedTrack() and the code can be shared.

::: content/media/omx/OMXCodecWrapper.cpp
@@ +65,5 @@
>    return avc.forget();
>  }
>  
>  OMXCodecWrapper::OMXCodecWrapper(CodecType aCodecType)
> +  : mCodecType(aCodecType),

Move comma to the beginning of next line.

@@ +534,5 @@
>          NS_ENSURE_TRUE(result == OK, NS_ERROR_FAILURE);
>        }
>  
> +      nsAutoTArray<AudioDataValue, 9600> pcm;
> +      pcm.SetLength(bytesToCopy);

pcm is only used in "if (mResampler) {...}". It seems no need to allocate/init outside the scope.

@@ +551,5 @@
> +        } else {
> +          AudioTrackEncoder::InterleaveTrackData(chunk, sourceSamplesToCopy,
> +                                                 mChannels,
> +                                                 dst);
> +          dstSamplesCopied = sourceSamplesToCopy * sourceSamplesToCopy;

Do you mean "dstSamplesCopied = sourceSamplesToCopy;" here?

::: content/media/omx/OMXCodecWrapper.h
@@ +83,5 @@
>      kAACFrameDuration = 1024, // How many samples per AAC frame.
>    };
>  
> +  enum {
> +    kAMRNBBitrate = 12200,      // AMRNB kbps

This constant is not needed by track encoder. Consider moving it to cpp file?

@@ +183,5 @@
>    /**
>     * Configure audio codec parameters and start media codec. It must be called
>     * before calling Encode() and GetNextEncodedFrame().
>     */
> +  nsresult Configure(int aChannelCount, int aSamplingRate, int aReSamplingRate);

Personal taste: How about giving aResamplingRate a default value (such as 0) to indicate 'no resampling' case?

Comment 17

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: patch v2

Thanks John's feedback to this patch, 
Fix nits and update to a new one.

Comment 18

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

Comment on attachment 8381935 [details] [diff] [review]
patch v2

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

::: content/media/MediaRecorder.cpp
@@ +323,5 @@
>      // At this stage, the API doesn't allow UA to choose the output mimeType format.
> +
> +    if (mRecorder->mAppStatus == nsIPrincipal::APP_STATUS_CERTIFIED &&
> +        aTrackTypes == ContainerWriter::CREATE_AUDIO_TRACK) {
> +      mEncoder = MediaEncoder::CreateEncoder(NS_LITERAL_STRING(AUDIO_MP4), aTrackTypes);

I don't understand why we're doing this.

@@ +688,5 @@
>    bool subsumes;
>    if (NS_FAILED(doc->NodePrincipal()->Subsumes(principal, &subsumes)))
>      return false;
>  
> +  doc->NodePrincipal()->GetAppStatus(&mAppStatus);

I don't think you need to do this or store mAppStatus. You can just get it when you need it.

::: content/media/encoder/MediaEncoder.cpp
@@ +126,5 @@
>      NS_ENSURE_TRUE(videoEncoder, nullptr);
>      mimeType = NS_LITERAL_STRING(VIDEO_MP4);
>    }
> +  else if (MediaEncoder::IsOMXEncoderEnabled() &&
> +          (aMIMEType.EqualsLiteral(AUDIO_MP4))) {

Why is audio/mp4 the right MIME type here? Shouldn't we require some extra codec parameter to force use of AMR-NB? We definitely don't want an app to accidentally use AMR for audio!

::: content/media/omx/OMXCodecWrapper.cpp
@@ +363,5 @@
>    aOutputBuf->AppendElements(aData + sizeof(length), aSize);
>  }
>  
>  nsresult
> +OMXAudioEncoder::Configure(int aChannels, int aSamplingRate, int aResamplingRate)

These parameter names are unclear. aInputSampleRate, aEncodedSampleRate?

Please move all the resampling support changes out into a separate patch.

Also, the OMX changes can be their own separate patch, with the MediaRecorder and MediaEncoder changes in their own patch.

Comment 19

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

This request comes from our partner for their special requirement.
As discuss before, we allow this kind of format can be generated on certified application.
If we don't want to let application have exception for that,
For API level, The Bug 963481  [Media Recorder] Implement MediaRecorder Constrainable Properties
still under discussing, Could I add permission for this? like amrnb-encode?

Comment 20

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

We can encode the AMR codec request in the MIME type. Something like
audio/mp4; codecs="samr"

We do need a way to request a specific MIME type. I think for now we should have a MediaRecorderOptions dictionary we can pass to the constructor, with a "DOMString mimeType" member.

Comment 21

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: omx patch part 1 v2

Separate patch for omx encoder.

Comment 22

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: recorder wip part 2 v2

still under development, wip

Comment 23

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

Comment on attachment 8382813 [details] [diff] [review]
omx patch part 1 v2

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

::: content/media/omx/OMXCodecWrapper.cpp
@@ +374,2 @@
>  
> +  if (aEncodedSampleRate) {

Shouldn't we be testing aInputSampleRate == aEncodedSampleRate?

@@ +552,5 @@
> +                                                 pcm.Elements());
> +          uint32_t inframes = sourceSamplesToCopy;
> +          short* in = reinterpret_cast<short*>(pcm.Elements());
> +          speex_resampler_process_interleaved_int(mResampler, in, &inframes,
> +                                                              dst, &dstSamplesCopied);

We probably need to do something to preserve the right length, calling speex_resampler_skip_zeros.

Comment 24

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: omx patch part 1 v2.1

Fix nits.

Comment 25

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: add audio/3gpp mime type part 3

Comment 26

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: media recorder change v3

Get the mimeType assigned from application and create proper encoder for certificated application.

Comment 27

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

Comment on attachment 8389679 [details] [diff] [review]
media recorder change v3

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

This patch is ok and we should land it.

However, we should do some refactoring so that checks for allowed media types are made in one place and shared by both MediaRecorder and DecoderTraits.

::: content/media/MediaRecorder.cpp
@@ +326,5 @@
> +    uint16_t appStatus = nsIPrincipal::APP_STATUS_NOT_INSTALLED;
> +    if (doc) {
> +      doc->NodePrincipal()->GetAppStatus(&appStatus);
> +    }
> +    // Only allow certificated application can assign AUDIO_3GPP

// Only allow certified application to assign AUDIO_3GPP

::: content/media/MediaRecorder.h
@@ +75,5 @@
>  
>    static already_AddRefed<MediaRecorder>
>    Constructor(const GlobalObject& aGlobal,
> +              DOMMediaStream& aStream,
> +              ErrorResult& aRv);

You can delete this constructor. It won't be called now.

Comment 28

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Thanks for review, We would modify and land it after the branch of 1.4.

Comment 29

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

Attachment: check-in patch

carry reviewer, 
try result:
https://tbpl.mozilla.org/?tree=Try&rev=13f2764bc9e2

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/8aa9ab187f81

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8aa9ab187f81