Closed Bug 959490 Opened 6 years ago Closed 6 years ago

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

Categories

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

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
1.4 S2 (28feb)

People

(Reporter: rlin, Assigned: rlin)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [ft:multimedia-platform] [m+])

Attachments

(1 file, 12 obsolete files)

32.50 KB, patch
Details | Diff | Splinter Review
No description provided.
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.
Flags: needinfo?(mkhoo)
Component: General → Video/Audio
Product: Firefox OS → Core
Depends on: 959861
Flags: needinfo?(mkhoo)
Summary: [MediaEncoder] Support m4a audio format → [MediaEncoder] Support *.3gp with AMR-NB audio format
Hi Randy, as dicussed, i modified the REQ here.
Hi Alfredo, 
Could you evolute how much work should we do for this task?
Flags: needinfo?(ayang)
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.
Flags: needinfo?(ayang)
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.
Whiteboard: [ucid:multimedia11, 1.4, ft:multimedia-platform]
Assignee: nobody → rlin
Summary: [MediaEncoder] Support *.3gp with AMR-NB audio format → [MediaEncoder] Support *.3gp with AMR-NB audio format on certificated application
Blocks: 972747
Attached patch wip (obsolete) — Splinter Review
todo: resample
Target Milestone: --- → 1.4 S2 (28feb)
Correct dependency
Blocks: 973765
No longer blocks: 1.4-multimedia
Whiteboard: [ucid:multimedia11, 1.4, ft:multimedia-platform] → [ft:multimedia-platform]
Attached patch wip2 (obsolete) — Splinter Review
Attachment #8376995 - Attachment is obsolete: true
Attached patch wip2.1 (obsolete) — Splinter Review
Attachment #8377408 - Attachment is obsolete: true
Attached patch wip v3 (obsolete) — Splinter Review
wip v3, add csd, fix sound problem.
Attachment #8377430 - Attachment is obsolete: true
Attached patch wip v3 (obsolete) — Splinter Review
update to the correct one.
Attachment #8379047 - Attachment is obsolete: true
Attached patch patch v1 (obsolete) — Splinter Review
Hi John, 
Could you help to give FB for this one? 
Thanks.
Attachment #8379353 - Attachment is obsolete: true
Attachment #8380503 - Flags: feedback?(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?
Attachment #8380503 - Flags: feedback?(jolin) → feedback+
Component: Video/Audio → Video/Audio: Recording
Attached patch patch v2 (obsolete) — Splinter Review
Thanks John's feedback to this patch, 
Fix nits and update to a new one.
Attachment #8381935 - Flags: review?(roc)
Attachment #8380503 - Attachment is obsolete: true
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.
Attachment #8381935 - Flags: review?(roc) → review-
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?
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc)
Attached patch omx patch part 1 v2 (obsolete) — Splinter Review
Separate patch for omx encoder.
Attachment #8381935 - Attachment is obsolete: true
Attachment #8382813 - Flags: review?(roc)
Attached patch recorder wip part 2 v2 (obsolete) — Splinter Review
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.
Attachment #8382813 - Flags: review?(roc) → review-
Attached patch omx patch part 1 v2.1 (obsolete) — Splinter Review
Fix nits.
Attachment #8382813 - Attachment is obsolete: true
Attachment #8384631 - Flags: review?(roc)
Attached patch media recorder change v3 (obsolete) — Splinter Review
Get the mimeType assigned from application and create proper encoder for certificated application.
Attachment #8382815 - Attachment is obsolete: true
Attachment #8389679 - Flags: review?(roc)
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.
Attachment #8389679 - Flags: review?(roc) → review+
Thanks for review, We would modify and land it after the branch of 1.4.
Attachment #8389679 - Attachment is obsolete: true
Attachment #8382815 - Attachment is obsolete: true
Attachment #8384631 - Attachment is obsolete: true
Attachment #8384638 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/8aa9ab187f81
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [ft:multimedia-platform] → [ft:multimedia-platform] [m+]
Duplicate of this bug: 963481
Flags: in-moztrap?(pyang)
Duplicate of this bug: 923035
Flags: in-moztrap?(pyang)
You need to log in before you can comment on or make changes to this bug.