879668 - [MediaEncoder] Implement OmxTrackEncoder on B2G

Status: RESOLVED FIXED

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

Description

[u459114]

MediaOmxEncoder(MediaOmxWriter) is a subclass of MediaEncoder(MediaWriter).
MediaOmxEncoder/MediaOmxWriter is front end wrapper for OMX AL back-end(StageFright for Android, GStreamer for Linux and WMF for Windows platform).

With MediaOmxEncoder support, B2G may take advantage of HW OMX IL component.

Comment 1

[Shelly Lin [:shelly]]

Should probably be:

For the media container writer,
class OMXWriter: public ContainerWriter

For the audio/video track encoder
class H264TrackEncoder: public VideoTrackEncoder

Comment 2

[u459114]

Compare to encoder module, we have 
1. MediaOmxDecoder which is inherited from MediaDecoder
2. MediaOmxReader which is inherited from MediaDecoderReader.

We might have similar class relation in MediaEncoder framework as well.

Comment 3

[u459114]

The main reason for OmxEncoder/ Writer construction is to support H.264/ AAC in MP4.

Comment 4

[Shelly Lin [:shelly]]

Attachment: [draft] Class structure

Hi Roc,

I'm kindda puzzled when setting up class structure for Omx encoder.

1. OmxH264TrackEncoder.h/.cpp and OmxAACTrackEncoder.h/.cpp

Technically speaking, Omx is not a codec nor a container format, thus I prefix the class name with "Omx", and follow by the video/audio codec. I thought about naming them to OmxAudioTrackEncoder and OmxVideoTrackEncoder, but they will use H264/AAC anyway though.

2. OmxWriter.h/.cpp

I think mpeg4 is the only container format we want to support? If so, should it be "OmxMPEG4Writer"? Also, there are MediaOmxDecoder, MediaOmxReader and OmxDecoder in the omx folder, looks like the one with "MediaOmx" is the wrapper of our MediaDecoder, and OmxDecoder is the actual instance of an android Omx decoder. I chose OmxWriter over MediaOmxWriter due to itself is the actual implementation class, and other decoders do not prefix their name w/ "Media".

3. "MPEG4Writer" from libstagefright takes care of packing encoded track data and outputting final container data together, so our OmxH264TrackEncoder and OmxAACTrackEncoder would mostly just setting up <stagefright/MediaSource> for <stagefright/MediaWriter>.

I'm not even sure we (who ever is in charge of?) are on the same page of implementing Omx encoder as the next step, but it would be great to hear some feedback from you, of course anyone is welcome too!

Comment 5

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

(In reply to Shelly Lin [:shelly] from comment #4)
> 1. OmxH264TrackEncoder.h/.cpp and OmxAACTrackEncoder.h/.cpp
> 
> Technically speaking, Omx is not a codec nor a container format, thus I
> prefix the class name with "Omx", and follow by the video/audio codec. I
> thought about naming them to OmxAudioTrackEncoder and OmxVideoTrackEncoder,
> but they will use H264/AAC anyway though.

Those names sound good.

> 2. OmxWriter.h/.cpp
> 
> I think mpeg4 is the only container format we want to support? If so, should
> it be "OmxMPEG4Writer"? Also, there are MediaOmxDecoder, MediaOmxReader and
> OmxDecoder in the omx folder, looks like the one with "MediaOmx" is the
> wrapper of our MediaDecoder, and OmxDecoder is the actual instance of an
> android Omx decoder. I chose OmxWriter over MediaOmxWriter due to itself is
> the actual implementation class, and other decoders do not prefix their name
> w/ "Media".

OmxMP4Writer sounds good to me.

> 3. "MPEG4Writer" from libstagefright takes care of packing encoded track
> data and outputting final container data together, so our
> OmxH264TrackEncoder and OmxAACTrackEncoder would mostly just setting up
> <stagefright/MediaSource> for <stagefright/MediaWriter>.
> 
> I'm not even sure we (who ever is in charge of?) are on the same page of
> implementing Omx encoder as the next step, but it would be great to hear
> some feedback from you, of course anyone is welcome too!

I think doing the OMX decoder next is definitely a good next step, mainly because it's unclear which unencumbered format we should use for video cross-platform.

Comment 6

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

Comment on attachment 772599 [details] [diff] [review]
[draft] Class structure

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

There's not much here, but what there is, is fine.

Comment 7

[u459114]

Hi Shelly,
In media playback module, there is no need to subclass for each OMX decoder. Extractor analyze meta data of a media source and choice correct ILComponent.

So, alternative architecture is
We may use the same strategy here: have a single OmxAudioTrachEncoder(OmxVideoTrackEncoder) and create a class, named as incoporator as oppose to extractor maybe, use this incoporator to pick up correct ILCompenent in StageFright.

With this, we prevent to create tons of encoder subclass in MediaEncoder module.

Comment 8

[Shelly Lin [:shelly]]

Hmmm, consider OMX might support vp9/opus someday in the future, name those track encoder classes to OmxAudioTrackEncoder/OmxVideoTrackEncoder might be a better idea.

On the other hand, I'm investigating the possibility of encapsulating H264/AAC track encoded data into Ogg container, so that we can reuse the current Ogg container writer.

Comment 9

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

(In reply to Shelly Lin [:shelly] from comment #8)
> On the other hand, I'm investigating the possibility of encapsulating
> H264/AAC track encoded data into Ogg container, so that we can reuse the
> current Ogg container writer.

We need to use MP4 as the container.

Comment 10

[u459114]

Transfer to John since Shelly is working on other topics.

Some ideas
1. Plug-in architecture: define plug-in interface to that 3rd party or partner can distribute their own track codec on the device easily.
2. Rename the base class name from OmxEncoder to PlatformEncoder. This class is literally an wrapper of Platform codec(StageFright/ GStreamer/ WMF).

Comment 11

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

(In reply to C.J. Ku[:CJKu] from comment #10)
> 1. Plug-in architecture: define plug-in interface to that 3rd party or
> partner can distribute their own track codec on the device easily.

Isn't libstagefright that plug-in interface for FirefoxOS?

> 2. Rename the base class name from OmxEncoder to PlatformEncoder. This class
> is literally an wrapper of Platform codec(StageFright/ GStreamer/ WMF).

Won't we have different classes for different platforms? Will the different platform encoders share much code?

Comment 12

[u459114]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> (In reply to C.J. Ku[:CJKu] from comment #10)
> > 1. Plug-in architecture: define plug-in interface to that 3rd party or
> > partner can distribute their own track codec on the device easily.
> 
> Isn't libstagefright that plug-in interface for FirefoxOS?
> 
Yes, you are right
> > 2. Rename the base class name from OmxEncoder to PlatformEncoder. This class
> > is literally an wrapper of Platform codec(StageFright/ GStreamer/ WMF).
> 
> Won't we have different classes for different platforms? Will the different
> platform encoders share much code?
My opinion is to have one single class definition, different member function definition, for all platforms. As a result, we only need to know PlatformEncoder while constructing encoding pipeline. For example, we want to use h.264 as video encoding fomat.
1. Is there any native encoder support this format? Yes, choose this native encoder; No, goto #2
2. Query the capability of Platform encoder, if h.264 is supported in this platform encoder, choose this one. Otherwise, return an error code indicate that format not support.

We don't really need to know this platform encoder is OpenMax-base or WMF(DirectShow) -base from class name.

Comment 13

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

(In reply to C.J. Ku[:CJKu] from comment #12)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> > (In reply to C.J. Ku[:CJKu] from comment #10)
> > > 1. Plug-in architecture: define plug-in interface to that 3rd party or
> > > partner can distribute their own track codec on the device easily.
> > 
> > Isn't libstagefright that plug-in interface for FirefoxOS?
>
> Yes, you are right

OK, I don't think we should build another one without a very good reason :-).

> > > 2. Rename the base class name from OmxEncoder to PlatformEncoder. This class
> > > is literally an wrapper of Platform codec(StageFright/ GStreamer/ WMF).
> > 
> > Won't we have different classes for different platforms? Will the different
> > platform encoders share much code?
> My opinion is to have one single class definition, different member function
> definition, for all platforms. As a result, we only need to know
> PlatformEncoder while constructing encoding pipeline. For example, we want
> to use h.264 as video encoding fomat.
> 1. Is there any native encoder support this format? Yes, choose this native
> encoder; No, goto #2
> 2. Query the capability of Platform encoder, if h.264 is supported in this
> platform encoder, choose this one. Otherwise, return an error code indicate
> that format not support.

I think we can do that with separate classes with different class names. I think it can be confusing to have a single class definition with different method implementations on different platforms. We don't really do that in Gecko.

Comment 14

[Shelly Lin [:shelly]]

(In reply to C.J. Ku[:CJKu] from comment #10)
> Transfer to John since Shelly is working on other topics.
> 
> Some ideas
> 1. Plug-in architecture: define plug-in interface to that 3rd party or
> partner can distribute their own track codec on the device easily.
> 2. Rename the base class name from OmxEncoder to PlatformEncoder. This class
> is literally an wrapper of Platform codec(StageFright/ GStreamer/ WMF).

There's already a base class called TrackEncoder, plus I didn't think the 
original draft has defined such class. I would prefer using OmxAACTrackEncoder/OmxH264TrackEncoder, or OmxAudioTrackEncoder/OmxVideoTrackEncoder.

Comment 15

[u459114]

(In reply to Shelly Lin [:shelly] from comment #14)
> (In reply to C.J. Ku[:CJKu] from comment #10)
> > Transfer to John since Shelly is working on other topics.
> > 
> > Some ideas
> > 1. Plug-in architecture: define plug-in interface to that 3rd party or
> > partner can distribute their own track codec on the device easily.
> > 2. Rename the base class name from OmxEncoder to PlatformEncoder. This class
> > is literally an wrapper of Platform codec(StageFright/ GStreamer/ WMF).
> 
> There's already a base class called TrackEncoder, plus I didn't think the 
> original draft has defined such class. I would prefer using
> OmxAACTrackEncoder/OmxH264TrackEncoder, or
> OmxAudioTrackEncoder/OmxVideoTrackEncoder.
OmxTrackEncoder aggregate a StageFright::OmxCodec. Base on meta data that OmxTrackEncoder input into OmxCodec, OmxCodec will pick up correct IOMX encoder component. 
The only difference between OmxH264TrackEncoder and OmxH265TrackEncoder, for instance, is how they setup coding META data. MediaEncoder::CreateEncoder(Encoder generator) is totally transparent with this.

Sub-class OmxTrackEncoder, define a new class for each platform codec is ok too. The difference is you move platform OMX encoder chooser from OmxTrackEncoder itself to MediaEncoder::CreateEncoder(Encoder generator). Once you have a new coding format support, you need to implement a new OmxTrackEncoder sub-class, and you need to modify MediaEncoder::CreateEncoder(Encoder generator). The benefit is, by this approach, each sub-class owns META data setup code for the associate codec, code looks clear.

John, any suggestion?

Comment 16

[u459114]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> > > > 2. Rename the base class name from OmxEncoder to PlatformEncoder. This class
> > > > is literally an wrapper of Platform codec(StageFright/ GStreamer/ WMF).
> > > 
> > > Won't we have different classes for different platforms? Will the different
> > > platform encoders share much code?
> > My opinion is to have one single class definition, different member function
> > definition, for all platforms. As a result, we only need to know
> > PlatformEncoder while constructing encoding pipeline. For example, we want
> > to use h.264 as video encoding fomat.
> > 1. Is there any native encoder support this format? Yes, choose this native
> > encoder; No, goto #2
> > 2. Query the capability of Platform encoder, if h.264 is supported in this
> > platform encoder, choose this one. Otherwise, return an error code indicate
> > that format not support.
> 
> I think we can do that with separate classes with different class names. I
> think it can be confusing to have a single class definition with different
> method implementations on different platforms. We don't really do that in
> Gecko.
Understand. By look into modules in Gecko, yes, you are right, we don't really do that.

Comment 17

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

(In reply to C.J. Ku[:CJKu] from comment #15)
> (In reply to Shelly Lin [:shelly] from comment #14)
> > (In reply to C.J. Ku[:CJKu] from comment #10)
> > > Transfer to John since Shelly is working on other topics.
> > > 
> > > Some ideas
> > > 1. Plug-in architecture: define plug-in interface to that 3rd party or
> > > partner can distribute their own track codec on the device easily.
> > > 2. Rename the base class name from OmxEncoder to PlatformEncoder. This class
> > > is literally an wrapper of Platform codec(StageFright/ GStreamer/ WMF).
> > 
> > There's already a base class called TrackEncoder, plus I didn't think the 
> > original draft has defined such class. I would prefer using
> > OmxAACTrackEncoder/OmxH264TrackEncoder, or
> > OmxAudioTrackEncoder/OmxVideoTrackEncoder.
> OmxTrackEncoder aggregate a StageFright::OmxCodec. Base on meta data that
> OmxTrackEncoder input into OmxCodec, OmxCodec will pick up correct IOMX
> encoder component. 
> The only difference between OmxH264TrackEncoder and OmxH265TrackEncoder, for
> instance, is how they setup coding META data.
> MediaEncoder::CreateEncoder(Encoder generator) is totally transparent with
> this.
> 
> Sub-class OmxTrackEncoder, define a new class for each platform codec is ok
> too. The difference is you move platform OMX encoder chooser from
> OmxTrackEncoder itself to MediaEncoder::CreateEncoder(Encoder generator).
> Once you have a new coding format support, you need to implement a new
> OmxTrackEncoder sub-class, and you need to modify
> MediaEncoder::CreateEncoder(Encoder generator). The benefit is, by this
> approach, each sub-class owns META data setup code for the associate codec,
> code looks clear.
> 
> John, any suggestion?

IMHO OMXCodec already provides a layer of abstraction for different codec format support and subclassing OmxTrackEncoder might not be necessary at all. Even if we'd like to use stagefright for formats other than H.264/AAC in the future, a MediaEncoder::Create(CodecConfig) factory method should still be sufficient.
One more thing, stagefright is also available on Android and we should consider share this implementation with Fennec, right?

Comment 18

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

(In reply to John Lin[:jolin][:jhlin] from comment #17)
> One more thing, stagefright is also available on Android and we should
> consider share this implementation with Fennec, right?

Yes, although I expect that will be extremely painful!

Comment 19

[Shelly Lin [:shelly]]

Attachment: [WIP] Part1: Add OMXCodecWrapper

A wrapper for MediaCodec, hopefully this module can be used by WebRTC as well.

Comment 20

[Shelly Lin [:shelly]]

Attachment: [WIP] Part2: Add OmxTrackEncoder

Again, hopefully there will be only small tweaks from this to make OmxAudioTrackEncoder.

Comment 21

[Shelly Lin [:shelly]]

Attachment: [WIP] Part1: Add OMXCodecWrapper

TODO:
Rewrite EncodeVideoFrame to PushInputBuffer and PopOutputBuffer.

Comment 22

[Shelly Lin [:shelly]]

Attachment: [WIP] Part2: Add OmxTrackEncoder

TODO:
Deal with EOS correctly.

Comment 23

[Shelly Lin [:shelly]]

Attachment: [WIP] Part3: Add OmxAudioTrackEncoder

Comment 24

[Shelly Lin [:shelly]]

Attachment: [WIP] Part1: Add OMXCodecWrapper

Comment 25

[Shelly Lin [:shelly]]

Attachment: [WIP] Part2: Add OmxTrackEncoder

Comment 26

[Shelly Lin [:shelly]]

Attachment: [WIP] Part1: Add OMXCodecWrapper

Comment 27

[Shelly Lin [:shelly]]

Attachment: [WIP] Part2: Add OmxTrackEncoder

Comment 28

[Shelly Lin [:shelly]]

Attachment: [WIP] Part1: Add OMXCodecWrapper

With implementation of appending black frames if chunks are null.

Comment 29

[Shelly Lin [:shelly]]

Attachment: [WIP] Part2: Add OmxTrackEncoder

Comment 30

[Shelly Lin [:shelly]]

Attachment: [changeset] OMXCodecWrapper: appending muted frames

If you need a change set from the last attached patch, here's a patch of changes in OMXCodecWrapper for "appending muted frames".

Comment 31

[Shelly Lin [:shelly]]

Attachment: [changeset] OmxTrackEncoder: appending muted frames

If you need a change set from the last attached patch, here's a patch of changes in OmxTrackEncoder for "appending muted frames".

Comment 32

[Shelly Lin [:shelly]]

Attachment: [WIP] Part2: Add OmxTrackEncoder

Wait if mRawSegment is empty and not EOS yet in |GetEncodedTrack| to prevent busy looping.

Comment 33

[Shelly Lin [:shelly]]

Attachment: [changeset] OmxTrackEncoder: performing a wait

Just in case if anyone needs only the change set.

Comment 34

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

Attachment: Part 1: Add OMXCodecWrapper

AVC/H.264 & AAC encoder class using MediaCodec API in libstagefright.

Comment 35

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

Comment on attachment 8335590 [details] [diff] [review]
Part 1: Add OMXCodecWrapper

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

::: content/media/omx/OMXCodecWrapper.h
@@ +16,5 @@
> +
> +namespace android {
> +
> +/**
> + * AVC/H.264 and AAC encoder using MediaCodec API in libstagefright.

Add to this comment to explain what state this class represents. I assume it's one instance of a libstagefright MediaCodec which can encode one track, either audio or video?

@@ +25,5 @@
> +  // Flags for constructor.
> +  enum {
> +    TYPE_VIDEO = 1 << 0,
> +    TYPE_AUDIO = 1 << 1,
> +  };

Can you use MediaSegment::Type instead please?

@@ +40,5 @@
> +  // Hard-coded values for AAC DecoderConfigDescriptor in libstagefright.
> +  // See MPEG4Writer::Track::writeMp4aEsdsBox()
> +  // Exposed for the need of MP4 container writer.
> +  const static uint32_t kAACBitrate = 96000; // kbps
> +  const static uint32_t kAACFrameSize = 768; // bytes

It's more portable to write
  enum {
    kAACBitrate = 96000,
    kAACFrameSize = 768
  };

@@ +42,5 @@
> +  // Exposed for the need of MP4 container writer.
> +  const static uint32_t kAACBitrate = 96000; // kbps
> +  const static uint32_t kAACFrameSize = 768; // bytes
> +
> +  OMXCodecWrapper(const int aFlags);

Document these flags. Is it just one of TYPE_VIDEO and TYPE_AUDIO?

@@ +44,5 @@
> +  const static uint32_t kAACFrameSize = 768; // bytes
> +
> +  OMXCodecWrapper(const int aFlags);
> +
> +  virtual ~OMXCodecWrapper();

Why this this virtual? Are there going to be subclasses of OMXCodecWrapper?

If not, declare it MOZ_FINAL.

@@ +46,5 @@
> +  OMXCodecWrapper(const int aFlags);
> +
> +  virtual ~OMXCodecWrapper();
> +
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(OMXCodecWrapper)

Does it need to be threadsafe? If so, please document somewhere the threading requirements, e.g. which thread it should be created on, and which methods you can call on which threads.

@@ +78,5 @@
> +  /**
> +   * Encode a video frame with type of nsTArray<uint8_t>.
> +   */
> +  void EncodeVideoFrame(const nsTArray<uint8_t>& aImage,
> +                        int64_t aDuration, int aInputFlags = 0);

Specify what format aImage is in.

Also specify what aInputFlags can be.

Also specify what units aDuration is in.

Comment 36

[Shelly Lin [:shelly]]

Attachment: Part2: Add OmxVideoTrackEncoder to file OmxTrackEncoder

Hi Roc, this is the part of OmxVideoTrackEncoder, please give it a review at your convenience :).

Comment 37

[Shelly Lin [:shelly]]

Attachment: Part2: Add OmxVideoTrackEncoder to file OmxTrackEncoder

Comment 38

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

Attachment: Part1: Add OMXCodecWrapper.

* Addresses previous r- comments.
* Timestamp in input/output parameters.

Comment 39

[u459114]

Comment on attachment 8338232 [details] [diff] [review]
Part1: Add OMXCodecWrapper.

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

::: content/media/omx/OMXCodecWrapper.cpp
@@ +34,5 @@
> +  if (!sThreadPoolStarted) {
> +    ProcessState::self()->startThreadPool();
> +    sThreadPoolStarted = true;
> +  }
> +

Thread safe is not guarantee here.

@@ +42,5 @@
> +  if (mMediaType == Type::VIDEO) {
> +    mCodec = MediaCodec::CreateByType(mLooper, MEDIA_MIMETYPE_VIDEO_AVC, true);
> +  } else if (mMediaType == Type::AUDIO) {
> +    mCodec = MediaCodec::CreateByType(mLooper, MEDIA_MIMETYPE_AUDIO_AAC, true);
> +  }

Unless you can 100% make sure MediaCodec::CreateByType is always a successful function call, I think we should move these two statements(if/ else if) into a lazy initiate function?
private:
bool Init() {
 if (mCodec != pullptr)
   return true;

 if (mMediaType == Type::VIDEO) {
   mCodec = MediaCodec::CreateByType(mLooper, MEDIA_MIMETYPE_VIDEO_AVC, true);
 } else if (mMediaType == Type::AUDIO) {
   mCodec = MediaCodec::CreateByType(mLooper, MEDIA_MIMETYPE_AUDIO_AAC, true);
 }
  
 return mCodec.get() != nullptr;
}

Call Init in the beginning of Start/ Config functions.

@@ +136,5 @@
> +
> +void
> +OMXCodecWrapper::EncodeVideoFrame(const GrallocImage& aImage, int64_t aTimestamp,
> +                                  int aInputFlags)
> +{

NS_ENSURE_TRUE(mCodec != nullptr && mMediaType == Type::VIDEO, NS_ERROR_FAILURE);

@@ +137,5 @@
> +void
> +OMXCodecWrapper::EncodeVideoFrame(const GrallocImage& aImage, int64_t aTimestamp,
> +                                  int aInputFlags)
> +{
> +  GrallocImage* nativeImage = const_cast<GrallocImage*>(&aImage);

GrallocImage  &nativeImage = const_cast<GrallocImage&>(aImage);
const_cast work for both pointer and reference

@@ +145,5 @@
> +
> +  // Size of PLANAR_YCBCR 4:2:0.
> +  uint32_t frameLen = graphicBuffer->getWidth()*graphicBuffer->getHeight()*3/2;
> +  //
> +  void *imgPtr;

void *imgPtr = nullptr;

@@ +155,5 @@
> +
> +void
> +OMXCodecWrapper::EncodeVideoFrame(const nsTArray<uint8_t>& aImage,
> +                                  int64_t aTimestamp, int aInputFlags)
> +{

NS_ENSURE_TRUE(mCodec != nullptr && mMediaType == Type::VIDEO, NS_ERROR_FAILURE);

@@ +164,5 @@
> +void
> +OMXCodecWrapper::EncodeAudioSamples(const nsTArray<AudioDataValue>& aSamples,
> +                                    int64_t aTimestamp, int64_t aDuration,
> +                                    int aInputFlags)
> +{

NS_ENSURE_TRUE(mCodec != nullptr && mMediaType == Type::Audio, NS_ERROR_FAILURE);

@@ +234,5 @@
> +  size_t index;
> +  size_t outOffset;
> +  size_t outSize;
> +  int64_t outTimeUs;
> +  uint32_t outFlags;

default value assignment

@@ +248,5 @@
> +        err = mCodec->getOutputBuffers(&mOutputBufs);
> +        // Get output from a new buffer.
> +        retry = true;
> +        break;
> +      case INFO_FORMAT_CHANGED:

break;

@@ +250,5 @@
> +        retry = true;
> +        break;
> +      case INFO_FORMAT_CHANGED:
> +      default:
> +        return;

ASSERT(false) here?

::: content/media/omx/OMXCodecWrapper.h
@@ +47,5 @@
> +  enum {
> +    kAACBitrate = 96000, // kbps
> +    kAACFrameSize = 768, // bytes
> +  };
> +

Is here a correct place to expose this enum to MP4 container?

@@ +53,5 @@
> +   * Create a media codec. It will be AVC/H.264 encoder if aMediaType is
> +   * MediaSegment::Type::VIDEO, or AAC encoder if aMediaType is
> +   * MediaSegment::Type::AUDIO
> +   */
> +  OMXCodecWrapper(const Type aMediaType);

Declare default/ copy constructor as private

@@ +64,5 @@
> +   * Starts the media codec. It must be called before calling
> +   * EncodeVideoFrame(), EncodeAudioSamples(), and GetNextEncodedFrame().
> +   */
> +  nsresult Start();
> +

According to your comment, please assert mStarted is true in the beginning of EncodeVideoFrame(), EncodeAudioSamples(), and GetNextEncodedFrame().

Comment 40

[u459114]

Comment on attachment 8337657 [details] [diff] [review]
Part2: Add OmxVideoTrackEncoder to file OmxTrackEncoder

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

::: content/media/encoder/OmxTrackEncoder.cpp
@@ +23,5 @@
> +#define GET_ENCODED_VIDEO_FRAME_TIMEOUT 100000 // microseconds
> +
> +nsresult
> +OmxVideoTrackEncoder::Init(int aWidth, int aHeight, TrackRate aTrackRate)
> +{

MOZ_ASSERT(mEncoder == nullptr) to make sure no double init

@@ +98,5 @@
> +    mEncoder->EncodeVideoFrame(imgBuf, totalDurationUs,
> +                               OMXCodecWrapper::BUFFER_EOS);
> +  }
> +
> +  while (!iter.IsEnded()) {

Comment what this loop work for

@@ +126,5 @@
> +  int outFlags = 0;
> +  int64_t outTimeStampUs = 0;
> +  mEncoder->GetNextEncodedFrame(&buffer, &outTimeStampUs, &outFlags,
> +                                GET_ENCODED_VIDEO_FRAME_TIMEOUT);
> +  if (!buffer.IsEmpty()) {

Comment for this if statement

::: content/media/encoder/OmxTrackEncoder.h
@@ +25,5 @@
> +{
> +public:
> +  OmxVideoTrackEncoder()
> +    : VideoTrackEncoder()
> +  {};

remove ;

Comment 41

[Jason Smith [:jsmith]]

Per discussion offline - this is being moved out of scope for 1.3. We will target this feature for 1.4.

However, we don't block on targeted features, so this is no longer a blocker.

Comment 42

[u459114]

Comment on attachment 8338232 [details] [diff] [review]
Part1: Add OMXCodecWrapper.

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

::: content/media/omx/OMXCodecWrapper.cpp
@@ +48,5 @@
> +
> +OMXCodecWrapper::~OMXCodecWrapper()
> +{
> +  Stop();
> +  mCodec->release();

I think we should release mCodec in Stop function.

::: content/media/omx/OMXCodecWrapper.h
@@ +113,5 @@
> +   * aOutputTimestamp.
> +   * Wait at most aTimeout microseconds to dequeue a outpout buffer.
> +   */
> +  void GetNextEncodedFrame(nsTArray<uint8_t>* aOutputBuf,
> +                           int64_t* aOutputTimestamp, int* aOutputFlags,

Since you had used reference parameter in EncodedXXXX function, is there any special reason that not using reference here?
nsTArray<uint8_t>& aOutputBuf, int64_t& aOutputTimestamp, int& aOutputFlags

Comment 43

[u459114]

Comment on attachment 8338232 [details] [diff] [review]
Part1: Add OMXCodecWrapper.

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

::: content/media/omx/OMXCodecWrapper.cpp
@@ +200,5 @@
> +    const sp<ABuffer>& inBuf = mInputBufs.itemAt(index);
> +    size_t bufferSize = inBuf->capacity();
> +
> +    size_t remain = aSize - copied;
> +    size_t toCopy = remain > bufferSize? bufferSize:remain;

Add space
size_t toCopy = (remain > bufferSize) ? bufferSize : remain;

Comment 44

[u459114]

Create android::MediaCodec in OMXCodecWrapper::Start and destroy android::MediaCodec OMXCodecWrapper::Stop.
So that we acquire HW codec _after_ MediaRecoder::Start and release that resource immidiately after MediaRecoder::Stop be called.

Comment 45

[Ivan Tsay (:ITsay)]

reset target milestone since this one is moved to v1.4

Comment 46

[u459114]

Comment on attachment 8338232 [details] [diff] [review]
Part1: Add OMXCodecWrapper.

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

::: content/media/omx/OMXCodecWrapper.cpp
@@ +187,5 @@
> +  do {
> +    // Dequeue an input buffer.
> +    uint32_t index;
> +    while (true) {
> +      err = mCodec->dequeueInputBuffer(&index, aTimeOut);

if the rate of filling input buffer > the rate of consuming output buffer, Session::Extract thread will spin in this while loop forever,

Comment 47

[Shelly Lin [:shelly]]

Attachment: Function interface change in OMXCodecWrapper

Hey John, could you make the following change for |EncodeVideoFrame()|?

Comment 48

[u459114]

Comment on attachment 8337657 [details] [diff] [review]
Part2: Add OmxVideoTrackEncoder to file OmxTrackEncoder

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

::: content/media/encoder/OmxTrackEncoder.cpp
@@ +84,5 @@
> +    }
> +
> +    segment.AppendFrom(&mRawSegment);
> +  }
> +

From Line 70 to Line 88 should be the same for all VideoTrack encoder, can we move this code segment into base class?

Comment 49

[u459114]

Comment on attachment 8338232 [details] [diff] [review]
Part1: Add OMXCodecWrapper.

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

::: content/media/omx/OMXCodecWrapper.h
@@ +21,5 @@
> + * This class wraps video and audio encoders provided by MediaCodec API in
> + * libstagefright. Each object represents either a AVC/H.264 or AAC encoder that
> + * can encode one track.
> + */
> +class OMXCodecWrapper MOZ_FINAL

A suggestion
1. Remove mMediaType in OMXCodecWrapper
2. Create two concrete inherited classes OMXAudioCodecWrapper and OMXVideoCodecWrapper.
3. Move ConfigureVideo and EncodeVideoFrame into OMXVideoCodecWrapper.
4. Move ConfigureAudio and EncodeAudioSample into OMXAudioCodecWrapper.
5. OmxAudioTrackEncoder uses OMXAudioCodecWrapper; OmxVideoTrackEncoder uses OMXVideoCodecWrapper

Then, you decouple API dependency by inheritance
EncodeVideoFrame depends on ConfigureVideo -- if you call ConfigureAudio and then EncodeVideoFrame-- dead...

Comment 50

[u459114]

I had finished my code review for this module. Code quality is good :)

In regards to performance,
1. We need to copy source gralloc buffer 
2. We need to create and copy output encoded buffers from MediaCodec
Those are performance work that we should follow up after we make video encoded path work.

In regards to stability, please check comment 46.

Comment 51

[Shelly Lin [:shelly]]

Attachment: Part2: Add OmxVideoTrackEncoder to file OmxTrackEncoder

Add #define MOZ_OMX_ENCODER to key off building on different platforms.

Comment 52

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

Comment on attachment 8339823 [details] [diff] [review]
Part2: Add OmxVideoTrackEncoder to file OmxTrackEncoder

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

::: content/media/encoder/OmxTrackEncoder.cpp
@@ +91,5 @@
> +  // Send the EOS signal and an empty frame to OMXCodecWrapper.
> +  if (mEndOfStream && iter.IsEnded() && !mEosSetInEncoder) {
> +    mEosSetInEncoder = true;
> +    nsTArray<uint8_t> imgBuf;
> +    CreateMutedFrame(&imgBuf);

I think we should send BUFFER_EOS without having to encode an extra black frame. Can we do this by moving this block of code to after we've encode thed frames we have, and setting BUFFER_EOS on the last frame if we have one?

@@ +113,5 @@
> +      }
> +    }
> +
> +    mLastFrame.TakeFrom(&chunk.mFrame);
> +    mLastFrame.SetForceBlack(chunk.mFrame.GetForceBlack());

Doesn't TakeFrom copy the force-black state already? If not, it should!

::: content/media/encoder/moz.build
@@ +22,5 @@
>      UNIFIED_SOURCES += ['OpusTrackEncoder.cpp']
>  
> +if CONFIG['MOZ_OMX_ENCODER']:
> +    EXPORTS += ['OmxTrackEncoder.h']
> +    SOURCES += ['OmxTrackEncoder.cpp']

UNIFIED_SOURCES

Comment 53

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

Attachment: bug879668-p1-add-omx-wrapper.patch

Re-structure classes and functions to address CJay's review comments, mainly
- Add video & audio encoder subclasses to eliminate the problem of incorrect usage.
- Align the lifecycle of underlying codec and wrapper object.

Comment 54

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

(In reply to C.J. Ku[:CJKu] from comment #42)
> Comment on attachment 8338232 [details] [diff] [review]
> Part1: Add OMXCodecWrapper.
> 
> Review of attachment 8338232 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/omx/OMXCodecWrapper.cpp
> @@ +48,5 @@
> > +
> > +OMXCodecWrapper::~OMXCodecWrapper()
> > +{
> > +  Stop();
> > +  mCodec->release();
> 
> I think we should release mCodec in Stop function.
 I'll bind the lifecycle of mCodec to construction/destruction of wrapper objects and make start/stop internal implementation that user don't need to care about. This seems simpler to use.
 
> 
> ::: content/media/omx/OMXCodecWrapper.h
> @@ +113,5 @@
> > +   * aOutputTimestamp.
> > +   * Wait at most aTimeout microseconds to dequeue a outpout buffer.
> > +   */
> > +  void GetNextEncodedFrame(nsTArray<uint8_t>* aOutputBuf,
> > +                           int64_t* aOutputTimestamp, int* aOutputFlags,
> 
> Since you had used reference parameter in EncodedXXXX function, is there any
> special reason that not using reference here?
> nsTArray<uint8_t>& aOutputBuf, int64_t& aOutputTimestamp, int& aOutputFlags
 Just in case that user would like to ignore some output parameters by giving null pointers as arguments.

Comment 55

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

(In reply to C.J. Ku[:CJKu] from comment #39)
> Comment on attachment 8338232 [details] [diff] [review]
> Part1: Add OMXCodecWrapper.
> 
> Review of attachment 8338232 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/omx/OMXCodecWrapper.cpp
> @@ +34,5 @@
> > +  if (!sThreadPoolStarted) {
> > +    ProcessState::self()->startThreadPool();
> > +    sThreadPoolStarted = true;
> > +  }
> > +
> 
> Thread safe is not guarantee here.
  Will remove sThreadPoolStarted because it turns out Android code already has a mutex for thread safety and a flag to avoid starting more than once.

> 
> @@ +42,5 @@
> > +  if (mMediaType == Type::VIDEO) {
> > +    mCodec = MediaCodec::CreateByType(mLooper, MEDIA_MIMETYPE_VIDEO_AVC, true);
> > +  } else if (mMediaType == Type::AUDIO) {
> > +    mCodec = MediaCodec::CreateByType(mLooper, MEDIA_MIMETYPE_AUDIO_AAC, true);
> > +  }
> 
> Unless you can 100% make sure MediaCodec::CreateByType is always a
> successful function call, I think we should move these two statements(if/
> else if) into a lazy initiate function?
> private:
> bool Init() {
>  if (mCodec != pullptr)
>    return true;
> 
>  if (mMediaType == Type::VIDEO) {
>    mCodec = MediaCodec::CreateByType(mLooper, MEDIA_MIMETYPE_VIDEO_AVC,
> true);
>  } else if (mMediaType == Type::AUDIO) {
>    mCodec = MediaCodec::CreateByType(mLooper, MEDIA_MIMETYPE_AUDIO_AAC,
> true);
>  }
>   
>  return mCodec.get() != nullptr;
> }
> 
> Call Init in the beginning of Start/ Config functions.
 Will align the lifecycle of mCodec and wrapper object, and add static creator functions to make sure user won't get a wrapper object with invalid mCodec.

> 
> @@ +136,5 @@
> > +
> > +void
> > +OMXCodecWrapper::EncodeVideoFrame(const GrallocImage& aImage, int64_t aTimestamp,
> > +                                  int aInputFlags)
> > +{
> 
> NS_ENSURE_TRUE(mCodec != nullptr && mMediaType == Type::VIDEO,
> NS_ERROR_FAILURE);
 These sanity check won't be necessary after adding creator function and individual subclasses for audio & video encoding.

> 
> @@ +137,5 @@
> > +void
> > +OMXCodecWrapper::EncodeVideoFrame(const GrallocImage& aImage, int64_t aTimestamp,
> > +                                  int aInputFlags)
> > +{
> > +  GrallocImage* nativeImage = const_cast<GrallocImage*>(&aImage);
> 
> GrallocImage  &nativeImage = const_cast<GrallocImage&>(aImage);
> const_cast work for both pointer and reference
 Will do.

> 
> @@ +145,5 @@
> > +
> > +  // Size of PLANAR_YCBCR 4:2:0.
> > +  uint32_t frameLen = graphicBuffer->getWidth()*graphicBuffer->getHeight()*3/2;
> > +  //
> > +  void *imgPtr;
> 
> void *imgPtr = nullptr;
 Will do.

> 
> @@ +155,5 @@
> > +
> > +void
> > +OMXCodecWrapper::EncodeVideoFrame(const nsTArray<uint8_t>& aImage,
> > +                                  int64_t aTimestamp, int aInputFlags)
> > +{
> 
> NS_ENSURE_TRUE(mCodec != nullptr && mMediaType == Type::VIDEO,
> NS_ERROR_FAILURE);
  Won't be necessary.

> 
> @@ +164,5 @@
> > +void
> > +OMXCodecWrapper::EncodeAudioSamples(const nsTArray<AudioDataValue>& aSamples,
> > +                                    int64_t aTimestamp, int64_t aDuration,
> > +                                    int aInputFlags)
> > +{
> 
> NS_ENSURE_TRUE(mCodec != nullptr && mMediaType == Type::Audio,
> NS_ERROR_FAILURE);
 Same as above.

> 
> @@ +234,5 @@
> > +  size_t index;
> > +  size_t outOffset;
> > +  size_t outSize;
> > +  int64_t outTimeUs;
> > +  uint32_t outFlags;
> 
> default value assignment
 Didn't think default values are necessary because these variables are not used when failure. Will do to please you anyway.

> 
> @@ +248,5 @@
> > +        err = mCodec->getOutputBuffers(&mOutputBufs);
> > +        // Get output from a new buffer.
> > +        retry = true;
> > +        break;
> > +      case INFO_FORMAT_CHANGED:
> 
> break;
 Will do. (It was meant to be falling through here. Should've added comment to make it clear. :)

> 
> @@ +250,5 @@
> > +        retry = true;
> > +        break;
> > +      case INFO_FORMAT_CHANGED:
> > +      default:
> > +        return;
> 
> ASSERT(false) here?
 Will do.

> 
> ::: content/media/omx/OMXCodecWrapper.h
> @@ +47,5 @@
> > +  enum {
> > +    kAACBitrate = 96000, // kbps
> > +    kAACFrameSize = 768, // bytes
> > +  };
> > +
> 
> Is here a correct place to expose this enum to MP4 container?
 I put them here because they're defined in stagefright code and seems part of wrapper. Any suggestion?

> 
> @@ +53,5 @@
> > +   * Create a media codec. It will be AVC/H.264 encoder if aMediaType is
> > +   * MediaSegment::Type::VIDEO, or AAC encoder if aMediaType is
> > +   * MediaSegment::Type::AUDIO
> > +   */
> > +  OMXCodecWrapper(const Type aMediaType);
> 
> Declare default/ copy constructor as private
 Will do.

> 
> @@ +64,5 @@
> > +   * Starts the media codec. It must be called before calling
> > +   * EncodeVideoFrame(), EncodeAudioSamples(), and GetNextEncodedFrame().
> > +   */
> > +  nsresult Start();
> > +
> 
> According to your comment, please assert mStarted is true in the beginning
> of EncodeVideoFrame(), EncodeAudioSamples(), and GetNextEncodedFrame().
 Will do.

Comment 56

[Shelly Lin [:shelly]]

Comment on attachment 8339823 [details] [diff] [review]
Part2: Add OmxVideoTrackEncoder to file OmxTrackEncoder

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

::: content/media/encoder/OmxTrackEncoder.cpp
@@ +113,5 @@
> +      }
> +    }
> +
> +    mLastFrame.TakeFrom(&chunk.mFrame);
> +    mLastFrame.SetForceBlack(chunk.mFrame.GetForceBlack());

Hmm, I don't think it does, see
http://hg.mozilla.org/mozilla-central/file/84a5a5800bd3/content/media/VideoSegment.cpp#l31

I'll fix it in |TakeFrom()|.

Comment 57

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #35)
> Comment on attachment 8335590 [details] [diff] [review]
> Part 1: Add OMXCodecWrapper
> 
> Review of attachment 8335590 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/omx/OMXCodecWrapper.h
> @@ +16,5 @@
> > +
> > +namespace android {
> > +
> > +/**
> > + * AVC/H.264 and AAC encoder using MediaCodec API in libstagefright.
> 
> Add to this comment to explain what state this class represents. I assume
> it's one instance of a libstagefright MediaCodec which can encode one track,
> either audio or video?
> 
> @@ +25,5 @@
> > +  // Flags for constructor.
> > +  enum {
> > +    TYPE_VIDEO = 1 << 0,
> > +    TYPE_AUDIO = 1 << 1,
> > +  };
> 
> Can you use MediaSegment::Type instead please?
> 
> @@ +40,5 @@
> > +  // Hard-coded values for AAC DecoderConfigDescriptor in libstagefright.
> > +  // See MPEG4Writer::Track::writeMp4aEsdsBox()
> > +  // Exposed for the need of MP4 container writer.
> > +  const static uint32_t kAACBitrate = 96000; // kbps
> > +  const static uint32_t kAACFrameSize = 768; // bytes
> 
> It's more portable to write
>   enum {
>     kAACBitrate = 96000,
>     kAACFrameSize = 768
>   };
> 
> @@ +42,5 @@
> > +  // Exposed for the need of MP4 container writer.
> > +  const static uint32_t kAACBitrate = 96000; // kbps
> > +  const static uint32_t kAACFrameSize = 768; // bytes
> > +
> > +  OMXCodecWrapper(const int aFlags);
> 
> Document these flags. Is it just one of TYPE_VIDEO and TYPE_AUDIO?
> 
> @@ +44,5 @@
> > +  const static uint32_t kAACFrameSize = 768; // bytes
> > +
> > +  OMXCodecWrapper(const int aFlags);
> > +
> > +  virtual ~OMXCodecWrapper();
> 
> Why this this virtual? Are there going to be subclasses of OMXCodecWrapper?
> 
> If not, declare it MOZ_FINAL.
> 
> @@ +46,5 @@
> > +  OMXCodecWrapper(const int aFlags);
> > +
> > +  virtual ~OMXCodecWrapper();
> > +
> > +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(OMXCodecWrapper)
> 
> Does it need to be threadsafe? If so, please document somewhere the
> threading requirements, e.g. which thread it should be created on, and which
> methods you can call on which threads.
> 
> @@ +78,5 @@
> > +  /**
> > +   * Encode a video frame with type of nsTArray<uint8_t>.
> > +   */
> > +  void EncodeVideoFrame(const nsTArray<uint8_t>& aImage,
> > +                        int64_t aDuration, int aInputFlags = 0);
> 
> Specify what format aImage is in.
> 
> Also specify what aInputFlags can be.
> 
> Also specify what units aDuration is in.

Thanks to your and CJay's comments, there has been lot of changes to OMXCodecWrapper since last r-. Please take a look at the (almost all) new version. :)

Comment 58

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

Attachment: Part 1: Add OMXCodecWrapper

Fix debug build break issue in attachment 8340904 [details] [diff] [review].

Comment 59

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

Comment on attachment 8340941 [details] [diff] [review]
Part 1: Add OMXCodecWrapper

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

::: content/media/omx/OMXCodecWrapper.cpp
@@ +1,5 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-*/
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +#include "OMXCodecWrapper.h"

Blank line after license comment.

@@ +29,5 @@
> +    } else { \
> +      delete codec; \
> +      return nullptr; \
> +    } \
> +  } while (0)

I don't think using this macro is worthwhile. I suggest writing each function body as
  nsAutoPtr<AudioEncoder> aac = new AudioEncoder(CodecType::AAC_ENC);
  if (!codec->mCodec.get()) {
    return nullptr;
  }
  return aac.forget();

@@ +66,5 @@
> +  if (mCodecType == CodecType::AVC_ENC) {
> +    mCodec = MediaCodec::CreateByType(mLooper, MEDIA_MIMETYPE_VIDEO_AVC, true);
> +  } else if (mCodecType == CodecType::AAC_ENC) {
> +    mCodec = MediaCodec::CreateByType(mLooper, MEDIA_MIMETYPE_AUDIO_AAC, true);
> +  }

Better add 
  } else {
    NS_ERROR("Unknown codec type");
  }

@@ +73,5 @@
> +OMXCodecWrapper::~OMXCodecWrapper()
> +{
> +  if (mCodec.get()) {
> +    Stop();
> +    mCodec->release();

Why do you need to call release() on mCodec? It's about to run its destructor which should call release()?

::: content/media/omx/OMXCodecWrapper.h
@@ +17,5 @@
> +
> +#define DEF_CTOR_AND_COPIERS(T) \
> +    T() {} \
> +    T(const T&) {} \
> +    T& operator=(const T&) { return *this; }

I don't think we should use a macro for this.

I don't think you should define this assignment operator. It's bad to just have an assignment operator that does nothing. Similar for the copy constructor. If you don't want these to be defined, use MOZ_DELETE.

@@ +158,5 @@
> +
> +/**
> + * Audio encoder.
> + */
> +class AudioEncoder MOZ_FINAL : public OMXCodecWrapper {

Call this OMXAudioEncoder.

@@ +193,5 @@
> +
> +/**
> + * Video encoder.
> + */
> +class VideoEncoder MOZ_FINAL : public OMXCodecWrapper {

Call this OMXVideoEncoder

Comment 60

[Shelly Lin [:shelly]]

Attachment: Part3: Add OmxVideoTrackEncoder to file OmxTrackEncoder

Comment 61

[Shelly Lin [:shelly]]

Comment on attachment 8341007 [details] [diff] [review]
Part3: Add OmxVideoTrackEncoder to file OmxTrackEncoder

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

::: content/media/encoder/OmxTrackEncoder.cpp
@@ +112,5 @@
> +    uint64_t totalDurationUs = mTotalFrameDuration * USECS_PER_S / mTrackRate;
> +    Image* imagePtr = (!mLastFrame.GetImage() || mLastFrame.GetForceBlack()) ?
> +                      nullptr : mLastFrame.GetImage();
> +    mEncoder->EncodeVideoFrame(imagePtr, totalDurationUs,
> +                               OMXCodecWrapper::BUFFER_EOS);

Like this? (I'll have OMXCodecWrapper change its interface, I think this is cleaner though.)

Comment 62

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #59)
> Comment on attachment 8340941 [details] [diff] [review]
> Part 1: Add OMXCodecWrapper
> 
> Review of attachment 8340941 [details] [diff] [review]:
> -----------------------------------------------------------------

 Review comments will be addressed in v3.

> 
> @@ +73,5 @@
> > +OMXCodecWrapper::~OMXCodecWrapper()
> > +{
> > +  if (mCodec.get()) {
> > +    Stop();
> > +    mCodec->release();
> 
> Why do you need to call release() on mCodec? It's about to run its
> destructor which should call release()?
> 
 Unfortunately MediaCodec doesn't do that and its document says user is responsible for calling release() before destruction.

Comment 63

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

Attachment: Part 1: Add OMXCodecWrapper - v3

Addresses last r- in comment #59.

Comment 64

[u459114]

Comment on attachment 8341012 [details] [diff] [review]
Part 1: Add OMXCodecWrapper - v3

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

::: content/media/omx/OMXCodecWrapper.cpp
@@ +62,5 @@
> +  mLooper = new ALooper();
> +  mLooper->start();
> +
> +  if (mCodecType == CodecType::AVC_ENC) {
> +    mCodec = MediaCodec::CreateByType(mLooper, MEDIA_MIMETYPE_VIDEO_AVC, true);

Make mCodec a protected member and move to OMXVideoEncoder's constructor

@@ +64,5 @@
> +
> +  if (mCodecType == CodecType::AVC_ENC) {
> +    mCodec = MediaCodec::CreateByType(mLooper, MEDIA_MIMETYPE_VIDEO_AVC, true);
> +  } else if (mCodecType == CodecType::AAC_ENC) {
> +    mCodec = MediaCodec::CreateByType(mLooper, MEDIA_MIMETYPE_AUDIO_AAC, true);

Make mCodec a protected member and move to OMXAudioEncoder's constructor

::: content/media/omx/OMXCodecWrapper.h
@@ +187,5 @@
> +   */
> +  OMXAudioEncoder(CodecType aCodecType): OMXCodecWrapper(aCodecType) {}
> +
> +  // For creator function to access hidden constructor.
> +  friend class OMXCodecWrapper;

Declare constructor of OMXCodecWrapper in protected section and remove friend class here.

@@ +199,5 @@
> +  /**
> +   * Configure video codec parameters and start media codec. It must be called
> +   * before calling EncodeVideoFrame() and GetNextEncodedFrame().
> +   */
> +  nsresult ConfigureVideo(int aWidth, int aHeight, int aFrameRate);

Since you already subclass OMXCodecWrapper, there is no need Video postfix here
OMXVideoEncoder::Config

@@ +208,5 @@
> +   * microseconds). To notify end of stream, set aInputFlags to BUFFER_EOS.
> +   */
> +  void EncodeVideoFrame(const mozilla::layers::Image& aImage,
> +                        int64_t aTimestamp, int aInputFlags = 0);
> +

Encode

@@ +215,5 @@
> +   * array. aTimestamp gives the frame timestamp/presentation time (in
> +   * microseconds). To notify end of stream, set aInputFlags to BUFFER_EOS.
> +   */
> +  void EncodeVideoFrame(const nsTArray<uint8_t>& aImage,
> +                        int64_t aTimestamp, int aInputFlags = 0);

Encode

@@ +230,5 @@
> +   */
> +  OMXVideoEncoder(CodecType aCodecType): OMXCodecWrapper(aCodecType) {}
> +
> +  // For creator function to access hidden constructor.
> +  friend class OMXCodecWrapper;

Declare constructor of OMXCodecWrapper in protected section and remove friend class here.

Comment 65

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

Comment on attachment 8341007 [details] [diff] [review]
Part3: Add OmxVideoTrackEncoder to file OmxTrackEncoder

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

::: content/media/encoder/OmxTrackEncoder.cpp
@@ +106,5 @@
> +    iter.Next();
> +  }
> +
> +  // Send the EOS signal and an empty frame to OMXCodecWrapper.
> +  if (mEndOfStream && iter.IsEnded() && !mEosSetInEncoder) {

iter.IsEnded() must be true here. Remove this.

@@ +112,5 @@
> +    uint64_t totalDurationUs = mTotalFrameDuration * USECS_PER_S / mTrackRate;
> +    Image* imagePtr = (!mLastFrame.GetImage() || mLastFrame.GetForceBlack()) ?
> +                      nullptr : mLastFrame.GetImage();
> +    mEncoder->EncodeVideoFrame(imagePtr, totalDurationUs,
> +                               OMXCodecWrapper::BUFFER_EOS);

Yes, I think this is OK. It might not be as efficient as it could be if we passed BUFFER_EOS when we encode the last frame for the first time, but it probably doesn't matter.

Comment 66

[u459114]

Comment on attachment 8341012 [details] [diff] [review]
Part 1: Add OMXCodecWrapper - v3

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

::: content/media/omx/OMXCodecWrapper.cpp
@@ +74,5 @@
> +OMXCodecWrapper::~OMXCodecWrapper()
> +{
> +  if (mCodec.get()) {
> +    Stop();
> +    mCodec->release();

Need to call release manually?

@@ +151,5 @@
> +  return err == OK ? NS_OK : NS_ERROR_FAILURE;
> +}
> +
> +nsresult
> +OMXAudioEncoder::ConfigureAudio(int aChannels, int aSamplingRate)

Change return value to boolean?

@@ +190,5 @@
> +  sp<GraphicBuffer> graphicBuffer = GrallocBufferActor::GetFrom(grallocHandle);
> +
> +  // Size of PLANAR_YCBCR 4:2:0.
> +  uint32_t frameLen = graphicBuffer->getWidth()*graphicBuffer->getHeight()*3/2;
> +  //

Remove //

@@ +215,5 @@
> +                                    int aInputFlags)
> +{
> +// MediaCodec accepts only 16-bit PCM data.
> +#ifndef MOZ_SAMPLE_TYPE_S16
> +  MOZ_ASSERT(false);

You want a compile time error or debug time error?

@@ +225,5 @@
> +}
> +
> +status_t
> +OMXCodecWrapper::PushInput(const void* aData, size_t aSize, int64_t aTimestamp,
> +                           int64_t aDuration, int aInputFlags, int64_t aTimeOut)

Instead of using return value of MediaCodec, you may change return value to boolean and check return value at the place you call this function.
Please do a negative test in caller. If anything wrong here, we must have a way to report error here to MediaCodec, MediaCodec need to report error to MediaRecorder, MediaRecoder need to report error to js client.

@@ +319,5 @@
> +  uint32_t outFlags = 0;
> +  bool retry = false;
> +  do {
> +    status_t err = mCodec->dequeueOutputBuffer(&index, &outOffset, &outSize,
> +                           &outTimeUs, &outFlags, aTimeOut);

it's not err... it's a status code

Comment 67

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

(In reply to C.J. Ku[:CJKu] from comment #64)
> Comment on attachment 8341012 [details] [diff] [review]
> Part 1: Add OMXCodecWrapper - v3
> 
> Review of attachment 8341012 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/omx/OMXCodecWrapper.cpp
> @@ +62,5 @@
> > +  mLooper = new ALooper();
> > +  mLooper->start();
> > +
> > +  if (mCodecType == CodecType::AVC_ENC) {
> > +    mCodec = MediaCodec::CreateByType(mLooper, MEDIA_MIMETYPE_VIDEO_AVC, true);
> 
> Make mCodec a protected member and move to OMXVideoEncoder's constructor
> 
> @@ +64,5 @@
> > +
> > +  if (mCodecType == CodecType::AVC_ENC) {
> > +    mCodec = MediaCodec::CreateByType(mLooper, MEDIA_MIMETYPE_VIDEO_AVC, true);
> > +  } else if (mCodecType == CodecType::AAC_ENC) {
> > +    mCodec = MediaCodec::CreateByType(mLooper, MEDIA_MIMETYPE_AUDIO_AAC, true);
> 
> Make mCodec a protected member and move to OMXAudioEncoder's constructor
> 
> ::: content/media/omx/OMXCodecWrapper.h
> @@ +187,5 @@
> > +   */
> > +  OMXAudioEncoder(CodecType aCodecType): OMXCodecWrapper(aCodecType) {}
> > +
> > +  // For creator function to access hidden constructor.
> > +  friend class OMXCodecWrapper;
> 
> Declare constructor of OMXCodecWrapper in protected section and remove
> friend class here.
 'Private constructors & friend subclasses' is kind of my way of saying 'Nobody can subclass OMXCodecWrapper but me!' :)

> 
> @@ +199,5 @@
> > +  /**
> > +   * Configure video codec parameters and start media codec. It must be called
> > +   * before calling EncodeVideoFrame() and GetNextEncodedFrame().
> > +   */
> > +  nsresult ConfigureVideo(int aWidth, int aHeight, int aFrameRate);
> 
> Since you already subclass OMXCodecWrapper, there is no need Video postfix
> here
> OMXVideoEncoder::Config
> 
> @@ +208,5 @@
> > +   * microseconds). To notify end of stream, set aInputFlags to BUFFER_EOS.
> > +   */
> > +  void EncodeVideoFrame(const mozilla::layers::Image& aImage,
> > +                        int64_t aTimestamp, int aInputFlags = 0);
> > +
> 
> Encode
> 
> @@ +215,5 @@
> > +   * array. aTimestamp gives the frame timestamp/presentation time (in
> > +   * microseconds). To notify end of stream, set aInputFlags to BUFFER_EOS.
> > +   */
> > +  void EncodeVideoFrame(const nsTArray<uint8_t>& aImage,
> > +                        int64_t aTimestamp, int aInputFlags = 0);
> 
> Encode
 Good point. Will change those names.

> 
> @@ +230,5 @@
> > +   */
> > +  OMXVideoEncoder(CodecType aCodecType): OMXCodecWrapper(aCodecType) {}
> > +
> > +  // For creator function to access hidden constructor.
> > +  friend class OMXCodecWrapper;
> 
> Declare constructor of OMXCodecWrapper in protected section and remove
> friend class here.
 Same as above.

Comment 68

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

(In reply to C.J. Ku[:CJKu] from comment #66)
> Comment on attachment 8341012 [details] [diff] [review]
> Part 1: Add OMXCodecWrapper - v3
> 
> Review of attachment 8341012 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/omx/OMXCodecWrapper.cpp
> @@ +74,5 @@
> > +OMXCodecWrapper::~OMXCodecWrapper()
> > +{
> > +  if (mCodec.get()) {
> > +    Stop();
> > +    mCodec->release();
> 
> Need to call release manually?
 Yes. It says so in MediaCodec document.

> 
> @@ +151,5 @@
> > +  return err == OK ? NS_OK : NS_ERROR_FAILURE;
> > +}
> > +
> > +nsresult
> > +OMXAudioEncoder::ConfigureAudio(int aChannels, int aSamplingRate)
> 
> Change return value to boolean?
 Are you suggesting only report success/failure (no error code) here?

> 
> @@ +190,5 @@
> > +  sp<GraphicBuffer> graphicBuffer = GrallocBufferActor::GetFrom(grallocHandle);
> > +
> > +  // Size of PLANAR_YCBCR 4:2:0.
> > +  uint32_t frameLen = graphicBuffer->getWidth()*graphicBuffer->getHeight()*3/2;
> > +  //
> 
> Remove //
 Okay.

> 
> @@ +215,5 @@
> > +                                    int aInputFlags)
> > +{
> > +// MediaCodec accepts only 16-bit PCM data.
> > +#ifndef MOZ_SAMPLE_TYPE_S16
> > +  MOZ_ASSERT(false);
> 
> You want a compile time error or debug time error?
 You're right. Will change to #error instead.
> 
> @@ +225,5 @@
> > +}
> > +
> > +status_t
> > +OMXCodecWrapper::PushInput(const void* aData, size_t aSize, int64_t aTimestamp,
> > +                           int64_t aDuration, int aInputFlags, int64_t aTimeOut)
> 
> Instead of using return value of MediaCodec, you may change return value to
> boolean and check return value at the place you call this function.
> Please do a negative test in caller. If anything wrong here, we must have a
> way to report error here to MediaCodec, MediaCodec need to report error to
> MediaRecorder, MediaRecoder need to report error to js client.
 Are you saying that PushInput() should only report success/failure, and Encode() returns that to its caller like this?
  bool PushInput(...)
  ...
  bool Encode(...) {
    ...
    return PushInput(...);
  }

> 
> @@ +319,5 @@
> > +  uint32_t outFlags = 0;
> > +  bool retry = false;
> > +  do {
> > +    status_t err = mCodec->dequeueOutputBuffer(&index, &outOffset, &outSize,
> > +                           &outTimeUs, &outFlags, aTimeOut);
> 
> it's not err... it's a status code
 Didn't see the difference here. Enlighten me please?

Comment 69

[Shelly Lin [:shelly]]

Attachment: Part1: Make CreateMutedFrame static

Comment 70

[u459114]

(In reply to John Lin[:jolin][:jhlin] from comment #68)
> (In reply to C.J. Ku[:CJKu] from comment #66)
> > 
> > @@ +151,5 @@
> > > +  return err == OK ? NS_OK : NS_ERROR_FAILURE;
> > > +}
> > > +
> > > +nsresult
> > > +OMXAudioEncoder::ConfigureAudio(int aChannels, int aSamplingRate)
> > 
> > Change return value to boolean?
>  Are you suggesting only report success/failure (no error code) here?
Yes
> > @@ +225,5 @@
> > > +}
> > > +
> > > +status_t
> > > +OMXCodecWrapper::PushInput(const void* aData, size_t aSize, int64_t aTimestamp,
> > > +                           int64_t aDuration, int aInputFlags, int64_t aTimeOut)
> > 
> > Instead of using return value of MediaCodec, you may change return value to
> > boolean and check return value at the place you call this function.
> > Please do a negative test in caller. If anything wrong here, we must have a
> > way to report error here to MediaCodec, MediaCodec need to report error to
> > MediaRecorder, MediaRecoder need to report error to js client.
>  Are you saying that PushInput() should only report success/failure, and
> Encode() returns that to its caller like this?
>   bool PushInput(...)
>   ...
>   bool Encode(...) {
>     ...
>     return PushInput(...);
>   }
Yes, unless the caller really care about the status code, otherwise, success or fail report is enough for caller.
> > 
> > @@ +319,5 @@
> > > +  uint32_t outFlags = 0;
> > > +  bool retry = false;
> > > +  do {
> > > +    status_t err = mCodec->dequeueOutputBuffer(&index, &outOffset, &outSize,
> > > +                           &outTimeUs, &outFlags, aTimeOut);
> > 
> > it's not err... it's a status code
>  Didn't see the difference here. Enlighten me please?
dequeueOutputBuffer does not return error code, it returns status code. Name this variable as "err" is not match what exactly it means.

Comment 71

[u459114]

Comment on attachment 8341012 [details] [diff] [review]
Part 1: Add OMXCodecWrapper - v3

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

::: content/media/omx/OMXCodecWrapper.cpp
@@ +36,5 @@
> +OMXCodecWrapper::CreateAACEncoder()
> +{
> +  nsRefPtr<OMXAudioEncoder> aac = new OMXAudioEncoder(CodecType::AAC_ENC);
> +  // Return valid object only when media codec is valid.
> +  if (!aac->mCodec.get()) {

You never know how many things you need to check for each sub classes in super class.

I would suggest create a private member function: virtual bool Initiate();
Allocate MediaCodec, object resource and check allocation result in this function.

@@ +114,5 @@
> +  return err;
> +}
> +
> +nsresult
> +OMXVideoEncoder::ConfigureVideo(int aWidth, int aHeight, int aFrameRate)

Return boolean to indicate success or fail.

@@ +267,5 @@
> +
> +  return err;
> +}
> +
> +static void InsertAVCDecodeSpecificInfo(nsTArray<uint8_t>* aData) {

Comment for the purpose of this static function

@@ +287,5 @@
> +
> +  aData->AppendElements(csd, sizeof(csd));
> +}
> +
> +static void InsertAACDecodeSpecificInfo(nsTArray<uint8_t>* aData) {

Comment for the purpose of this static function

Comment 72

[u459114]

Comment on attachment 8341012 [details] [diff] [review]
Part 1: Add OMXCodecWrapper - v3

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

::: content/media/omx/OMXCodecWrapper.h
@@ +114,5 @@
> +   * aCodecType if CODEC_AAC_ENC, or AAC audio encoder if aCodecType is
> +   * CODEC_AVC_ENC.
> +   */
> +  OMXCodecWrapper(CodecType aCodecType);
> +

Do you still need to pass codec type in constructor after subclass?

Comment 74

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

(In reply to C.J. Ku[:CJKu] from comment #72)
> Comment on attachment 8341012 [details] [diff] [review]
> Part 1: Add OMXCodecWrapper - v3
> 
> Review of attachment 8341012 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/omx/OMXCodecWrapper.h
> @@ +114,5 @@
> > +   * aCodecType if CODEC_AAC_ENC, or AAC audio encoder if aCodecType is
> > +   * CODEC_AVC_ENC.
> > +   */
> > +  OMXCodecWrapper(CodecType aCodecType);
> > +
> 
> Do you still need to pass codec type in constructor after subclass?
 Yes. I wish to keep it for supporting more codec types in the future.

Comment 75

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

Attachment: Part 2: Add OMXCodecWrapper - v4.

- Addresses CJay's comments (naming, return types, ...).
- Fix incorrect decoder config data generation code.

Comment 76

[u459114]

Comment on attachment 8342285 [details] [diff] [review]
Part 2: Add OMXCodecWrapper - v4.

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

::: content/media/omx/OMXCodecWrapper.cpp
@@ +371,5 @@
> +      if (mCodecType == AAC_ENC) {
> +        InsertAVCDecoderConfig(aOutputBuf, omxBuf.get());
> +      } else if (mCodecType == AVC_ENC) {
> +        InsertAACDecoderConfig(aOutputBuf, omxBuf.get());
> +      }

create a pure virtual function in base class
virtual InsertDocoderConfig(nsTArray<uint8_t>* aOutputBuf, ABuffer* csd) = 0;

Implement InsertDocoderConfig in Audio and video concrete wrapper.

And make code here more simpler.
if (outFlags & MediaCodec::BUFFER_FLAG_CODECCONFIG) 
  InsertDocoderConfig(aOutputBuf, omxBuf.get());

I don't think it's a good idea to have AAC or AVC knowledge in the base class.
Please look through code, move AAC relative code into AduioWrapper, move AVC relative code into VideoWrapper

Comment 77

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

Comment on attachment 8342285 [details] [diff] [review]
Part 2: Add OMXCodecWrapper - v4.

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

::: content/media/omx/OMXCodecWrapper.cpp
@@ +176,5 @@
> +  } else {
> +    nsTArray<uint8_t> imgBuf;
> +    VideoTrackEncoder::CreateMutedFrame(&imgBuf, aWidth, aHeight);
> +    result = PushInput(imgBuf.Elements(), imgBuf.Length(), aTimestamp, 0,
> +                       aInputFlags, INPUT_BUFFER_TIMEOUT_US);

Rendering every format other than GRALLOC_PLANAR_YCBCR as black isn't good :-).

If you don't want to handle other formats, at least do NS_ERROR when you see them.

I think you should probably handle PLANAR_YCBCR so our software video decoders produce frames that you can consume. It should be relatively easy to handle.

For other types I think you could use Image->GetAsSurface()->GetAsImageSurface(), convert to YCBCR and feed that to the encoder.

@@ +252,5 @@
> +
> +    // Copy data to this input buffer.
> +    memcpy(inBuf->data(), aData + copied, toCopy);
> +    copied += toCopy;
> +    timeOffset = aDuration * copied / aSize;

What's the plan for getting rid of this copy?

Is there a good reason to not just get rid of it now?

I think we can get rid of it by having OMXCodecWrapper take a reference to the Image objects that are in mCodec's buffer.

@@ +287,5 @@
> +    0x20, // Object type: MPEG-4 video (defined in ISO/IEC 14492-2).
> +    0x11, // Stream type: visual, reserved: 1.
> +    0x01, 0x77, 0x00, // Buffer size: 96000.
> +    0x00, 0x03, 0xe8, 0x00, // Max bitrate: 256000.
> +    0x00, 0x03, 0xe8, 0x00, // Avg bitrate: 256000.

Shouldn't these values depend on the characteristics of the stream, such as the size of the frames and the frame rate?

::: content/media/omx/OMXCodecWrapper.h
@@ +134,5 @@
> +
> +  /**
> +   * Stop the media codec.
> +   */
> +  bool Stop();

I think these should return status_t or nsresult.

Comment 78

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #77)
> Comment on attachment 8342285 [details] [diff] [review]
> Part 2: Add OMXCodecWrapper - v4.
> 
> Review of attachment 8342285 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/omx/OMXCodecWrapper.cpp
> @@ +176,5 @@
> > +  } else {
> > +    nsTArray<uint8_t> imgBuf;
> > +    VideoTrackEncoder::CreateMutedFrame(&imgBuf, aWidth, aHeight);
> > +    result = PushInput(imgBuf.Elements(), imgBuf.Length(), aTimestamp, 0,
> > +                       aInputFlags, INPUT_BUFFER_TIMEOUT_US);
> 
> Rendering every format other than GRALLOC_PLANAR_YCBCR as black isn't good
> :-).
 Yeah. I found that problem too AFTER I uploaded the patch yesterday. Will fix it in next version.
 
> 
> @@ +252,5 @@
> > +
> > +    // Copy data to this input buffer.
> > +    memcpy(inBuf->data(), aData + copied, toCopy);
> > +    copied += toCopy;
> > +    timeOffset = aDuration * copied / aSize;
> 
> What's the plan for getting rid of this copy?
> 
> Is there a good reason to not just get rid of it now?
> 
> I think we can get rid of it by having OMXCodecWrapper take a reference to
> the Image objects that are in mCodec's buffer.
 I don't have a complete story for getting rid of the copy yet. In current MediaCodec (ACodec, acutally) implementation, input buffers are allocated and managed internally and user can only copy raw data to them for encoding. As far as I can tell, we can either modify stagefright code to accept buffers we supply, or use the result of MediaCodec::CreateInputSurface() as the camera preview surface (which creates a direct connection between camera and omx encoder that MSG cannot control). I'll post a proposal after I think it through.  :)

> 
> @@ +287,5 @@
> > +    0x20, // Object type: MPEG-4 video (defined in ISO/IEC 14492-2).
> > +    0x11, // Stream type: visual, reserved: 1.
> > +    0x01, 0x77, 0x00, // Buffer size: 96000.
> > +    0x00, 0x03, 0xe8, 0x00, // Max bitrate: 256000.
> > +    0x00, 0x03, 0xe8, 0x00, // Avg bitrate: 256000.
> 
> Shouldn't these values depend on the characteristics of the stream, such as
> the size of the frames and the frame rate?
 If I understand it correctly, the bitrate values only specify the required resources needed for decoding. These values are picked by Android MP4 muxer so I think they are good enough.

> 
> ::: content/media/omx/OMXCodecWrapper.h
> @@ +134,5 @@
> > +
> > +  /**
> > +   * Stop the media codec.
> > +   */
> > +  bool Stop();
> 
> I think these should return status_t or nsresult.
 I changed them to return bool since it seems MediaCodec doesn't actually report to caller any useful error code that's either informative or handleable.

Comment 79

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

Attachment: Part 2: Add OMXCodecWrapper - v5.

- change return value types.
- support non-gralloc YCbCr image as input.

Comment 80

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

Attachment: Part 4: Add OmxAudioTrackEncoder to file OmxTrackEncoder.

Audio track encoder for AAC.

Comment 81

[u459114]

Comment on attachment 8345223 [details] [diff] [review]
Part 2: Add OMXCodecWrapper - v5.

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

::: content/media/omx/OMXCodecWrapper.cpp
@@ +102,5 @@
> +    return OK;
> +  }
> +
> +  status_t result = mCodec->stop();
> +  mStarted = !(result == OK);

if result != OK, you still want to keep OMXCodecWrapper in start state?

@@ +285,5 @@
> +void
> +OMXVideoEncoder::AppendDecoderConfig(nsTArray<uint8_t>* aBuffer,
> +                                     ABuffer* aData) {
> +  size_t csdSize = aData->size();
> +

const size_t

@@ +406,5 @@
> +void
> +OMXAudioEncoder::AppendDecoderConfig(nsTArray<uint8_t>* aBuffer,
> +                                     ABuffer* aData) {
> +  size_t csdSize = aData->size();
> +

const size_t

@@ +459,5 @@
> +        retry = true;
> +        break;
> +      case INFO_FORMAT_CHANGED:
> +        // It's okay: for encoder, MediaCodec reports this only to inform caller
> +        //  that there will be a codec config buffer next.

Remove extra space before "that"

::: content/media/omx/OMXCodecWrapper.h
@@ +191,5 @@
> +
> +protected:
> +    virtual void AppendDecoderConfig(nsTArray<uint8_t>* aOutputBuf,
> +                                     ABuffer* aData) MOZ_OVERRIDE;
> +

alignment.

Comment 82

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

Attachment: Part 4: Add OmxAudioTrackEncoder to file OmxTrackEncoder.

Fix an uninitializd data member issue.

Comment 83

[u459114]

Comment on attachment 8345713 [details] [diff] [review]
Part 4: Add OmxAudioTrackEncoder to file OmxTrackEncoder.

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

::: content/media/encoder/OmxTrackEncoder.cpp
@@ +147,5 @@
> +nsresult
> +OmxAudioTrackEncoder::Init(int aChannels, int aSamplingRate)
> +{
> +  MOZ_ASSERT(!mInitialized && !mEncoder);
> +

check aSamplingRate at run-time. aSamplingRate must be bigger 0.

@@ +198,5 @@
> +  MOZ_ASSERT(frames > 0);
> +
> +  size_t copied = 0;
> +  // Sent input PCM data to encoder until queue is empty
> +  if (frames > 0) {

if (frames == 0) 
  return 0;

size_t copied = 0;

@@ +202,5 @@
> +  if (frames > 0) {
> +    // Get raw data from source
> +    AudioSegment::ChunkIterator iter(aSegment);
> +    size_t toCopy = 0;
> +    while (!iter.IsEnded()) {

for (;!iter.IsEnded(); iter.Next())
and remove line 221

@@ +304,5 @@
> +    rv = mEncoder->Encode(silence, mTimestamp, duration, OMXCodecWrapper::BUFFER_EOS);
> +  }
> +  if (NS_SUCCEEDED(rv)) {
> +    mTimestamp += duration;
> +  }

How about fail case?

Comment 84

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

(In reply to John Lin[:jolin][:jhlin] from comment #78)
>  I changed them to return bool since it seems MediaCodec doesn't actually
> report to caller any useful error code that's either informative or
> handleable.

Even if there's only one success and one failure code, it's still better to return nsresult or status_t since they are more meaningful than "bool". For example, it's never clear whether "true" means success or failure.

Comment 85

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

Comment on attachment 8345223 [details] [diff] [review]
Part 2: Add OMXCodecWrapper - v5.

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

::: content/media/omx/OMXCodecWrapper.cpp
@@ +158,5 @@
> +// For 4:4:4/4:2:2 -> 4:2:0, subsample using odd row/column without
> +// interpolation.
> +static
> +void
> +ConvertYUVPlanarTo420SP(PlanarYCbCrImage* aImage, uint8_t* dst)

You need to be very explicit about what the size of the dst buffer has to be here.

@@ +164,5 @@
> +  const PlanarYCbCrImage::Data* data = aImage->GetData();
> +
> +  // Fill Y plane.
> +  uint8_t* y = data->mYChannel;
> +  gfxIntSize ySize = data->mYSize;

What constraints are there on ySize? Assert them here.

@@ +171,5 @@
> +    size_t yLen = ySize.width * ySize.height;
> +    memcpy(dst, y, yLen);
> +    dst += yLen;
> +  } else {
> +    // Line by line copy.

I would remove the width==stride fast path here. I think it's not going to make you significantly faster. You should only keep it if you have some real evidence it is faster.

@@ +182,5 @@
> +
> +  // Fill interleaved UV plane.
> +  uint8_t* u = data->mCbChannel;
> +  uint8_t* v = data->mCrChannel;
> +  gfxIntSize uvSize = data->mCbCrSize;

Assert constriants on uvSize here.

@@ +184,5 @@
> +  uint8_t* u = data->mCbChannel;
> +  uint8_t* v = data->mCrChannel;
> +  gfxIntSize uvSize = data->mCbCrSize;
> +  size_t hSkip = ySize.width / uvSize.width;
> +  size_t vSkip = ySize.height / uvSize.height;

Shouldn't these be called 'stride' not 'skip'?

@@ +203,5 @@
> +status_t
> +OMXVideoEncoder::PushInputImage(const Image* aImage, int aWidth, int aHeight,
> +                                int64_t aTimestamp, int aInputFlags,
> +                                int64_t aTimeOut)
> +{

Need to assert some constraints here. If aImage is non-null do aWidth and aHeight have to equal the size of the image?

@@ +234,5 @@
> +        ConvertYUVPlanarTo420SP(static_cast<PlanarYCbCrImage*>(img), dst);
> +        break;
> +      default:
> +        // TODO: support RGB to YUV color conversion.
> +        MOZ_ASSERT(false, "Encoder doesn't support RGB image as input.");

Just say "unsupported image type" since we don't know what it is.

I don't think handling some types here and some types in Encode makes sense. Let's just fold this into Encode completely so we can test img for null and then switch to handle all types we're going to handle.

@@ +356,5 @@
> +}
> +
> +status_t
> +OMXCodecWrapper::PushInput(const void* aData, size_t aSize, int64_t aTimestamp,
> +                           int64_t aDuration, int aInputFlags, int64_t aTimeOut)

Call this PushInputAudio

@@ +394,5 @@
> +                                   aTimestamp + timeOffset, inFlags);
> +    if (result != OK) {
> +      break;
> +    }
> +  } while (copied < aSize);

Slightly more logical to make this a proper while loop (testing copies < aSize at the beginning)

Comment 86

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

Comment on attachment 8345713 [details] [diff] [review]
Part 4: Add OmxAudioTrackEncoder to file OmxTrackEncoder.

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

::: content/media/encoder/OmxTrackEncoder.cpp
@@ +219,5 @@
> +
> +      copied += toCopy;
> +      iter.Next();
> +    }
> +    MOZ_ASSERT(copied == frames);

Why don't we pass AudioChunks to OMXCodecWrapper and have it be responsible for conversion, so avoiding a copy of the audio data?

@@ +225,5 @@
> +  return copied;
> +}
> +
> +nsresult
> +OmxAudioTrackEncoder::FillEncodedFrames(EncodedFrameContainer& aContainer) {

{ on new line

::: content/media/encoder/OmxTrackEncoder.h
@@ +64,5 @@
> +  nsRefPtr<android::OMXAudioEncoder> mEncoder;
> +
> +  // The total duration of audio samples that have been copied from segment.
> +  int64_t mTimestamp; // in microseconds
> +  int64_t mSampleDuration; // time per sample in microseconds

Are these protected by the monitor or not? Please say so in comments.

Comment 87

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

Comment on attachment 8345223 [details] [diff] [review]
Part 2: Add OMXCodecWrapper - v5.

Revision needed for incorrect OMXVideoEncoder::AppendDecoderConfig() implementation.

Comment 88

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

Do we have new OMXCodecWrapper?

Comment 89

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

Attachment: Part 2: Add OMXCodecWrapper - v6.

* Addresses review comments, mostly rewrite image & audio input code to eliminate extra copies.
* Add AVC/H.264 decoder decriptor generation code (Last version incorrectly produces MPEG-4 Visual descriptor).

Comment 90

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #85)
> Comment on attachment 8345223 [details] [diff] [review]
> Part 2: Add OMXCodecWrapper - v5.
> 
> Review of attachment 8345223 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/omx/OMXCodecWrapper.cpp
> 
> @@ +164,5 @@
> > +  const PlanarYCbCrImage::Data* data = aImage->GetData();
> > +
> > +  // Fill Y plane.
> > +  uint8_t* y = data->mYChannel;
> > +  gfxIntSize ySize = data->mYSize;
> 
> What constraints are there on ySize? Assert them here.
> 
> 
> @@ +182,5 @@
> > +
> > +  // Fill interleaved UV plane.
> > +  uint8_t* u = data->mCbChannel;
> > +  uint8_t* v = data->mCrChannel;
> > +  gfxIntSize uvSize = data->mCbCrSize;
> 
> Assert constriants on uvSize here.
> 

 Couldn't think of constriants on these values. Would you mind enlighten me? Thanks a lot.

Comment 91

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

Attachment: Part 4: Add OmxAudioTrackEncoder to file OmxTrackEncoder - v2.

Addresses previous review comments, mostly move audio input conversion code to OMXCodecWrapper.

Comment 92

[u459114]

Comment on attachment 8348672 [details] [diff] [review]
Part 2: Add OMXCodecWrapper - v6.

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

::: content/media/omx/OMXCodecWrapper.cpp
@@ +115,5 @@
> +  MOZ_ASSERT(!mStarted);
> +
> +  NS_ENSURE_TRUE(aWidth > 0 && aHeight > 0 && aFrameRate > 0,
> +                 NS_ERROR_INVALID_ARG);
> +

Configure is a public function on OMXWrapper.
Instead only Assertion, you may need to validate parameter as well

@@ +198,5 @@
> +OMXVideoEncoder::Encode(const Image* aImage, int aWidth, int aHeight,
> +                        int64_t aTimestamp, int aInputFlags)
> +{
> +  MOZ_ASSERT(mStarted);
> +

Add comment here.
// Make sure Configure() be called before Encode()

@@ +600,5 @@
> +  MOZ_ASSERT(mStarted);
> +
> +  size_t numSamples = aSegment.GetDuration();
> +  NS_ENSURE_TRUE(numSamples > 0, NS_OK); // Source segment is empty.
> +

Remove this NS_ENSURE_TRUE or if statement at #616. They are conflict

@@ +608,5 @@
> +  NS_ENSURE_TRUE(result == OK, NS_ERROR_FAILURE);
> +  sp<ABuffer> inBuf = mInputBufs.itemAt(index);
> +  uint8_t* dst = inBuf->data();
> +  size_t dstSize = inBuf->capacity();
> +

Segment(#Line 606 ~ #Line 61) and segment(#Line 625 ~ #640) are basically duplicate. Move it out to a static free function

@@ +617,5 @@
> +    // Copy input PCM data to input buffer until queue is empty.
> +    AudioSegment::ChunkIterator iter(const_cast<AudioSegment&>(aSegment));
> +    while (!iter.IsEnded()) {
> +      AudioChunk chunk = *iter;
> +      size_t numToCopy = chunk.GetDuration(); // Number of samples to copy.

rename to samplesToCopy?

@@ +660,5 @@
> +    bytesCopied = mChannels * sizeof(AudioDataValue);
> +    memset(dst, 0, bytesCopied);
> +    numCopied = 1;
> +  }
> +

if "numSamples == 0" and "aInputFlags & BUFFER_EOS" is false, we don't expect this condition, have an assertion here.

::: content/media/omx/OMXCodecWrapper.h
@@ +152,5 @@
> +
> +  Vector<sp<ABuffer> > mInputBufs; // MediaCodec buffers to hold input data.
> +  Vector<sp<ABuffer> > mOutputBufs; // MediaCodec buffers to hold output data.
> +
> +  CodecType mCodecType;

You create a creator function for each codec type, I can't find any reason now to have this data member.
CreateAACEncoder for AAC
CreateAVCEncoder for H264

Add this data back in the future when you really need it

Comment 93

[u459114]

> > +  MOZ_ASSERT(!mStarted);
> > +
> > +  NS_ENSURE_TRUE(aWidth > 0 && aHeight > 0 && aFrameRate > 0,
> > +                 NS_ERROR_INVALID_ARG);
> > +
> 
> Configure is a public function on OMXWrapper.
> Instead only Assertion, you may need to validate parameter as well
> 
Ignore this one. In fact, you already do parameter check here

Comment 94

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

Comment on attachment 8348676 [details] [diff] [review]
Part 4: Add OmxAudioTrackEncoder to file OmxTrackEncoder - v2.

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

::: content/media/encoder/OmxTrackEncoder.cpp
@@ +195,5 @@
> +  nsresult rv = mEncoder->GetNextEncodedFrame(&frameData, &outTime, &outFlags,
> +                                              3000); // wait up to 3ms
> +  if (rv != NS_OK) {
> +    return rv;
> +  }

NS_ENSURE_SUCCESS ?

@@ +204,5 @@
> +      isCSD = true;
> +    } else if (outFlags & OMXCodecWrapper::BUFFER_EOS) { // last frame
> +      mEncodingComplete = true;
> +    } else {
> +      MOZ_ASSERT(frameData.Length() == OMXCodecWrapper::kAACFrameSize);

How does this assert be triggered?

@@ +210,5 @@
> +
> +    nsRefPtr<EncodedFrame> audiodata = new EncodedFrame();
> +    audiodata->SetFrameType(isCSD?
> +      EncodedFrame::AAC_CSD : EncodedFrame::AUDIO_FRAME);
> +    audiodata->SetTimeStamp(outTime);

outTime = ms? us ?

@@ +221,5 @@
> +
> +nsresult
> +OmxAudioTrackEncoder::GetEncodedTrack(EncodedFrameContainer& aData)
> +{
> +  MOZ_ASSERT(!mEncodingComplete);

you check it on L238:  if (mCanceled || mEncodingComplete) {

@@ +242,5 @@
> +    segment.AppendFrom(&mRawSegment);
> +  }
> +
> +  nsresult rv = mEncoder->Encode(segment,
> +                                 mEndOfStream ? OMXCodecWrapper::BUFFER_EOS : 0);

Ensure rv?

Comment 95

[u459114]

Comment on attachment 8348676 [details] [diff] [review]
Part 4: Add OmxAudioTrackEncoder to file OmxTrackEncoder - v2.

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

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

Move down {?

@@ +202,5 @@
> +    bool isCSD = false;
> +    if (outFlags & OMXCodecWrapper::BUFFER_CODEC_CONFIG) { // codec specific data
> +      isCSD = true;
> +    } else if (outFlags & OMXCodecWrapper::BUFFER_EOS) { // last frame
> +      mEncodingComplete = true;

Is there dependency between OMXCodecWrapper::BUFFER_CODEC_CONFIG and outFlags & OMXCodecWrapper::BUFFER_EOS? Are they mutual exclusive?

If not, you may not use "else if" statement here.
bool isCSD = (outFlags & OMXCodecWrapper::BUFFER_CODEC_CONFIG);
mEncodingComplete = (outFlags & OMXCodecWrapper::BUFFER_EOS);

@@ +236,5 @@
> +    }
> +
> +    if (mCanceled || mEncodingComplete) {
> +      return NS_ERROR_FAILURE;
> +    }

Open Questions:
Should OmxAudioTrackEncoder::GetEncodedTrack be called after EOS, mEncodingComplete == true?
Do we really need mEncodingComplete member? Does TrackEncoder really need to know EOS? Or, we may only have MediaEncoder knows that?

Comment 96

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

Comment on attachment 8348676 [details] [diff] [review]
Part 4: Add OmxAudioTrackEncoder to file OmxTrackEncoder - v2.

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

::: content/media/encoder/OmxTrackEncoder.cpp
@@ +237,5 @@
> +
> +    if (mCanceled || mEncodingComplete) {
> +      return NS_ERROR_FAILURE;
> +    }
> +

The MediaEncoder would try to check IsEncodingComplete before call the GetEncodedTrack.
And pass this information to muxer.

Comment 97

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

(In reply to C.J. Ku[:CJKu] from comment #92)
> Comment on attachment 8348672 [details] [diff] [review]
> Part 2: Add OMXCodecWrapper - v6.
> 
> Review of attachment 8348672 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/omx/OMXCodecWrapper.cpp
> @@ +115,5 @@
> > +  MOZ_ASSERT(!mStarted);
> > +
> > +  NS_ENSURE_TRUE(aWidth > 0 && aHeight > 0 && aFrameRate > 0,
> > +                 NS_ERROR_INVALID_ARG);
> > +
> 
> Configure is a public function on OMXWrapper.
> Instead only Assertion, you may need to validate parameter as well

 NS_ENSURE_TRUE() does that.

> 
> @@ +198,5 @@
> > +OMXVideoEncoder::Encode(const Image* aImage, int aWidth, int aHeight,
> > +                        int64_t aTimestamp, int aInputFlags)
> > +{
> > +  MOZ_ASSERT(mStarted);
> > +
> 
> Add comment here.
> // Make sure Configure() be called before Encode()

 Will append message text to make it more clear.

> 
> @@ +600,5 @@
> > +  MOZ_ASSERT(mStarted);
> > +
> > +  size_t numSamples = aSegment.GetDuration();
> > +  NS_ENSURE_TRUE(numSamples > 0, NS_OK); // Source segment is empty.
> > +
> 
> Remove this NS_ENSURE_TRUE or if statement at #616. They are conflict

 Will remove NS_ENSURE_TRUE since we should accept empty segment for EOS notification case.

> 
> @@ +608,5 @@
> > +  NS_ENSURE_TRUE(result == OK, NS_ERROR_FAILURE);
> > +  sp<ABuffer> inBuf = mInputBufs.itemAt(index);
> > +  uint8_t* dst = inBuf->data();
> > +  size_t dstSize = inBuf->capacity();
> > +
> 
> Segment(#Line 606 ~ #Line 61) and segment(#Line 625 ~ #640) are basically
> duplicate. Move it out to a static free function

 Will do.

> 
> @@ +617,5 @@
> > +    // Copy input PCM data to input buffer until queue is empty.
> > +    AudioSegment::ChunkIterator iter(const_cast<AudioSegment&>(aSegment));
> > +    while (!iter.IsEnded()) {
> > +      AudioChunk chunk = *iter;
> > +      size_t numToCopy = chunk.GetDuration(); // Number of samples to copy.
> 
> rename to samplesToCopy?

 Will do, and also rename numCopied to samplesCopied.

> 
> @@ +660,5 @@
> > +    bytesCopied = mChannels * sizeof(AudioDataValue);
> > +    memset(dst, 0, bytesCopied);
> > +    numCopied = 1;
> > +  }
> > +
> 
> if "numSamples == 0" and "aInputFlags & BUFFER_EOS" is false, we don't
> expect this condition, have an assertion here.

 Actually that occurs from time to time, and I think it's a valid case. We can reject it earlier in track encoder but I don't think it matters much.

> 
> ::: content/media/omx/OMXCodecWrapper.h
> @@ +152,5 @@
> > +
> > +  Vector<sp<ABuffer> > mInputBufs; // MediaCodec buffers to hold input data.
> > +  Vector<sp<ABuffer> > mOutputBufs; // MediaCodec buffers to hold output data.
> > +
> > +  CodecType mCodecType;
> 
> You create a creator function for each codec type, I can't find any reason
> now to have this data member.
> CreateAACEncoder for AAC
> CreateAVCEncoder for H264
> 
> Add this data back in the future when you really need it

 Will do.

Comment 98

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

(In reply to Randy Lin [:rlin] from comment #94)
> Comment on attachment 8348676 [details] [diff] [review]
> Part 4: Add OmxAudioTrackEncoder to file OmxTrackEncoder - v2.
> 
> Review of attachment 8348676 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/encoder/OmxTrackEncoder.cpp
> @@ +195,5 @@
> > +  nsresult rv = mEncoder->GetNextEncodedFrame(&frameData, &outTime, &outFlags,
> > +                                              3000); // wait up to 3ms
> > +  if (rv != NS_OK) {
> > +    return rv;
> > +  }
> 
> NS_ENSURE_SUCCESS ?

 Will do.

> 
> @@ +204,5 @@
> > +      isCSD = true;
> > +    } else if (outFlags & OMXCodecWrapper::BUFFER_EOS) { // last frame
> > +      mEncodingComplete = true;
> > +    } else {
> > +      MOZ_ASSERT(frameData.Length() == OMXCodecWrapper::kAACFrameSize);
> 
> How does this assert be triggered?

 This assertion is here to make sure encoded data is consistent with meta data. Will add some comment to make it more clear.

> 
> @@ +210,5 @@
> > +
> > +    nsRefPtr<EncodedFrame> audiodata = new EncodedFrame();
> > +    audiodata->SetFrameType(isCSD?
> > +      EncodedFrame::AAC_CSD : EncodedFrame::AUDIO_FRAME);
> > +    audiodata->SetTimeStamp(outTime);
> 
> outTime = ms? us ?

 Will rename to outTimeUs to make it more clear.

> 
> @@ +221,5 @@
> > +
> > +nsresult
> > +OmxAudioTrackEncoder::GetEncodedTrack(EncodedFrameContainer& aData)
> > +{
> > +  MOZ_ASSERT(!mEncodingComplete);
> 
> you check it on L238:  if (mCanceled || mEncodingComplete) {

 Will remove this assertion.

> 
> @@ +242,5 @@
> > +    segment.AppendFrom(&mRawSegment);
> > +  }
> > +
> > +  nsresult rv = mEncoder->Encode(segment,
> > +                                 mEndOfStream ? OMXCodecWrapper::BUFFER_EOS : 0);
> 
> Ensure rv?

 Will do.

Comment 99

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

(In reply to C.J. Ku[:CJKu] from comment #95)
> Comment on attachment 8348676 [details] [diff] [review]
> Part 4: Add OmxAudioTrackEncoder to file OmxTrackEncoder - v2.
> 
> Review of attachment 8348676 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/encoder/OmxTrackEncoder.cpp
> @@ +186,5 @@
> > +  return meta.forget();
> > +}
> > +
> > +nsresult
> > +OmxAudioTrackEncoder::AppendEncodedFrames(EncodedFrameContainer& aContainer) {
> 
> Move down {?

 Will do.

> 
> @@ +202,5 @@
> > +    bool isCSD = false;
> > +    if (outFlags & OMXCodecWrapper::BUFFER_CODEC_CONFIG) { // codec specific data
> > +      isCSD = true;
> > +    } else if (outFlags & OMXCodecWrapper::BUFFER_EOS) { // last frame
> > +      mEncodingComplete = true;
> 
> Is there dependency between OMXCodecWrapper::BUFFER_CODEC_CONFIG and
> outFlags & OMXCodecWrapper::BUFFER_EOS? Are they mutual exclusive?

 They are mutual exclusive. There will always be at least one more output buffer after CODEC_CONFIG because you need to send something to media codec for EOS notification.

> 
> @@ +236,5 @@
> > +    }
> > +
> > +    if (mCanceled || mEncodingComplete) {
> > +      return NS_ERROR_FAILURE;
> > +    }
> 
> Open Questions:
> Should OmxAudioTrackEncoder::GetEncodedTrack be called after EOS,
> mEncodingComplete == true?

 I think so. There could be some encoded frames left in media codec output buffer and we should keep getting them until BUFFER_EOS.

> Do we really need mEncodingComplete member? Does TrackEncoder really need to
> know EOS? Or, we may only have MediaEncoder knows that?

 If think so, track encoder needs to know EOS to notify media codec about it.

Comment 100

[u459114]

Comment on attachment 8348672 [details] [diff] [review]
Part 2: Add OMXCodecWrapper - v6.

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

::: content/media/omx/OMXCodecWrapper.cpp
@@ +517,5 @@
> +                                     ABuffer* aData) {
> +  const size_t csdSize = aData->size();
> +  const uint8_t *csd = aData->data();
> +  if (csdSize < sizeof(kNALUnitStartCode)) {
> +    return ERROR_MALFORMED;

assertion(false)

@@ +519,5 @@
> +  const uint8_t *csd = aData->data();
> +  if (csdSize < sizeof(kNALUnitStartCode)) {
> +    return ERROR_MALFORMED;
> +  }
> +

Two empty lines

@@ +746,5 @@
> +        return NS_OK;
> +      case -EAGAIN:
> +        // Output buffer not available. Caller can try again later.
> +        return NS_OK;
> +      default:

Dump result in logger and have an assertion(false) here

Comment 101

[u459114]

(In reply to John Lin[:jolin][:jhlin] from comment #97)
> (In reply to C.J. Ku[:CJKu] from comment #92)
> > 
> > @@ +660,5 @@
> > > +    bytesCopied = mChannels * sizeof(AudioDataValue);
> > > +    memset(dst, 0, bytesCopied);
> > > +    numCopied = 1;
> > > +  }
> > > +
> > 
> > if "numSamples == 0" and "aInputFlags & BUFFER_EOS" is false, we don't
> > expect this condition, have an assertion here.
> 
>  Actually that occurs from time to time, and I think it's a valid case. We
> can reject it earlier in track encoder but I don't think it matters much.
> 

It happens while Encode function be called while no new raw samples carries in media stream. In that case, you don't need event dequeue/enqueue a buffer from/to MediaCodec

Comment 102

[Ivan Tsay (:ITsay)]

Plus it for v1.4 because it is the committed feature work

Comment 103

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

(In reply to C.J. Ku[:CJKu] from comment #100)
> Comment on attachment 8348672 [details] [diff] [review]
> Part 2: Add OMXCodecWrapper - v6.
> 
> Review of attachment 8348672 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/omx/OMXCodecWrapper.cpp
> @@ +517,5 @@
> > +                                     ABuffer* aData) {
> > +  const size_t csdSize = aData->size();
> > +  const uint8_t *csd = aData->data();
> > +  if (csdSize < sizeof(kNALUnitStartCode)) {
> > +    return ERROR_MALFORMED;
> 
> assertion(false)

 Will do.

> 
> @@ +519,5 @@
> > +  const uint8_t *csd = aData->data();
> > +  if (csdSize < sizeof(kNALUnitStartCode)) {
> > +    return ERROR_MALFORMED;
> > +  }
> > +
> 
> Two empty lines

 Will fix.

> 
> @@ +746,5 @@
> > +        return NS_OK;
> > +      case -EAGAIN:
> > +        // Output buffer not available. Caller can try again later.
> > +        return NS_OK;
> > +      default:
> 
> Dump result in logger and have an assertion(false) here
 
 Will do.

Comment 104

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

(In reply to C.J. Ku[:CJKu] from comment #101)
> (In reply to John Lin[:jolin][:jhlin] from comment #97)
> > (In reply to C.J. Ku[:CJKu] from comment #92)
> > > 
> > > @@ +660,5 @@
> > > > +    bytesCopied = mChannels * sizeof(AudioDataValue);
> > > > +    memset(dst, 0, bytesCopied);
> > > > +    numCopied = 1;
> > > > +  }
> > > > +
> > > 
> > > if "numSamples == 0" and "aInputFlags & BUFFER_EOS" is false, we don't
> > > expect this condition, have an assertion here.
> > 
> >  Actually that occurs from time to time, and I think it's a valid case. We
> > can reject it earlier in track encoder but I don't think it matters much.
> > 
> 
> It happens while Encode function be called while no new raw samples carries
> in media stream. In that case, you don't need event dequeue/enqueue a buffer
> from/to MediaCodec

 You're right. Will add a check before calling Encode() in OmxAudioTrackEncoder.

Comment 105

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

Attachment: Part 2: Add OMXCodecWrapper - v7.

Addresses feedback comments.

Comment 106

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

Attachment: Part 4: Add OmxAudioTrackEncoder to file OmxTrackEncoder - v3.

Addresses feedback comments.

Comment 107

[Shelly Lin [:shelly]]

Comment on attachment 8349331 [details] [diff] [review]
Part 4: Add OmxAudioTrackEncoder to file OmxTrackEncoder - v3.

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

::: content/media/encoder/OmxTrackEncoder.cpp
@@ +246,5 @@
> +    if (segment.GetDuration() > 0 || mEndOfStream) {
> +      // Notify EOS at least once, even when segment is empty.
> +      nsresult rv = mEncoder->Encode(segment,
> +                                mEndOfStream ? OMXCodecWrapper::BUFFER_EOS : 0);
> +      segment.RemoveLeading(segment.GetDuration());

I don't think you need to |RemoveLeading()| here, |segment| is a local variable and its destructor will be called at the exit of this function.

Comment 108

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

Comment on attachment 8349330 [details] [diff] [review]
Part 2: Add OMXCodecWrapper - v7.

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

::: configure.in
@@ +245,5 @@
>          MOZ_B2G_CAMERA=1
>          MOZ_OMX_DECODER=1
>          AC_SUBST(MOZ_OMX_DECODER)
> +        MOZ_OMX_ENCODER=1
> +        AC_SUBST(MOZ_OMX_ENCODER)

Separate this into another patch

::: content/media/omx/OMXCodecWrapper.h
@@ +1,4 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-*/
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

add new line here.

@@ +76,5 @@
> +  enum {
> +    kAACBitrate = 96000, // kbps
> +    kAACFrameSize = 768, // bytes
> +    kAACFrameDuration = 1024, // How many samples per AAC frame.
> +  };

use #DEFINE

@@ +110,5 @@
> +   * Construct codec specific configuration blob with given data aData generated
> +   * by media codec and append it into aOutputBuf. Needed by MP4 container
> +   * writer for generating decoder config box. Returns OK if succeed.
> +   */
> +  virtual status_t AppendDecoderConfig(nsTArray<uint8_t>* aOutputBuf,

GetCodecPrivatedata?

@@ +121,5 @@
> +  OMXCodecWrapper& operator=(const OMXCodecWrapper&) MOZ_DELETE;
> +
> +  /**
> +   * Create a media codec of given type. It will be a AVC/H.264 video encoder if
> +   * aCodecType if CODEC_AVC_ENC, or AAC audio encoder if aCodecType is

* aCodecType is CODEC_AVC_ENC

@@ +132,5 @@
> +  friend class OMXVideoEncoder;
> +
> +  /**
> +   * Start the media codec.
> +   */

// Start the media codec.

@@ +137,5 @@
> +  status_t Start();
> +
> +  /**
> +   * Stop the media codec.
> +   */

// ...

@@ +160,5 @@
> +public:
> +  /**
> +   * Configure audio codec parameters and start media codec. It must be called
> +   * before calling Encode() and GetNextEncodedFrame().
> +   */

// ...

@@ +166,5 @@
> +
> +  /**
> +   * Encode 16-bit PCM audio samples stored in aSegment. To notify end of
> +   * stream, set aInputFlags to BUFFER_EOS.
> +   */

// ...

Comment 109

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

oh, ignore the #define and the comment format, for public function, we use /* */, private to be //

Comment 110

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

Comment on attachment 8349330 [details] [diff] [review]
Part 2: Add OMXCodecWrapper - v7.

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

Can separate OMXVideoEncoder and OMXAudioEncoder into another files and patches? :)

::: content/media/omx/OMXCodecWrapper.cpp
@@ +312,5 @@
> +  {
> +  }
> +
> +  //  Parse SPS + PPS data and append descriptor blob to aOutputBuf.
> +  status_t AppendDescriptorBlobTo(nsTArray<uint8_t>* aOutputBuf)

AppendDescriptorBlob

@@ +528,5 @@
> +    // Already in descriptor format.
> +    // Should has at least 13 bytes.
> +    NS_ENSURE_TRUE(csdSize >= 13, ERROR_MALFORMED);
> +
> +    aOutputBuf->SetCapacity(csdSize);

don't need to SetCapacity

@@ +569,5 @@
> +
> +void
> +OMXAudioEncoder::InterleaveTrackData(AudioChunk& aChunk, int32_t aDuration,
> +                                     uint32_t aOutputChannels,
> +                                     AudioDataValue* aOutput)

dup with this one..
void
AudioTrackEncoder::InterleaveTrackData(AudioChunk& aChunk,

@@ +585,5 @@
> +                               aChunk.mBufferFormat, aDuration, aChunk.mVolume,
> +                               aOutputChannels, aOutput);
> +  }
> +}
> +

comment for this class.

@@ +732,5 @@
> +// MPEG4Writer::Track::writeMp4aEsdsBox() implemention in libstagefright.
> +status_t
> +OMXAudioEncoder::AppendDecoderConfig(nsTArray<uint8_t>* aOutputBuf,
> +                                     ABuffer* aData)
> +{

assert aData

@@ +761,5 @@
> +    0x01, // Size.
> +    0x02, // Fixed value.
> +  };
> +
> +  aOutputBuf->SetCapacity(sizeof(decConfig) + csdSize + sizeof(slConfig));

Do we need this?

@@ +786,5 @@
> +  uint32_t outFlags = 0;
> +  bool retry = false;
> +  do {
> +    status_t result = mCodec->dequeueOutputBuffer(&index, &outOffset, &outSize,
> +                           &outTimeUs, &outFlags, aTimeOut);

indent.

Comment 111

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

(In reply to Randy Lin [:rlin] from comment #110)
> Comment on attachment 8349330 [details] [diff] [review]
> Part 2: Add OMXCodecWrapper - v7.
> 
> Review of attachment 8349330 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can separate OMXVideoEncoder and OMXAudioEncoder into another files and
> patches? :)

 Prefer keeping it this way now because I couldn't see any significant benefit doing this at the moment (other than slightly reduce the chances of patch conflict).

> 
> @@ +528,5 @@
> > +    // Already in descriptor format.
> > +    // Should has at least 13 bytes.
> > +    NS_ENSURE_TRUE(csdSize >= 13, ERROR_MALFORMED);
> > +
> > +    aOutputBuf->SetCapacity(csdSize);
> 
> don't need to SetCapacity

 I thought this is a good practice since it reduces the chance that nsTArray might need to enlarge/realloc multiple times during appending data?

> 
> @@ +569,5 @@
> > +
> > +void
> > +OMXAudioEncoder::InterleaveTrackData(AudioChunk& aChunk, int32_t aDuration,
> > +                                     uint32_t aOutputChannels,
> > +                                     AudioDataValue* aOutput)
> 
> dup with this one..
> void
> AudioTrackEncoder::InterleaveTrackData(AudioChunk& aChunk,

 Unfortunately |AudioTrackEncoder::InterleaveTrackData| is protected and not accessible from |OMXCodecWrapper|. Refactoring it to a static function is another option but IMO it would be better to do that in another bug. I will file one if you like.

Comment 112

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

Attachment: Part 2: Add OMXCodecWrapper - v8.

* Addresses comments.
* Make OMXCodecWrapper non ref-counted

Comment 113

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

Attachment: Part 4: Add OmxAudioTrackEncoder to file OmxTrackEncoder - v4.

* Addresses comments.
* Use |nsAutoPtr| to reference encoder wrapper object since it's not ref-counted now.

Comment 114

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

Attachment: Part 5: Add build flag for OMX encoders.

Move configure.in change out of Part 2 patch.

Comment 115

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

(In reply to John Lin[:jolin][:jhlin] from comment #111)
> (In reply to Randy Lin [:rlin] from comment #110)
> > Comment on attachment 8349330 [details] [diff] [review]
> > Part 2: Add OMXCodecWrapper - v7.
> > 
> > Review of attachment 8349330 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Can separate OMXVideoEncoder and OMXAudioEncoder into another files and
> > patches? :)
> 
>  Prefer keeping it this way now because I couldn't see any significant
> benefit doing this at the moment (other than slightly reduce the chances of
> patch conflict).
hmm, I think better for reviewer :-)
> 
> > 
> > @@ +528,5 @@
> > > +    // Already in descriptor format.
> > > +    // Should has at least 13 bytes.
> > > +    NS_ENSURE_TRUE(csdSize >= 13, ERROR_MALFORMED);
> > > +
> > > +    aOutputBuf->SetCapacity(csdSize);
> > 
> > don't need to SetCapacity
> 
>  I thought this is a good practice since it reduces the chance that nsTArray
> might need to enlarge/realloc multiple times during appending data?
both function run through this->EnsureCapacity(Length()...
Maybe it is useful when you have AppendElement in a loop.
> 
> > 
> > @@ +569,5 @@
> > > +
> > > +void
> > > +OMXAudioEncoder::InterleaveTrackData(AudioChunk& aChunk, int32_t aDuration,
> > > +                                     uint32_t aOutputChannels,
> > > +                                     AudioDataValue* aOutput)
> > 
> > dup with this one..
> > void
> > AudioTrackEncoder::InterleaveTrackData(AudioChunk& aChunk,
> 
>  Unfortunately |AudioTrackEncoder::InterleaveTrackData| is protected and not
> accessible from |OMXCodecWrapper|. Refactoring it to a static function is
> another option but IMO it would be better to do that in another bug. I will
> file one if you like.

Comment 116

[Shelly Lin [:shelly]]

Comment on attachment 8349846 [details] [diff] [review]
Part 2: Add OMXCodecWrapper - v8.

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

::: content/media/omx/OMXCodecWrapper.cpp
@@ +326,5 @@
> +    size_t paramSetsSize = Parse(header);
> +    NS_ENSURE_TRUE(paramSetsSize > 0, ERROR_MALFORMED);
> +
> +    // Extra 1 byte for number of SPS & the other for number of PPS.
> +    aOutputBuf->SetCapacity(sizeof(header) + paramSetsSize + 2);

s/number/numbers

And don't use SetCapacity(), AppendElement or AppendElements does a pretty good job on memory management.

@@ +567,5 @@
> +static const int AUDIO_PROCESSING_FRAMES = 640; /* > 10ms of 48KHz audio */
> +static const uint8_t gZeroChannel[MAX_AUDIO_SAMPLE_SIZE*AUDIO_PROCESSING_FRAMES] = {0};
> +
> +void
> +OMXAudioEncoder::InterleaveTrackData(AudioChunk& aChunk, int32_t aDuration,

The easiest way of not duplicating this function is to pass in its AudioTrackEncoder.

::: content/media/omx/OMXCodecWrapper.h
@@ +80,5 @@
> +    kAACFrameDuration = 1024, // How many samples per AAC frame.
> +  };
> +
> +  /** Create a AAC audio encoder. Returns nullptr when failed. */
> +  static OMXAudioEncoder* CreateAACEncoder();

Why is the return type not already_AddRefed<OMXAudioEncoder>?

Comment 117

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

(In reply to Shelly Lin [:shelly] from comment #116)
> Comment on attachment 8349846 [details] [diff] [review]
> Part 2: Add OMXCodecWrapper - v8.
> 
> Review of attachment 8349846 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/omx/OMXCodecWrapper.cpp
> @@ +326,5 @@
> > +    size_t paramSetsSize = Parse(header);
> > +    NS_ENSURE_TRUE(paramSetsSize > 0, ERROR_MALFORMED);
> > +
> > +    // Extra 1 byte for number of SPS & the other for number of PPS.
> > +    aOutputBuf->SetCapacity(sizeof(header) + paramSetsSize + 2);
> 
> s/number/numbers
> 
> And don't use SetCapacity(), AppendElement or AppendElements does a pretty
> good job on memory management.

 Except for the one :rlin pointed out in comment 115 (which I'll fix), all other calls to SetCapacity() are followed by multiple AppendElements(). SetCapacity() function comment (http://dxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#1368) says it minimizes heap re-allocation and I think it's good.

> 
> @@ +567,5 @@
> > +static const int AUDIO_PROCESSING_FRAMES = 640; /* > 10ms of 48KHz audio */
> > +static const uint8_t gZeroChannel[MAX_AUDIO_SAMPLE_SIZE*AUDIO_PROCESSING_FRAMES] = {0};
> > +
> > +void
> > +OMXAudioEncoder::InterleaveTrackData(AudioChunk& aChunk, int32_t aDuration,
> 
> The easiest way of not duplicating this function is to pass in its
> AudioTrackEncoder.

 If I understand correctly, protected functions can only be invoked in subclass implementation code.

> 
> ::: content/media/omx/OMXCodecWrapper.h
> @@ +80,5 @@
> > +    kAACFrameDuration = 1024, // How many samples per AAC frame.
> > +  };
> > +
> > +  /** Create a AAC audio encoder. Returns nullptr when failed. */
> > +  static OMXAudioEncoder* CreateAACEncoder();
> 
> Why is the return type not already_AddRefed<OMXAudioEncoder>?

|OMXCodecWrapper| is no longer ref-counted in this patch, so the return type is changed too.

Comment 118

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

Attachment: Part 2: Add OMXCodecWrapper.

* OMXVideoEncoder - add buffer color conversion code for GrallocImage case: camera source uses NV21 but encoder accepts NV12.
* OMXVideoEncoder - replace AVC/H.264 NAL start code in encoded frames with unit length before sending them to MP4 container writer.

Comment 119

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

Attachment: Part 4: Add OmxAudioTrackEncoder to file OmxTrackEncoder.

some code style correction.

Comment 120

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

Comment on attachment 8351330 [details] [diff] [review]
Part 2: Add OMXCodecWrapper.

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

::: content/media/omx/OMXCodecWrapper.cpp
@@ +191,5 @@
> +  for (int i = 0; i < ySize.height; i++) {
> +    memcpy(aDestination, y, ySize.width);
> +    y += data->mYStride;
> +    aDestination += ySize.width;
> +  }

You're ignoring mYSkip/mCbSkip/mCrSkip. Either assert that they're zero, or handle non-zero values. It would be better to handle non-zero values, since it seems OMX video decoding can produce non-zero skip values (maybe we won't see that here because it will take the gralloc path? but it's a bit fragile to rely on that). It's probably worth having a separate slow path for non-zero mYSkip.

These values aren't documented properly in PlanarYCbCrData. Basically, mYSkip > 0 means that after reading each Y sample, skip the y pointer forward by mYSkip bytes. If you don't mind you could add a patch to ImageContainer.h documenting the members of PlanarYCbCrData.

@@ +198,5 @@
> +  uint8_t* u = data->mCbChannel;
> +  uint8_t* v = data->mCrChannel;
> +  mozilla::gfx::IntSize uvSize = data->mCbCrSize;
> +  size_t hStride = ySize.width / uvSize.width;
> +  size_t vStride = ySize.height / uvSize.height;

You're requiring that ySize.width is an exact multiple of uvSize.width and similar for height. Please assert that here.

@@ +251,5 @@
> +    Image* img = const_cast<Image*>(aImage);
> +    ImageFormat format = img->GetFormat();
> +
> +    MOZ_ASSERT(aWidth == img->GetSize().width &&
> +                aHeight == img->GetSize().height);

Remove 1 space before aHeight

@@ +264,5 @@
> +
> +      graphicBuffer->lock(GraphicBuffer::USAGE_SW_READ_MASK, &imgPtr);
> +      inBuf->setRange(0, yLen + uvLen);
> +      // Convert to NV12.
> +      ConvertNV21ToNV12(static_cast<uint8_t*>(imgPtr), dst, yLen, uvLen);

How do you know the gralloc buffer is NV21?

It seems kind of crazy that the gralloc video decoder produces a different format to the format used by the encoder. Is there no way to avoid that?

If we add mYSkip/mCbCrSkip support to ConvertYUVPlanarToNV12 maybe we can use that here instead of ConvertNV21ToNV12?

@@ +289,5 @@
> +// NAL unit start code.
> +static const uint8_t kNALUnitStartCode[] = { 0x00, 0x00, 0x00, 0x01 };
> +
> +// This class is used to generate AVC/H.264 decoder config descriptor blob from
> +// the SPS + PPS data.

Please explain "SPS" and "PPS" acronyms here.

@@ +325,5 @@
> +//     Length               (2 bytes)
> +//     PPS NAL unit         (1+ bytes)
> +//   ...
> +// --- End ---
> +class AVCSpecificDataParser {

This class needs a better name, something more specific to its function. I think it should also go into its own file, in its own patch. If you do that, the interface could just be a single function, and handle the "already have a descriptor blob" case internally. E.g.
void GenerateAVCDescriptorBlob(ABuffer* aData, nsTArray<uint8_t>* aOutputBuf);

@@ +526,5 @@
> +// MPEG4Writer::Track::makeAVCCodecSpecificData() and
> +// MPEG4Writer::Track::writeAvccBox()  implemention in libstagefright.
> +// Blob from encoder could be in descriptor format already, or Sequence
> +// parameter set + picture parameter set (defined ISO/IEC 14496-15 B.1).
> +// If later, it needs to be parsed and converted into descriptor format.

This seems nasty but I guess it's just Android being stupid.

Comment 121

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

Comment on attachment 8349849 [details] [diff] [review]
Part 5: Add build flag for OMX encoders.

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

Actually this needs build-system review

Comment 122

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

Comment on attachment 8351332 [details] [diff] [review]
Part 4: Add OmxAudioTrackEncoder to file OmxTrackEncoder.

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

::: content/media/encoder/OmxTrackEncoder.cpp
@@ +209,5 @@
> +    }
> +
> +    nsRefPtr<EncodedFrame> audiodata = new EncodedFrame();
> +    audiodata->SetFrameType(isCSD?
> +      EncodedFrame::AAC_CSD : EncodedFrame::AUDIO_FRAME);

Space before ?

::: content/media/encoder/OmxTrackEncoder.h
@@ +54,5 @@
> +protected:
> +  nsresult Init(int aChannels, int aSamplingRate) MOZ_OVERRIDE;
> +
> +private:
> +  // Append encodecd frames to aContainer.

Fix typo "encoded"

Comment 123

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #120)
> Comment on attachment 8351330 [details] [diff] [review]
> Part 2: Add OMXCodecWrapper.
> 
> Review of attachment 8351330 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/omx/OMXCodecWrapper.cpp
> @@ +191,5 @@
> > +  for (int i = 0; i < ySize.height; i++) {
> > +    memcpy(aDestination, y, ySize.width);
> > +    y += data->mYStride;
> > +    aDestination += ySize.width;
> > +  }
> 
> You're ignoring mYSkip/mCbSkip/mCrSkip. Either assert that they're zero, or
> handle non-zero values. It would be better to handle non-zero values, since
> it seems OMX video decoding can produce non-zero skip values (maybe we won't
> see that here because it will take the gralloc path? but it's a bit fragile
> to rely on that). It's probably worth having a separate slow path for
> non-zero mYSkip.

 Okay. Will handle non-zero skip values in next patch.

> 
> These values aren't documented properly in PlanarYCbCrData. Basically,
> mYSkip > 0 means that after reading each Y sample, skip the y pointer
> forward by mYSkip bytes. If you don't mind you could add a patch to
> ImageContainer.h documenting the members of PlanarYCbCrData.

 Will discuss with KanRu(the original author) about improving documenting those.

> 
> @@ +264,5 @@
> > +
> > +      graphicBuffer->lock(GraphicBuffer::USAGE_SW_READ_MASK, &imgPtr);
> > +      inBuf->setRange(0, yLen + uvLen);
> > +      // Convert to NV12.
> > +      ConvertNV21ToNV12(static_cast<uint8_t*>(imgPtr), dst, yLen, uvLen);
> 
> How do you know the gralloc buffer is NV21?

 Because encoded video shows odd color? :P
 Android camera API document recommends 2 preview image format: NV21 or YV12 (http://developer.android.com/reference/android/hardware/Camera.Parameters.html#setPreviewFormat%28int%29), and it looks like |GonkCameraControl| forces YUV420SP: http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkCameraControl.cpp?from=GonkCameracontrol.cpp#59

> 
> It seems kind of crazy that the gralloc video decoder produces a different
> format to the format used by the encoder. Is there no way to avoid that?

 A wild guess would be: decoder and camera preview produce format that Android rendering/display system prefers.
 For camera output, I think it's possible to improve this with "correct" Android camera API. If I understand correctly, GonkCamera only support preview data which is meant to be used for, well, previewing. Will continue to explore that approach and file a bug if it looks fine.

Comment 124

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

Attachment: Part 2-1: Add an utility function to convert codec specific data from OMX AVC encoder for MP4 container.

Move the code that converts codec specific data produced by OMX AVC encoder from OMXCodecWrapper implementation file to its own file.

Comment 125

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

Attachment: Part 2-2: Add OMXCodecWrapper - v9.

Move AVC codec specific data conversion code to its own file.

Comment 126

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

Attachment: Part 4: Add OmxAudioTrackEncoder to file OmxTrackEncoder.

r+ nits addressed.

Comment 127

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

Attachment: Part 3: Add OmxVideoTrackEncoder to file OmxTrackEncoder.

Revised to use latest OMXCodecWrapper API. Needs Shelly's comment.

Comment 128

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

Comment on attachment 8355476 [details] [diff] [review]
Part 2-2: Add OMXCodecWrapper - v9.

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

::: content/media/omx/OMXCodecWrapper.cpp
@@ +231,5 @@
> +  // V plane.
> +  PickPixels(v, aDestination + 1, uvSize.width, uvSize.height,
> +             horiSubsample * (1 + data->mCrSkip), // Source pixel stride
> +             vertSubsample * data->mCbCrStride, // Source line stride
> +             2); // Destination pixel stride.

You would probably find that it's faster to handle u and v together at the same time so we only walk through the destination buffer once. Also, we don't really benefit from calling PickPixels here since we'll never take the fast paths. So it probably makes sense --- no more complicated, and probably faster --- just to write another per-pixel loop here.

@@ +282,5 @@
> +      GrallocImage* nativeImage = static_cast<GrallocImage*>(img);
> +      SurfaceDescriptor handle = nativeImage->GetSurfaceDescriptor();
> +      SurfaceDescriptorGralloc gralloc = handle.get_SurfaceDescriptorGralloc();
> +      sp<GraphicBuffer> graphicBuffer = GrallocBufferActor::GetFrom(gralloc);
> +      graphicBuffer->lock(GraphicBuffer::USAGE_SW_READ_MASK, &imgPtr);

My question about NV21 really is: Isn't there some way to check that the buffer is in NV21 format?

@@ +302,5 @@
> +      PickPixels(src, dst + 1, aWidth, aHeight, 2, lineStride, 2);
> +      // Copy even(U) pixels to odd pixels in destination.
> +      PickPixels(src + 1, dst, aWidth, aHeight, 2, lineStride, 2);
> +
> +      graphicBuffer->unlock();

Is this code actually faster than just calling ConvertYUVPlanarToNV12? Why not just call ConvertYUVPlanarToNV12?

Comment 129

[Shelly Lin [:shelly]]

Comment on attachment 8355496 [details] [diff] [review]
Part 3: Add OmxVideoTrackEncoder to file OmxTrackEncoder.

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

Thanks for the revising, looks good overall :)

Comment 130

[Shelly Lin [:shelly]]

Since John has took over most of the patches, make more sense to direct the assignee to him.

Comment 131

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

Comment on attachment 8351330 [details] [diff] [review]
Part 2: Add OMXCodecWrapper.

Obsoleted by Attachment 8355472 [details] [diff] & 8355476.

Comment 132

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

Attachment: Part 2-2: Add OMXCodecWrapper - v10.

- Rewrite color conversion function to support NV21 -> NV12.
- Add assertion to check graphic buffer format from camera is NV21.

Comment 133

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #128)
> Comment on attachment 8355476 [details] [diff] [review]
> Part 2-2: Add OMXCodecWrapper - v9.
> 
> Review of attachment 8355476 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/omx/OMXCodecWrapper.cpp
> 
> @@ +282,5 @@
> > +      GrallocImage* nativeImage = static_cast<GrallocImage*>(img);
> > +      SurfaceDescriptor handle = nativeImage->GetSurfaceDescriptor();
> > +      SurfaceDescriptorGralloc gralloc = handle.get_SurfaceDescriptorGralloc();
> > +      sp<GraphicBuffer> graphicBuffer = GrallocBufferActor::GetFrom(gralloc);
> > +      graphicBuffer->lock(GraphicBuffer::USAGE_SW_READ_MASK, &imgPtr);
> 
> My question about NV21 really is: Isn't there some way to check that the
> buffer is in NV21 format?
> 

 GraphicBuffer::getPixelFormat() returns buffer color format. On Nexus 4 the value is HAL_PIXEL_FORMAT_YCrCb_420_SP (defined in B2G/system/core/include/system/graphics.h), which is NV21. However the header file also suggests this is a "legacy" value, and defines a new "flexible" value HAL_PIXEL_FORMAT_YCbCr_420_888 instead. Will assert here to make sure it's NV21 for now.

Comment 134

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

Attachment: Part 3: Add OmxVideoTrackEncoder to file OmxTrackEncoder.

Fix typos.

Comment 135

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

Comment on attachment 8356053 [details] [diff] [review]
Part 2-2: Add OMXCodecWrapper - v10.

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

::: content/media/omx/OMXCodecWrapper.cpp
@@ +164,5 @@
> +ConvertPlanarYCbCrToNV12(const PlanarYCbCrData* aSource, uint8_t* aDestination)
> +{
> +  // Fill Y plane.
> +  uint8_t* y = aSource->mYChannel;
> +  mozilla::gfx::IntSize ySize = aSource->mYSize;

Add "using namespace mozilla::gfx;" to this file so you don't need these prefixes.

Comment 136

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

Add dependency to bug 801571 because MediaCodec::extractCSD()(http://androidxref.com/4.2.2_r1/xref/frameworks/av/media/libstagefright/MediaCodec.cpp#1185) calls StringPrintf()(http://androidxref.com/4.2.2_r1/xref/frameworks/av/media/libstagefright/foundation/AString.cpp#323) that uses vasprintf() and meets exact same problem as bug 801571.

Hi Michael, could you please provide some suggestion about it? Patching extractCSD() to not use StringPrintf() can avoid this problem. The patch could be as simple as the following:

diff --git a/media/libstagefright/MediaCodec.cpp b/media/libstagefright/MediaCodec.cpp
index f412dc8..e7b7941 100644
--- a/media/libstagefright/MediaCodec.cpp
+++ b/media/libstagefright/MediaCodec.cpp
@@ -1390,7 +1390,9 @@ void MediaCodec::extractCSD(const sp<AMessage> &format) {
     size_t i = 0;
     for (;;) {
         sp<ABuffer> csd;
-        if (!format->findBuffer(StringPrintf("csd-%u", i).c_str(), &csd)) {
+        AString str("csd-", 10);
+        str.append(i);
+        if (!format->findBuffer(str.c_str(), &csd)) {
             break;
         }

Comment 137

[Michael Wu [:mwu]]

In this particular case, I suspect we might start seeing other pieces of code (like vendor hals) start to use vasprintf since bionic supports it now, so it's worth overriding it in a place like mozglue/build/BionicGlue.cpp.

Comment 138

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

(In reply to Michael Wu [:mwu] from comment #137)
> In this particular case, I suspect we might start seeing other pieces of
> code (like vendor hals) start to use vasprintf since bionic supports it now,
> so it's worth overriding it in a place like mozglue/build/BionicGlue.cpp.

In that case I think we should continue the discussion in bug 801571. ;)

Comment 139

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

Attachment: Part 2: Add utility function to convert codec data from OMX AVC/H.264 encoder for MP4 container writer.

Update commit message for landing and carry r+ from roc.

Comment 140

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

Attachment: Part 3: Add OMXCodecWrapper.

- Get rid of duplicated audio sample copy code using new static function provided in bug 955981.
- Update commit message for landing and carry r+ from roc.

Comment 141

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

Attachment: Part 4: Add OmxVideoTrackEncoder to file OmxTrackEncoder.

- Fix nits.
- Update commit message for landing and carry r+ from roc.

Comment 142

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

Attachment: Part 5: Add OmxAudioTrackEncoder to file OmxTrackEncoder.

Update commit message for landing and carry r+ from roc.

Comment 143

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

Attachment: Part 6: Add build flag for OMX encoders.

Update commit message for landing and carry r+ from gps.

Comment 144

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

Try result: https://tbpl.mozilla.org/?tree=Try&rev=c66dfd2cfff7

Comment 145

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

follow by bug 801571, mark check-in needed with part 1-6.

Comment 146

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/1395d2a59698
https://hg.mozilla.org/integration/b2g-inbound/rev/ab65363c73e3
https://hg.mozilla.org/integration/b2g-inbound/rev/20cdddbc8cef
https://hg.mozilla.org/integration/b2g-inbound/rev/58053102c604
https://hg.mozilla.org/integration/b2g-inbound/rev/e751d3f676a2
https://hg.mozilla.org/integration/b2g-inbound/rev/6496b4cc3966

Comment 147

[Shelly Lin [:shelly]]

Part 1 was meant for use in OMXCodecWrapper, but if John decide not to use it later on, why do you still want to check in that patch? (plus, IT IS NOT REVIEWED YET!!)

Comment 148

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

ah.........This patch has roc reviewed. (Attachment #8341608 [details] [diff])
==>Bug 879668 - Part 1: Make CreateMutedFrame static. r=roc.
And john also run all of those patches on try server.
Something wrong?

Comment 149

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

Hi Ryan, 
Could you help to backout this one?
https://hg.mozilla.org/integration/b2g-inbound/rev/1395d2a59698
try result.
https://tbpl.mozilla.org/?tree=Try&rev=887f7868fffa

Comment 150

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/7b655cd5cce8

Assuming [leave open] until Part 1 re-lands.

Comment 151

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

Hi Ryan, 
We don't need the part 1 patch.

Comment 152

[Ryan VanderMeulen [:RyanVM]]

You don't need to needinfo? me to remove the [leave open] if it isn't needed.

Comment 153

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ab65363c73e3
https://hg.mozilla.org/mozilla-central/rev/20cdddbc8cef
https://hg.mozilla.org/mozilla-central/rev/58053102c604
https://hg.mozilla.org/mozilla-central/rev/e751d3f676a2
https://hg.mozilla.org/mozilla-central/rev/6496b4cc3966