Closed Bug 971641 Opened 6 years ago Closed 6 years ago

[MediaEncoder] Support AMR audio format in 3GP

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
1.4 S3 (14mar)

People

(Reporter: ayang, Assigned: ayang)

References

(Blocks 1 open bug)

Details

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

Attachments

(3 files, 14 obsolete files)

33.11 KB, patch
ayang
: review+
Details | Diff | Splinter Review
8.00 KB, patch
ayang
: review+
Details | Diff | Splinter Review
8.37 KB, patch
ayang
: review+
Details | Diff | Splinter Review
No description provided.
Blocks: MediaEncoder
Attached patch add_amr_meta (obsolete) — Splinter Review
patch-1 wip
Blocks: 973765
Target Milestone: --- → 1.4 S3 (14mar)
Whiteboard: [ft:multimedia-platform]
Attached patch add_amr_box (obsolete) — Splinter Review
wip
Hi Randy,
Please update your patch about AMR metadata in bug https://bugzilla.mozilla.org/show_bug.cgi?id=959490.
Thanks.
Flags: needinfo?(rlin)
Depends on: 971639
ok, I will update a new one later.
Flags: needinfo?(rlin)
Attached patch add_amr_meta (obsolete) — Splinter Review
Attachment #8376980 - Attachment is obsolete: true
Hi Chris,

While writing the AMR 3GP muxing function, I found the audio type defined in [1] is not good enough, the "AUDIO_FRAME" is used for AAC only which is not good for other audio, like AMR. The quickly way is to add a new audio type like "AMR_AUDIO_FRAME" but IMHO it may not easy to maintain when supported codec type increases.

So I plan to re-factory this part, my current plan is to remove this enumeration "FrameType" out of EncodedFrameContainer.h to codec/muxer specific header.

For example, move AAC_CSD, AVC_CSD to ISOTrackMetadata.h. Add AVC_I_FRAME, AVC_B_FRAME, and AVC_P_FRAME to ISOTrackMetadata.h.

So it can reduce the dependency to MediaEncoder layer for both muxer and encoder. And reduce the unnecessary info exposes to MediaEncoder layer.
Do you think it make sense?

[1] http://dxr.mozilla.org/mozilla-central/source/content/media/encoder/EncodedFrameContainer.h#54
Flags: needinfo?(cpearce)
If you use two separate enumerations to define the type contained by the EncodedFrame, how will you ensure that in future someone does not add two names for the same value. i.e. what is to stop someone defining AMR_AUDIO_FRAME=2 and AVC_I_FRAME=2?
Flags: needinfo?(cpearce)
(In reply to Chris Pearce (:cpearce) from comment #7)
> If you use two separate enumerations to define the type contained by the
> EncodedFrame, how will you ensure that in future someone does not add two
> names for the same value. i.e. what is to stop someone defining
> AMR_AUDIO_FRAME=2 and AVC_I_FRAME=2?

No, we can't stop it.
hmm... From this perspective, it'd be better to have a central enum in EncodedFrame.
And rename the original enum items to have a better name.
Attached patch add_amr_meta (obsolete) — Splinter Review
Attachment #8377378 - Attachment is obsolete: true
Attachment #8377385 - Attachment is obsolete: true
Attached patch add_amr_box (obsolete) — Splinter Review
Attached patch add_amr_box (obsolete) — Splinter Review
Attachment #8378049 - Attachment is obsolete: true
Attachment #8378050 - Attachment is obsolete: true
Attached patch add_amr_meta (obsolete) — Splinter Review
Component: Video/Audio → Video/Audio: Recording
Attached patch add_amr_meta (obsolete) — Splinter Review
wip, workable for both recording and playback on nexus4.
Attachment #8379631 - Attachment is obsolete: true
Attached patch add_amr_meta (obsolete) — Splinter Review
1. Add AMR metadata type for encoder in ISOTrackMetadata.h
2. Refactory MetaHelper in ISOMediaBoxes to retrieve codec info among different meta types.
Attachment #8379630 - Attachment is obsolete: true
Attachment #8381221 - Attachment is obsolete: true
Attachment #8382847 - Flags: review?(cpearce)
Attached patch add_amr_box (obsolete) — Splinter Review
1. Add AMR box in ISOMediaBoxes.cpp.
2. Create different codec box in ISOMediaBoxes according to metadata type.
Attachment #8382849 - Flags: review?(cpearce)
Comment on attachment 8382847 [details] [diff] [review]
add_amr_meta

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

::: content/media/encoder/fmp4_muxer/ISOMediaBoxes.cpp
@@ +269,5 @@
>          // default_sample_duration.
>          MOZ_ASSERT(0);
>          default_sample_duration = 0;
>        } else {
> +        default_sample_duration = mMeta.GetVideoFrequency() / mMeta.GetVideoFrameRate();

You should check that mMeta.GetVideoFrameRate() is not 0 before dividing by it!

@@ +447,2 @@
>        default_sample_duration =
> +        mMeta.GetVideoFrequency() / mMeta.GetVideoFrameRate();

You should check that mMeta.GetVideoFrameRate() is not 0 before dividing by it!

::: content/media/encoder/fmp4_muxer/ISOMediaBoxes.h
@@ +58,1 @@
>    class MetaHelper {

It seems like the fact that you only need a MetaHelper class because your design of TrackMetaData is too specific. That is, you want a non-specific interface, so you wrote MetaHelper to adapt the too specific TrackMetadata for audio to a single interface. So why not just have an AudioTrackMetadata that conforms to the less specific interface, and implement if for AAC and for AMR.

e.g.:

class AudioTrackMetadata : public TrackMetadata {
public:
  virtual uint32_t GetAudioFrameDuration() = 0;
  virtual uint32_t GetAudioFrameSize() = 0;
  virtual uint32_t GetAudioSampleRate() = 0;
  virtual uint32_t GetAudioChannels() = 0;
  virtual TrackMetadataBase::MetadataKind GetAudioType() = 0;
};

class AACTrackMetadata : public AudioTrackMetadata /* ... */
class AMRTrackMetadata : public AudioTrackMetadata /* ... */

Are there reasons why that is a bad idea?

@@ +58,5 @@
>    class MetaHelper {
>    public:
>      nsresult Init(ISOControl* aControl);
> +
> +    uint32_t GetVideoFrequency();

"Frequency" usually implies a value measured in Hz, i.e. samples per second. So it's not clear how VideoFrequency() and VideoFrameRate() are different. Is VideoFrequency() the count of the number of frames seen so far?

You should rename VideoFrequency() so it's clear how it differs from VideoFrameRate().

::: content/media/encoder/fmp4_muxer/ISOTrackMetadata.h
@@ +57,5 @@
> +  };
> +  AMRType Type;
> +
> +  const uint32_t SampleRate;    // AMR sample rate is always 8k.
> +  const uint32_t Channels;      // Channel number is always 1.

Do you need  FrameDuration and FrameSize, like you have for AACTrackMetadata?

Does it make sense for TrackMetadataHelper to return 0 for AMR for these fields?
Attachment #8382847 - Flags: review?(cpearce) → review-
Comment on attachment 8382849 [details] [diff] [review]
add_amr_box

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

::: content/media/encoder/fmp4_muxer/ISOMediaBoxes.h
@@ +352,5 @@
>    ~TrackFragmentBox();
>  
>  protected:
>    uint32_t mTrackType;
> +  MetaHelper mMeta;

Could this be:

nsRefPtr<AudioTrackMetadata> mAudioMetadata;
nsRefPtr<VideoTrackMetadata> mVideoMetadata;

Or is there a good reason why you need a helper?
(In reply to Chris Pearce (:cpearce) from comment #16)
> Comment on attachment 8382847 [details] [diff] [review]
> add_amr_meta
> 
> Review of attachment 8382847 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/encoder/fmp4_muxer/ISOMediaBoxes.cpp
> @@ +269,5 @@
> >          // default_sample_duration.
> >          MOZ_ASSERT(0);
> >          default_sample_duration = 0;
> >        } else {
> > +        default_sample_duration = mMeta.GetVideoFrequency() / mMeta.GetVideoFrameRate();
> 
> You should check that mMeta.GetVideoFrameRate() is not 0 before dividing by
> it!
> 
> @@ +447,2 @@
> >        default_sample_duration =
> > +        mMeta.GetVideoFrequency() / mMeta.GetVideoFrameRate();
> 
> You should check that mMeta.GetVideoFrameRate() is not 0 before dividing by
> it!
> 

I'll fix it.

> ::: content/media/encoder/fmp4_muxer/ISOMediaBoxes.h
> @@ +58,1 @@
> >    class MetaHelper {
> 
> It seems like the fact that you only need a MetaHelper class because your
> design of TrackMetaData is too specific. That is, you want a non-specific
> interface, so you wrote MetaHelper to adapt the too specific TrackMetadata
> for audio to a single interface. So why not just have an AudioTrackMetadata
> that conforms to the less specific interface, and implement if for AAC and
> for AMR.
> 
> e.g.:
> 
> class AudioTrackMetadata : public TrackMetadata {
> public:
>   virtual uint32_t GetAudioFrameDuration() = 0;
>   virtual uint32_t GetAudioFrameSize() = 0;
>   virtual uint32_t GetAudioSampleRate() = 0;
>   virtual uint32_t GetAudioChannels() = 0;
>   virtual TrackMetadataBase::MetadataKind GetAudioType() = 0;
> };
> 
> class AACTrackMetadata : public AudioTrackMetadata /* ... */
> class AMRTrackMetadata : public AudioTrackMetadata /* ... */
> 
> Are there reasons why that is a bad idea?
> 

That's a good idea. I'll change this part.

> @@ +58,5 @@
> >    class MetaHelper {
> >    public:
> >      nsresult Init(ISOControl* aControl);
> > +
> > +    uint32_t GetVideoFrequency();
> 
> "Frequency" usually implies a value measured in Hz, i.e. samples per second.
> So it's not clear how VideoFrequency() and VideoFrameRate() are different.
> Is VideoFrequency() the count of the number of frames seen so far?
> 

No, VideoFrquency() is "how many Hz per second" here, which uses to measure the playback duration of a frame.
In H264, that should be 90k Hz, so a frame playback duration should be 3000 Hz if VideoFrameRate() is 30 per second.

> You should rename VideoFrequency() so it's clear how it differs from
> VideoFrameRate().
> 
> ::: content/media/encoder/fmp4_muxer/ISOTrackMetadata.h
> @@ +57,5 @@
> > +  };
> > +  AMRType Type;
> > +
> > +  const uint32_t SampleRate;    // AMR sample rate is always 8k.
> > +  const uint32_t Channels;      // Channel number is always 1.
> 
> Do you need  FrameDuration and FrameSize, like you have for AACTrackMetadata?
> 
> Does it make sense for TrackMetadataHelper to return 0 for AMR for these
> fields?

FrameDuration and FrameSize is used if encoder generates the same duration and size in WriteEncodedTrack(). If encoder can't guarantee them, 0 means variant. So return 0 should make sense.
Attachment #8382849 - Flags: review?(cpearce)
(In reply to Chris Pearce (:cpearce) from comment #16)
> You should rename VideoFrequency() so it's clear how it differs from
> VideoFrameRate().
> 

Is VideoFrequencyClock() better? Or VideoClockRate()?
(In reply to Chris Pearce (:cpearce) from comment #17)
> Comment on attachment 8382849 [details] [diff] [review]
> add_amr_box
> 
> Review of attachment 8382849 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/encoder/fmp4_muxer/ISOMediaBoxes.h
> @@ +352,5 @@
> >    ~TrackFragmentBox();
> >  
> >  protected:
> >    uint32_t mTrackType;
> > +  MetaHelper mMeta;
> 
> Could this be:
> 
> nsRefPtr<AudioTrackMetadata> mAudioMetadata;
> nsRefPtr<VideoTrackMetadata> mVideoMetadata;
> 
> Or is there a good reason why you need a helper?

I'll replace the helper with: 
nsRefPtr<AudioTrackMetadata> mAudioMetadata;
nsRefPtr<VideoTrackMetadata> mVideoMetadata;
Attached patch 1_abstract_metadata (obsolete) — Splinter Review
According to previous review, I changed following things.
1. Add AudioTrackMetadata and VideoTrackMetadata class
2. Remove MetaHelp class
3. Rename VideoFrequency to VideoClockRate

Hi Chris,
Could you please feedback about this?
Thanks.
Attachment #8382847 - Attachment is obsolete: true
Attachment #8382849 - Attachment is obsolete: true
Attachment #8385941 - Flags: feedback?(cpearce)
Comment on attachment 8385941 [details] [diff] [review]
1_abstract_metadata

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

Looks good! Calling it VideoClockRate is fine, or just TimeScale.

::: content/media/encoder/OmxTrackEncoder.cpp
@@ +63,5 @@
>    if (mCanceled || mEncodingComplete) {
>      return nullptr;
>    }
>  
>    nsRefPtr<AVCTrackMetadata> meta = new AVCTrackMetadata();

If VideoClockRate *must* be 90kHz for AVC, then I think you should set VideoClockRate in the constructor of AVCTrackMetadata() and maybe even make it const.

::: content/media/encoder/TrackMetadataBase.h
@@ +41,5 @@
> +class VideoTrackMetadata : public TrackMetadataBase {
> +public:
> +  virtual uint32_t GetVideoHeight() = 0;
> +  virtual uint32_t GetVideoWidth() = 0;
> +  virtual uint32_t GetVideoClockRate() = 0;

I think you should add comments to explain that VideoClockRate is the number of samples per second in an (encoded?) video frame's timestamp. Whereas FrameRate is the number of frames per second. So that it's clear.
Attachment #8385941 - Flags: feedback?(cpearce) → feedback+
Attached patch 1_abstract_metadata (obsolete) — Splinter Review
Attachment #8385941 - Attachment is obsolete: true
Attachment #8387433 - Flags: review?(cpearce)
Attached patch 2_add_amr_meta (obsolete) — Splinter Review
Attachment #8387434 - Flags: review?(cpearce)
Attached patch 3_add_amr_box (obsolete) — Splinter Review
Attachment #8387436 - Flags: review?(cpearce)
(In reply to Chris Pearce (:cpearce) from comment #22)
> Comment on attachment 8385941 [details] [diff] [review]
> 1_abstract_metadata
> 
> Review of attachment 8385941 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good! Calling it VideoClockRate is fine, or just TimeScale.
> 
> ::: content/media/encoder/OmxTrackEncoder.cpp
> @@ +63,5 @@
> >    if (mCanceled || mEncodingComplete) {
> >      return nullptr;
> >    }
> >  
> >    nsRefPtr<AVCTrackMetadata> meta = new AVCTrackMetadata();
> 
> If VideoClockRate *must* be 90kHz for AVC, then I think you should set
> VideoClockRate in the constructor of AVCTrackMetadata() and maybe even make
> it const.
> 
> ::: content/media/encoder/TrackMetadataBase.h
> @@ +41,5 @@
> > +class VideoTrackMetadata : public TrackMetadataBase {
> > +public:
> > +  virtual uint32_t GetVideoHeight() = 0;
> > +  virtual uint32_t GetVideoWidth() = 0;
> > +  virtual uint32_t GetVideoClockRate() = 0;
> 
> I think you should add comments to explain that VideoClockRate is the number
> of samples per second in an (encoded?) video frame's timestamp. Whereas
> FrameRate is the number of frames per second. So that it's clear.

Thanks for feedback. I've updated the patch.
Attachment #8387433 - Flags: review?(cpearce) → review+
Attachment #8387436 - Flags: review?(cpearce) → review+
Comment on attachment 8387434 [details] [diff] [review]
2_add_amr_meta

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

::: content/media/encoder/fmp4_muxer/ISOControl.cpp
@@ +66,2 @@
>      mCSDFrame = aFrame;
>      // Ue CSD's timestamp as the start time. Encoder should send CSD frame first

s/Ue/Use/

::: content/media/encoder/fmp4_muxer/ISOTrackMetadata.h
@@ +89,5 @@
> +
> +  // AMRTrackMetadata members
> +  enum AMRType {
> +    AMR_NB,
> +    AMR_WB

You don't ever set Type to AMR_WB. Do you need it?

@@ +91,5 @@
> +  enum AMRType {
> +    AMR_NB,
> +    AMR_WB
> +  };
> +  AMRType Type;

Can this be private and/or const? Especially given that you have an accessor?
Attachment #8387434 - Flags: review?(cpearce) → review+
Rebase, carry r+.
Attachment #8387433 - Attachment is obsolete: true
Attachment #8387434 - Attachment is obsolete: true
Attachment #8387436 - Attachment is obsolete: true
Attachment #8392105 - Flags: review+
Attached patch 2_add_amr_metaSplinter Review
Rebase, carry r+.
Attachment #8392106 - Flags: review+
Keywords: checkin-needed
Whiteboard: [ft:multimedia-platform] → [ft:multimedia-platform][m+]
You need to log in before you can comment on or make changes to this bug.