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.

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

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

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.

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.

had ping Robert to this, will communicate with him.

(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. ;)

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.

todo: resample

Correct dependency

wip v3, add csd, fix sound problem.

update to the correct one.

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

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?

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

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.

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?

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.

Separate patch for omx encoder.

still under development, wip

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.

Fix nits.

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

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.

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

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