971641 - [MediaEncoder] Support AMR audio format in 3GP

Status: RESOLVED FIXED

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

Comment 1

[Alfredo Yang (:alfredo)]

Attachment: add_amr_meta

patch-1 wip

Comment 2

[Alfredo Yang (:alfredo)]

Attachment: add_amr_box

wip

Comment 3

[Alfredo Yang (:alfredo)]

Hi Randy,
Please update your patch about AMR metadata in bug https://bugzilla.mozilla.org/show_bug.cgi?id=959490.
Thanks.

Comment 4

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

ok, I will update a new one later.

Comment 5

[Alfredo Yang (:alfredo)]

Attachment: add_amr_meta

Comment 6

[Alfredo Yang (:alfredo)]

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

Comment 7

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

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?

Comment 8

[Alfredo Yang (:alfredo)]

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

Comment 9

[Alfredo Yang (:alfredo)]

Attachment: add_amr_meta

Comment 10

[Alfredo Yang (:alfredo)]

Attachment: add_amr_box

Comment 11

[Alfredo Yang (:alfredo)]

Attachment: add_amr_box

Comment 12

[Alfredo Yang (:alfredo)]

Attachment: add_amr_meta

Comment 13

[Alfredo Yang (:alfredo)]

Attachment: add_amr_meta

wip, workable for both recording and playback on nexus4.

Comment 14

[Alfredo Yang (:alfredo)]

Attachment: add_amr_meta

1. Add AMR metadata type for encoder in ISOTrackMetadata.h
2. Refactory MetaHelper in ISOMediaBoxes to retrieve codec info among different meta types.

Comment 15

[Alfredo Yang (:alfredo)]

Attachment: add_amr_box

1. Add AMR box in ISOMediaBoxes.cpp.
2. Create different codec box in ISOMediaBoxes according to metadata type.

Comment 16

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

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?

Comment 17

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

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?

Comment 18

[Alfredo Yang (:alfredo)]

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

Comment 19

[Alfredo Yang (:alfredo)]

(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()?

Comment 20

[Alfredo Yang (:alfredo)]

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

Comment 21

[Alfredo Yang (:alfredo)]

Attachment: 1_abstract_metadata

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.

Comment 22

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

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.

Comment 23

[Alfredo Yang (:alfredo)]

Attachment: 1_abstract_metadata

Comment 24

[Alfredo Yang (:alfredo)]

Attachment: 2_add_amr_meta

Comment 25

[Alfredo Yang (:alfredo)]

Attachment: 3_add_amr_box

Comment 26

[Alfredo Yang (:alfredo)]

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

Comment 27

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

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?

Comment 28

[Alfredo Yang (:alfredo)]

Attachment: 1_abstract_metadata

Rebase, carry r+.

Comment 29

[Alfredo Yang (:alfredo)]

Attachment: 2_add_amr_meta

Rebase, carry r+.

Comment 30

[Alfredo Yang (:alfredo)]

Attachment: 3_add_amr_box

Rebase, carry r+.

https://tbpl.mozilla.org/?tree=Try&rev=9f76f3c9e79c

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/a6abe0d4e0be
https://hg.mozilla.org/integration/b2g-inbound/rev/b7f5aaff6558
https://hg.mozilla.org/integration/b2g-inbound/rev/819d7aa12df9

Comment 32

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

https://hg.mozilla.org/mozilla-central/rev/a6abe0d4e0be
https://hg.mozilla.org/mozilla-central/rev/b7f5aaff6558
https://hg.mozilla.org/mozilla-central/rev/819d7aa12df9