879669 - Support Video Encoder module in MediaEncoder framework

Status: RESOLVED FIXED

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

Description

[u459114]

Implement video encoding path in MediaEncoding framework.

Two use cases for Video Encoding module
1. Video recording 
   With getUserMedia and MediaRecorder API, user is able to do video recording thing without mozCamera. 
   
2. WebRTC encoding path
   Redirect video encoding path in GIPS into MediaEncoding framework to take advantage of HW OMX component(Bug 879668)

Comment 2

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

Let it as meta bug

Comment 3

[Benjamin Chen [:bechen]]

Attachment: WIP:bug-879669-VideoTrackEncoder.v01.patch

It's a wip patch: basic videoTrackEncoder implementaion that I extract from bug 881840.

Hi Randy:
As we discussed before, the interface |GetEncodedTrack(nsTArray<uint8_t>* aOutput, int &aOutputDuration) and GetHeader(nsTArray<uint8_t>* aOutput) | can not be applied for various encoder.
Maybe we should re-design(or add) the interface for various video/audio mimetype on this bug.

Comment 4

[u459114]

Next blocker for encoding path, we need this bug be fixed so that all developers works on concrete video encoder, VP8/ OmxVideo, and container writer, such as MP4Writer/ WebMWriter, be capable to construct their module without patch need.

We need a prototype for discussion first.

Comment 5

[u459114]

For this bug, 
1. Track and writer creator need to be more generic.
2. No source buffer copy for both SW and HW track encoder path
3. If we can have a buffer arena for encoded buffer management, to prevent encoded buffer allocation, it would be great.

Comment 6

[Benjamin Chen [:bechen]]

Regarding the initial video track encoder parameters, I would like to add "trackRate" so that we can calculate the play time for encoded frame.
virtual nsresult Init(int aWidth, int aHeight, int aFrameRate, uint64_t aTacckRate) = 0;
                                                               ^^^^^^^^^^^^^^^^^^^

Comment 7

[u459114]

In track encoder/ writer factory function
Only "B2G + JB(ANDROID_VERSION >= 18)" uses MediaCodec, which means we create OmxVideoTrackEncoder/ OmxAudioTrackEncoder in recording path
On all the other platforms, we use VP8 + Vorbis.

Comment 8

[u459114]

(In reply to Benjamin Chen [:bechen] from comment #6)
> Regarding the initial video track encoder parameters, I would like to add
> "trackRate" so that we can calculate the play time for encoded frame.
> virtual nsresult Init(int aWidth, int aHeight, int aFrameRate, uint64_t
> aTacckRate) = 0;
What exactly track rate means?

Comment 9

[Benjamin Chen [:bechen]]

(In reply to C.J. Ku[:CJKu] from comment #8)
> (In reply to Benjamin Chen [:bechen] from comment #6)
> > Regarding the initial video track encoder parameters, I would like to add
> > "trackRate" so that we can calculate the play time for encoded frame.
> > virtual nsresult Init(int aWidth, int aHeight, int aFrameRate, uint64_t
> > aTacckRate) = 0;
> What exactly track rate means?
I think the track rate is the reciprocal of minimal time unit.
ex: track rate:90000; a video frame duration is 2. That means the frame should be played for 2/90000 seconds.

Comment 10

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

Does this parameter only required for vp8? 
Where can we get this parameter? from media stream?

Comment 11

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

Attachment: WIP2

WIP for supporting video encoder, some source code come from bechen's WIP patch.
ToDO:
1. EOS handle
2. Build Flag define
3. Avoid Buffer copy

Comment 12

[Shelly Lin [:shelly]]

Attachment: [WIP] VideoTrackEncoder and some refactor in AudioTrackEncoder

Need to clean up more.

Comment 13

[Shelly Lin [:shelly]]

Attachment: [WIP] VideoTrackEncoder and some refactor in AudioTrackEncoder

The previous one was empty lol.

Comment 14

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

Muxer needs to aware the the existence of A/V track during the init step.

Comment 15

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

Comment on attachment 825083 [details] [diff] [review]
[WIP] VideoTrackEncoder and some refactor in AudioTrackEncoder

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

two small change require

::: content/media/encoder/TrackEncoder.cpp
@@ +155,5 @@
> +    while (!iter.IsEnded()) {
> +      VideoChunk chunk = *iter;
> +      if (!chunk.IsNull()) {
> +        gfxIntSize size = chunk.mFrame.GetIntrinsicSize();
> +        nsresult rv = Init(size.width, size.height);

nsresult rv = Init(size.width, size.height, aTrackRate);

::: content/media/encoder/TrackEncoder.h
@@ +230,5 @@
> +  int mFrameHeight;
> +
> +  ReentrantMonitor mReentrantMonitor;
> +
> +  nsAutoPtr<VideoSegment> mRawSegment;

Those members should be protected..

Comment 16

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

Attachment: WIP3

Remove the VideoTrackEncoder changed by slin.
Add TestWriter.h. for testing.

Comment 17

[u459114]

Comment on attachment 825730 [details] [diff] [review]
WIP3

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

::: content/media/encoder/MediaEncoder.cpp
@@ +73,5 @@
>    LOG("NotifyRemoved in [MediaEncoder].");
>    mAudioEncoder->NotifyRemoved(aGraph);
>  }
>  
> +static const char* gSupportedmimeType[4] = {

static const char* gSupportedmimeType[]

@@ +121,3 @@
>  
>    encoder = new MediaEncoder(writer.forget(), audioEncoder.forget(),
> +                             videoEncoder.forget(), aMIMEType, isAudioOnly);

I don't understand why we need isAudioOnly here.
For audio only recording, videoEncoder should contain nullptr, why MediaEncoder does not use this value(videoEncoder) to tell whether it is audio only recording or video/audio recording.

@@ +218,5 @@
> +          LOG("ERROR! Fail to write encoded track to the media container.");
> +          mState = ENCODE_DONE;
> +          break;
> +        }
> +      }

189 ~ 204/ 205 ~ 222 are duplicate. Have another member function
  For exp: bool WriteEncodedTrack(TrackEncoder &trackEncoder);

Change code here to
if (!WriteEncodedTrack(mAudioEncoder) || !WriteEncodedTrack(mVideoEncoder))
{
  mState = ENCODE_DONE;
  break;
}

Comment 18

[Shelly Lin [:shelly]]

Attachment: [WIP] Part1: VideoTrackEncoder and some refactor in AudioTrackEncoder

Things that implemented in this patch:
- Appending video frames from MediaStreamGraph.
- Deal with muted video chunks.

TODO:
- Documentation.
- Test in real world.

Comment 19

[Shelly Lin [:shelly]]

Attachment: [WIP] Part2: Add video encoder to MediaEncoder, and some other changes in MediaRecorder

This is actually Randy's patch.

Comment 20

[Shelly Lin [:shelly]]

Attachment: [WIP] Part3: A fake test of video track encoder.

This is actually Randy's patch too.

Comment 21

[Ivan Tsay (:ITsay)]

Update milestone to sprint 5 after discussing the implementation status with engineer

Comment 22

[Jason Smith [:jsmith]]

FWIW - Let's make sure we're enhancing the existing mochitests to test the video encoder path as well. There's quite a lot of payoff you'll get if you do the work to make the tests work across audio & video encoding, as we'll be able expand to multiple audio/video codecs with existing tests easily.

Comment 23

[Alfredo Yang (:alfredo)]

http://dxr.mozilla.org/mozilla-central/source/content/media/encoder/ContainerWriter.h#l45

Would it be better not to use smart potiner as parameter?

Comment 24

[u459114]

Comment on attachment 826670 [details] [diff] [review]
[WIP] Part1: VideoTrackEncoder and some refactor in AudioTrackEncoder

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

::: content/media/encoder/TrackEncoder.cpp
@@ +156,5 @@
> +   // Check and initialize parameters for codec encoder.
> +  if (!mInitialized) {
> +    VideoSegment::ChunkIterator iter(const_cast<VideoSegment&>(video));
> +    while (!iter.IsEnded()) {
> +      VideoChunk chunk = *iter;

Are you sure you want deep copy here? Why not just use reference. Audio path has the same problem

Comment 25

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

[WIP] Part3: A fake test of video track encoder.
The dummy muxer can write data into file system. (separate video/audio)

Comment 26

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

Attachment: FAKE video track encoder /writer

This is a testing patch, this would write A/V encoded data directly into file system.

Comment 27

[Shelly Lin [:shelly]]

Attachment: [WIP] Part1: VideoTrackEncoder and some refactor in AudioTrackEncoder

Comment 28

[Shelly Lin [:shelly]]

Attachment: [WIP] Part1: VideoTrackEncoder and some refactor in AudioTrackEncoder

Fix compiling error.

Comment 29

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

Attachment: TestWriter for dump TrackEncoder

Comment 30

[Benjamin Chen [:bechen]]

Comment on attachment 827940 [details] [diff] [review]
[WIP] Part1: VideoTrackEncoder and some refactor in AudioTrackEncoder

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

::: content/media/encoder/TrackEncoder.cpp
@@ +210,5 @@
> +      mLastFrameSerial = image->GetSerial();
> +      mRawSegment->AppendFrame(image.forget(), chunk.mDuration,
> +                               chunk.mFrame.GetIntrinsicSize());
> +    }
> +

We should append all images into mRawSegment, or the timestamp/duration is not correct in mRawSegment.

And add a function in VideoSegment to merge the same videoChunk in mRawSegment before encoding.

Comment 31

[Shelly Lin [:shelly]]

Attachment: [WIP] Part1: VideoTrackEncoder and some refactor in AudioTrackEncoder

Major change:
In |AppendVideoSegment()|, use |mLastFrame| to hold the last different in-coming frame, and use |mLastFrameDuration| to accumulate duration of duplicated frames.

Comment 32

[Shelly Lin [:shelly]]

Attachment: [WIP] Small fix of part 2

Comment 33

[Shelly Lin [:shelly]]

Attachment: TestWriter for OmxEncoder

Comment 34

[Shelly Lin [:shelly]]

Comment on attachment 829851 [details] [diff] [review]
[WIP] Small fix of part 2

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

::: content/media/encoder/MediaEncoder.cpp
@@ +245,5 @@
>        }
>  
> +      // TODO: Should use writer->IsWritingComplete();
> +      mState = (mAudioEncoder->IsEncodingComplete() ||
> +                mVideoEncoder->IsEncodingComplete()) ? ENCODE_DONE : ENCODE_TRACK;

Randy: Maybe we should let the ContainerWriter decide when encoding is complete?

Comment 35

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

hmm. it should be. Let writer to decide this encoder session should be ended.

Comment 36

[u459114]

Comment on attachment 829851 [details] [diff] [review]
[WIP] Small fix of part 2

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

::: content/media/encoder/MediaEncoder.cpp
@@ +250,3 @@
>        break;
>      }
>  

Suggest moving codes in this case statement into another member function.

Comment 37

[u459114]

Comment on attachment 829851 [details] [diff] [review]
[WIP] Small fix of part 2

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

::: content/media/encoder/MediaEncoder.cpp
@@ +250,3 @@
>        break;
>      }
>  

Codes in video path and audio path are similar.
Please try to use member function or template to prevent redundant code.

Comment 38

[Benjamin Chen [:bechen]]

Attachment: [WIP] MergeSameVideoChunk.patch

Merge the same video chunk in VideoSegment.

Comment 39

[Shelly Lin [:shelly]]

Comment on attachment 830096 [details] [diff] [review]
[WIP] MergeSameVideoChunk.patch

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

::: content/media/encoder/VP8TrackEncoder.cpp
@@ +377,5 @@
>  }
>  #endif
>      mSourceSegment->AppendFrom(mRawSegment);
>      printf("mSourceSegment GetDuration %lld\n",mSourceSegment->GetDuration());
> +    mSourceSegment->MergeSameVideoChunk();

TrackEncoder sends raw frame to mRawSegment in every cycle, are you queuing the mSourceSegment here? How do you know the merge is complete and can be sent to encoder?

Comment 40

[Shelly Lin [:shelly]]

Attachment: [WIP] Part1: VideoTrackEncoder and some refactor in AudioTrackEncoder

Comment 41

[Benjamin Chen [:bechen]]

(In reply to Shelly Lin [:shelly] from comment #39)
> Comment on attachment 830096 [details] [diff] [review]
> [WIP] MergeSameVideoChunk.patch
> 
> Review of attachment 830096 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/encoder/VP8TrackEncoder.cpp
> @@ +377,5 @@
> >  }
> >  #endif
> >      mSourceSegment->AppendFrom(mRawSegment);
> >      printf("mSourceSegment GetDuration %lld\n",mSourceSegment->GetDuration());
> > +    mSourceSegment->MergeSameVideoChunk();
> 
> TrackEncoder sends raw frame to mRawSegment in every cycle, are you queuing
> the mSourceSegment here? How do you know the merge is complete and can be
> sent to encoder?
Yes, VP8TrackEncoder has a local queue mSourceSegment. So the merge function can be called in mSourceSegment.
Or if we don't have a local queue in XXXTrackEncoder, the function need to be protected by a lock.

Comment 42

[Shelly Lin [:shelly]]

Attachment: [WIP] Part1: VideoTrackEncoder (m-c changeset 154451)

Comment 43

[Shelly Lin [:shelly]]

Comment on attachment 830570 [details] [diff] [review]
[WIP] Part1: VideoTrackEncoder and some refactor in AudioTrackEncoder

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

::: content/media/encoder/TrackEncoder.cpp
@@ +220,5 @@
> +
> +        nsRefPtr<layers::Image> lastImg = mLastChunk->mFrame.GetImage();
> +        mRawSegment->AppendFrame(lastImg.forget(), mLastChunk->GetDuration(),
> +                                 mLastChunk->mFrame.GetIntrinsicSize());
> +        mLastChunk->mDuration = chunk.GetDuration();

Let me explain a bit more on this, of how it "Merges" duplicate chunks:

mLastChunk holds the image pointer of last in-coming frame (ln227, mLastChunk->mFrame.TakeFrom(&chunk.mFrame);), and check whether the frame in this iteration is the same as the one we hold in mLastChunk.

If this current chunk is a duplicate of mLastChunk (can be combined with mLastChunk), we sum the mLastChunk->mDuration with the duration of this chunk. Else, if this current chunk is a different one, which means mLastChunk has done accumulating its duration from the past duplicate chunks, we append mLastChunk to mRawSegment.

Comment 44

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

Attachment: [WIP] Part2 Media Encoder modify

Comment 45

[Shelly Lin [:shelly]]

Attachment: [WIP] Part1: VideoTrackEncoder (m-c changeset 154451)

With implementation of appending black frames if chunks are null.

Comment 46

[Shelly Lin [:shelly]]

Attachment: [WIP] Part1: VideoTrackEncoder (m-c changeset 154451)

Notify the wait of monitor when mRawSegment has appended some duration of chunks.

Comment 47

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

(In reply to Shelly Lin [:shelly] from comment #45)
> Created attachment 830690 [details] [diff] [review]
> [WIP] Part1: VideoTrackEncoder (m-c changeset 154451)
> 
> With implementation of appending black frames if chunks are null.

From the gUM Spec 
http://dev.w3.org/2011/webrtc/editor/getusermedia.html
The black frame may generate from media stream.

4. MediaStream API
The output of a MediaStream object must correspond to the tracks in its input. Muted audio tracks must be replaced with silence. Muted video tracks must be replaced with blackness.

Comment 48

[Benjamin Chen [:bechen]]

Comment on attachment 830746 [details] [diff] [review]
[WIP] Part1: VideoTrackEncoder (m-c changeset 154451)

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

::: content/media/encoder/TrackEncoder.cpp
@@ +211,5 @@
> +      mLastChunk->mFrame.SetForceBlack(chunk.mFrame.GetForceBlack());
> +      mLastChunk->mDuration = chunk.GetDuration();
> +    } else {
> +      if (mLastChunk->CanCombineWithFollowing(chunk)) {
> +        mLastChunk->mDuration += chunk.GetDuration();

The mLastChunk always queues one video chunk . So if there is a video chunk with long duration after merge, the track encoder and container writer won't output any data during this period.
Maybe we should compare the mLastChunk's duration with |GetPacketDuration| and then append it to mRawSegment (also reset the mFirstFrameTaken)

@@ +231,5 @@
> +  }
> +
> +  if (mRawSegment->GetDuration() > 0) {
> +    mReentrantMonitor.NotifyAll();
> +  }

Here we can call |GetPacketDuration| implement by VP8/OMX TrackEncder to notify the other thread.

Comment 49

[Benjamin Chen [:bechen]]

Comment on attachment 830746 [details] [diff] [review]
[WIP] Part1: VideoTrackEncoder (m-c changeset 154451)

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

::: content/media/encoder/TrackEncoder.cpp
@@ +268,5 @@
> +VideoTrackEncoder::CreateMutedFrame(nsTArray<uint8_t>* aOutputBuffer)
> +{
> +  // Supports YUV420 image format only, for now.
> +  int yPlaneLen = mFrameWidth *mFrameHeight;
> +  int cbcrPlaneLen = yPlaneLen/2;

The cbcrPlaneLen is not correct?
It should be "cbcrPlaneLen = ((mFrameWidth+1)/2) * ((mFrameHeight+1)/2)  * 2"

Another question for the black screen, only the member |mForceBlack| in class VideoFrame is set to TRUE. So the VP8/OMX track encoder needs to check the member and call
CreateMutedFrame before encoding. Can we do this in VideoTrackEncoder (replace the video chunk to black)?

Comment 50

[Shelly Lin [:shelly]]

(In reply to Benjamin Chen [:bechen] from comment #48)
> Comment on attachment 830746 [details] [diff] [review]
> [WIP] Part1: VideoTrackEncoder (m-c changeset 154451)
> 
> Review of attachment 830746 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/encoder/TrackEncoder.cpp
> @@ +211,5 @@
> > +      mLastChunk->mFrame.SetForceBlack(chunk.mFrame.GetForceBlack());
> > +      mLastChunk->mDuration = chunk.GetDuration();
> > +    } else {
> > +      if (mLastChunk->CanCombineWithFollowing(chunk)) {
> > +        mLastChunk->mDuration += chunk.GetDuration();
> 
> The mLastChunk always queues one video chunk . So if there is a video chunk
> with long duration after merge, the track encoder and container writer won't
> output any data during this period.
> Maybe we should compare the mLastChunk's duration with |GetPacketDuration|
> and then append it to mRawSegment (also reset the mFirstFrameTaken)
> 
> @@ +231,5 @@
> > +  }
> > +
> > +  if (mRawSegment->GetDuration() > 0) {
> > +    mReentrantMonitor.NotifyAll();
> > +  }
> 
> Here we can call |GetPacketDuration| implement by VP8/OMX TrackEncder to
> notify the other thread.
Yeah, that's better. As about the muted chunks, I think I'll use |GetPacketDuration| be the max duration of duplicated frames.

Comment 51

[Shelly Lin [:shelly]]

(In reply to Benjamin Chen [:bechen] from comment #49)
> Comment on attachment 830746 [details] [diff] [review]
> [WIP] Part1: VideoTrackEncoder (m-c changeset 154451)
> 
> Review of attachment 830746 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/encoder/TrackEncoder.cpp
> @@ +268,5 @@
> > +VideoTrackEncoder::CreateMutedFrame(nsTArray<uint8_t>* aOutputBuffer)
> > +{
> > +  // Supports YUV420 image format only, for now.
> > +  int yPlaneLen = mFrameWidth *mFrameHeight;
> > +  int cbcrPlaneLen = yPlaneLen/2;
> 
> The cbcrPlaneLen is not correct?
> It should be "cbcrPlaneLen = ((mFrameWidth+1)/2) * ((mFrameHeight+1)/2)  * 2"
> 
Do we need to consider the odd size resolution?

> Another question for the black screen, only the member |mForceBlack| in
> class VideoFrame is set to TRUE. So the VP8/OMX track encoder needs to check
> the member and call
> CreateMutedFrame before encoding. Can we do this in VideoTrackEncoder
> (replace the video chunk to black)?
I'm not sure what you mean....but what's the problem of calling |CreateMutedFrame| before encoding?

Comment 52

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

Comment on attachment 830746 [details] [diff] [review]
[WIP] Part1: VideoTrackEncoder (m-c changeset 154451)

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

::: content/media/encoder/TrackEncoder.cpp
@@ +268,5 @@
> +VideoTrackEncoder::CreateMutedFrame(nsTArray<uint8_t>* aOutputBuffer)
> +{
> +  // Supports YUV420 image format only, for now.
> +  int yPlaneLen = mFrameWidth *mFrameHeight;
> +  int cbcrPlaneLen = yPlaneLen/2;

use ceil... Could we assign the black memory pointer to encoder to avoid the memset operation?

::: content/media/encoder/TrackEncoder.h
@@ +77,5 @@
> +   */
> +  virtual void NotifyEndOfStream() = 0;
> +
> +  bool mInitialized;
> +  bool mDoneEncoding;

mEncodingDone?

@@ +81,5 @@
> +  bool mDoneEncoding;
> +
> +  /**
> +   * The total duration of null chunks we have received from MediaStreamGraph
> +   * before initializing the audio track encoder.

remove audio

Comment 53

[Shelly Lin [:shelly]]

Attachment: Part1: VideoTrackEncoder and some refactor in AudioTrackEncoder

Hi Robert, this patch is mainly for the implementation of VideoTrackEncoder. I also moved some common methods from AudioTrackEncoder to TrackEncoder, fixed some comments here and there just because I got confused by my previous code.

Comment 54

[Shelly Lin [:shelly]]

(In reply to Randy Lin [:rlin] from comment #52)
> Comment on attachment 830746 [details] [diff] [review]
> [WIP] Part1: VideoTrackEncoder (m-c changeset 154451)
> 
> Review of attachment 830746 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/encoder/TrackEncoder.cpp
> @@ +268,5 @@
> > +VideoTrackEncoder::CreateMutedFrame(nsTArray<uint8_t>* aOutputBuffer)
> > +{
> > +  // Supports YUV420 image format only, for now.
> > +  int yPlaneLen = mFrameWidth *mFrameHeight;
> > +  int cbcrPlaneLen = yPlaneLen/2;
> 
> use ceil... Could we assign the black memory pointer to encoder to avoid the
> memset operation?
Just saw your comments after I post the new patch....

If we want to deal with odd size resolution, maybe we should do it at where we assign mFrameWidth/mFrameHeight? instead of fixing the mathematical operation here and there.
> 
> ::: content/media/encoder/TrackEncoder.h
> @@ +77,5 @@
> > +   */
> > +  virtual void NotifyEndOfStream() = 0;
> > +
> > +  bool mInitialized;
> > +  bool mDoneEncoding;
> 
> mEncodingDone?
> 
I'll change it to mEncodingComplete just to match it with function |IsEncodingComplete()|.
> @@ +81,5 @@
> > +  bool mDoneEncoding;
> > +
> > +  /**
> > +   * The total duration of null chunks we have received from MediaStreamGraph
> > +   * before initializing the audio track encoder.
> 
> remove audio

Comment 55

[Shelly Lin [:shelly]]

Attachment: Part1: VideoTrackEncoder and some refactor in AudioTrackEncoder

Robert, please find my original comment in comment 53, this patch includes some nit fixes per Randy's comment.

Comment 56

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Shelly Lin [:shelly] from comment #54)

> If we want to deal with odd size resolution, maybe we should do it at where
> we assign mFrameWidth/mFrameHeight? instead of fixing the mathematical
> operation here and there.

We should not assume video resolutions are even.

Comment 57

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

Attachment: part2:MediaEncoder Change

Comment 58

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

Comment on attachment 8333669 [details] [diff] [review]
Part1: VideoTrackEncoder and some refactor in AudioTrackEncoder

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

Please submit separate patches for the refactoring vs the new code.

Comment 59

[Shelly Lin [:shelly]]

Attachment: Previous Part 1, for dev

Comment 60

[Shelly Lin [:shelly]]

Attachment: Part1: Refactor of TrackEncoder

Comment 61

[Shelly Lin [:shelly]]

Comment on attachment 8335006 [details] [diff] [review]
Part1: Refactor of TrackEncoder

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

::: content/media/encoder/EncodedFrameContainer.h
@@ +5,5 @@
>  
>  #ifndef EncodedFrameContainer_H_
>  #define EncodedFrameContainer_H_
>  
> +#include "nsAutoPtr.h"

I added this because the compiler is complaining about it when I include this header file in another header file that isn't using any nsAutoPtr.

Comment 62

[Shelly Lin [:shelly]]

Attachment: Part2: Add VideoTrackEncoder

Comment 63

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

Comment on attachment 8335006 [details] [diff] [review]
Part1: Refactor of TrackEncoder

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

::: content/media/encoder/TrackEncoder.h
@@ +69,5 @@
>    virtual nsresult GetEncodedTrack(EncodedFrameContainer& aData) = 0;
> +
> +  bool IsEncodingComplete() { return mEncodingComplete; }
> +
> +  protected:

Don't indent this line.

@@ +77,5 @@
> +   */
> +  virtual void NotifyEndOfStream() = 0;
> +
> +  bool mInitialized;
> +  bool mEncodingComplete;

Put these with the other bools below, and document them!

@@ +156,5 @@
>    /**
>     * Notifies the audio encoder that we have reached the end of source stream,
>     * and wakes up mReentrantMonitor if encoder is waiting for more track data.
>     */
> +  void NotifyEndOfStream() MOZ_OVERRIDE;

Should be declared "virtual"

@@ +177,4 @@
>  
>    /**
> +   * A ReentrantMonitor to protect the pushing and pulling of mRawSegment, and
> +   * some other flags.

Better say which flags :-)

Comment 64

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

Comment on attachment 8335014 [details] [diff] [review]
Part2: Add VideoTrackEncoder

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

::: content/media/encoder/TrackEncoder.cpp
@@ +202,5 @@
> +  // Append all video segments from MediaStreamGraph, including null an
> +  // non-null frames.
> +  VideoSegment::ChunkIterator iter(const_cast<VideoSegment&>(aSegment));
> +  while (!iter.IsEnded()) {
> +    VideoChunk chunk = *iter;

Why are we doing all this? Can't we just append aSegment to mRawSegment directly?

::: content/media/encoder/TrackEncoder.h
@@ +195,5 @@
> +    : TrackEncoder()
> +    , mFrameWidth(0)
> +    , mFrameHeight(0)
> +    , mTrackRate(0)
> +    , mLastChunk(new VideoChunk())

Initialize it to null instead of an empty video chunk

@@ +232,5 @@
> +  nsresult AppendVideoSegment(const VideoSegment& aSegment);
> +
> +  // Tells the video track encoder that we've reached the end of source stream,
> +  // and wakes up mReentrantMonitor if encoder is waiting for more track data.
> +  void NotifyEndOfStream() MOZ_OVERRIDE;

declare this as virtual.

@@ +251,5 @@
> +  // segment.
> +  bool mFirstFrameTaken;
> +
> +  // A ReentrantMonitor to protect the pushing and pulling of mRawSegment, and
> +  // some other flags.

Say which flags :-)

Comment 65

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

Comment on attachment 8335006 [details] [diff] [review]
Part1: Refactor of TrackEncoder

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

::: content/media/encoder/TrackEncoder.h
@@ +177,4 @@
>  
>    /**
> +   * A ReentrantMonitor to protect the pushing and pulling of mRawSegment, and
> +   * some other flags.

Actually, if we're protecting flags in the superclass, I think we should probably put the monitor in the superclass.

Comment 66

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

Hi roc, 
1. Do we need to support video only recording? Right now I just query the video/audio track number from mediaStream and choose the suitable mimeType according platform (for ex, desktop would be video/webm, b2g would be video/mp4), and pass the ContentHint/MimeType to encoder via MediaEncoder::CreateEncoder(aMimeType, aContentHinet) Is this design OK for this?
For the mimeType constant like video/mpv have not define in nsMimeType.h, can I add it?

Comment 67

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

Replace video/mpv to video/m4v for video only mp4.

Comment 68

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

As we discussed F2F, we should default to video/mp4 for anything with a video track in B2G, and video/webm for anything with a video track on desktop.

Comment 69

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

Thanks, I will register the cb to media stream track's change and choose the proper of video mimeType.

Comment 70

[Shelly Lin [:shelly]]

Attachment: Part1: Refactor of TrackEncoder

Comment 71

[Shelly Lin [:shelly]]

Attachment: Part1: Refactor of TrackEncoder

Comment 72

[Shelly Lin [:shelly]]

Comment on attachment 8335014 [details] [diff] [review]
Part2: Add VideoTrackEncoder

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

::: content/media/encoder/TrackEncoder.cpp
@@ +202,5 @@
> +  // Append all video segments from MediaStreamGraph, including null an
> +  // non-null frames.
> +  VideoSegment::ChunkIterator iter(const_cast<VideoSegment&>(aSegment));
> +  while (!iter.IsEnded()) {
> +    VideoChunk chunk = *iter;

Because aSegment is giving us duplicate images, we don't wanna send duplicate frames to the track encoder, making the encoder too busy on unnecessary frames.
 
However, we still need the durations from those duplicate images. mLastChunk is suppose to accumulate the duration.

Comment 73

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

Comment on attachment 8335014 [details] [diff] [review]
Part2: Add VideoTrackEncoder

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

::: content/media/encoder/TrackEncoder.cpp
@@ +202,5 @@
> +  // Append all video segments from MediaStreamGraph, including null an
> +  // non-null frames.
> +  VideoSegment::ChunkIterator iter(const_cast<VideoSegment&>(aSegment));
> +  while (!iter.IsEnded()) {
> +    VideoChunk chunk = *iter;

You can probably just call mRawSegment->AppendFrom(&chunk) here.

Comment 74

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

Comment on attachment 8335560 [details] [diff] [review]
Part1: Refactor of TrackEncoder

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

r+ with those fixed.

::: content/media/encoder/TrackEncoder.h
@@ +102,5 @@
> +
> +  /**
> +   * True if the track encoder has encoded all source data.
> +   */
> +  bool mEncodingComplete;

Put this boolean flag below with the other flags for better packing of fields.

@@ +106,5 @@
> +  bool mEncodingComplete;
> +
> +  /**
> +   * A ReentrantMonitor to protect the pushing and pulling of mRawSegment, and
> +   * flags such as mInitialized, mEndOfStream, mCanceled.

Add to this comment, to explain that subclasses add their own fields which are protected by mReentrantMonitor.

Comment 75

[Shelly Lin [:shelly]]

Attachment: Part1: Refactor of TrackEncoder

- Added comment to the patch description.
- Nit fix.

Comment 76

[Shelly Lin [:shelly]]

Attachment: Part1: Refactor of TrackEncoder

Comment 77

[Shelly Lin [:shelly]]

Attachment: Part2: Add VideoTrackEncoder

Comment 78

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

Attachment: part3:MediaEncoder Change v2

1. Get track information through TracksAvailableCallback.
2. because the error handle has been backout -.-..., this version is based on current mc version.

Comment 79

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

Comment on attachment 8336424 [details] [diff] [review]
Part2: Add VideoTrackEncoder

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

::: content/media/encoder/TrackEncoder.cpp
@@ +195,5 @@
> +  // Append this many null data to our queued segment if there is a completely
> +  // muted duration before receiving the first non-null video chunk.
> +  if (mNullDuration > 0) {
> +    mRawSegment->AppendNullData(mNullDuration);
> +    mCurrentFrameTimeStamp += TimeDuration::FromMicroseconds(mNullDuration);

mNullDuration isn't in microseconds, it's in MediaTime which is 2^-20 seconds.

Also, make mCurrentFrameTimeStamp a MediaTime too so we don't have rounding issues here.

Also, call it mTotalFrameDuration.

@@ +209,5 @@
> +    if (!mLastChunk || !mLastChunk->CanCombineWithFollowing(chunk)) {
> +      nsRefPtr<layers::Image> image = chunk.mFrame.GetImage();
> +      mRawSegment->AppendFrame(image.forget(), chunk.GetDuration(),
> +                               chunk.mFrame.GetIntrinsicSize());
> +      mRawSegment->GetLastChunk()->mTimeStamp = mCurrentFrameTimeStamp;

Why are you doing this line? mTimeStamp is only for latency calculations, which aren't relevant here I think.

@@ +213,5 @@
> +      mRawSegment->GetLastChunk()->mTimeStamp = mCurrentFrameTimeStamp;
> +    }
> +
> +    mCurrentFrameTimeStamp += TimeDuration::FromMicroseconds(chunk.GetDuration());
> +    mLastChunk = &chunk;

I don't think you want to take the address here. "chunk" is going out of scope so this will be a dangling pointer. You probably should make mLastChunk a VideoFrame, and use TakeFrom to take it from 'chunk' here.

Comment 80

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

Attachment: part3:MediaEncoder Change v3

Add build flags to decide which encoding format can be chosen.

Comment 81

[Shelly Lin [:shelly]]

Comment on attachment 8336424 [details] [diff] [review]
Part2: Add VideoTrackEncoder

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

::: content/media/encoder/TrackEncoder.cpp
@@ +209,5 @@
> +    if (!mLastChunk || !mLastChunk->CanCombineWithFollowing(chunk)) {
> +      nsRefPtr<layers::Image> image = chunk.mFrame.GetImage();
> +      mRawSegment->AppendFrame(image.forget(), chunk.GetDuration(),
> +                               chunk.mFrame.GetIntrinsicSize());
> +      mRawSegment->GetLastChunk()->mTimeStamp = mCurrentFrameTimeStamp;

I don't know what mTimeStamp stands for in each source chunk.

I'm trying to store the timestamp of this frame so that the OmxTrackEncoder can pull it out and send it to stagefright encoder.

@@ +258,5 @@
> +  aOutputBuffer->SetLength(frameLen);
> +  // Fill Y plane.
> +  memset(aOutputBuffer->Elements(), 0x10, yPlaneLen);
> +  // Fill Cb/Cr planes.
> +  memset(aOutputBuffer->Elements() + yPlaneLen, 0x80, cbcrPlaneLen);

Is it better if I use |nsAutoArrayPtr<uint8_t> buffer| inside the function and return buffer.forget() here?

Comment 82

[Benjamin Chen [:bechen]]

(In reply to Shelly Lin [:shelly] from comment #81)
> Comment on attachment 8336424 [details] [diff] [review]
> Part2: Add VideoTrackEncoder
> 
> Review of attachment 8336424 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/encoder/TrackEncoder.cpp
> @@ +209,5 @@
> > +    if (!mLastChunk || !mLastChunk->CanCombineWithFollowing(chunk)) {
> > +      nsRefPtr<layers::Image> image = chunk.mFrame.GetImage();
> > +      mRawSegment->AppendFrame(image.forget(), chunk.GetDuration(),
> > +                               chunk.mFrame.GetIntrinsicSize());
> > +      mRawSegment->GetLastChunk()->mTimeStamp = mCurrentFrameTimeStamp;
> 
> I don't know what mTimeStamp stands for in each source chunk.
> 
> I'm trying to store the timestamp of this frame so that the OmxTrackEncoder
> can pull it out and send it to stagefright encoder.
> 

I think you don't need to store the timestamp here because there is |mDuration| for each VideoChunk.
OmxTrackEncoder can calculate timestamp by mDuration and TrackRate if all VideoChunks are not missing in MediaStreamGraph.
Or if you really need the timestamp, you can pass/use the |TrackTicks aTrackOffset| in NotifyQueuedTrackChanges.

Comment 83

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

Comment on attachment 8336424 [details] [diff] [review]
Part2: Add VideoTrackEncoder

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

::: content/media/encoder/TrackEncoder.cpp
@@ +258,5 @@
> +  aOutputBuffer->SetLength(frameLen);
> +  // Fill Y plane.
> +  memset(aOutputBuffer->Elements(), 0x10, yPlaneLen);
> +  // Fill Cb/Cr planes.
> +  memset(aOutputBuffer->Elements() + yPlaneLen, 0x80, cbcrPlaneLen);

It depends on how it's going to be used. Can you return an already_AddRefed<Image>?

Comment 84

[Shelly Lin [:shelly]]

Attachment: Part2: Add VideoTrackEncoder

Comment 85

[Shelly Lin [:shelly]]

Comment on attachment 8336612 [details] [diff] [review]
Part2: Add VideoTrackEncoder

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

::: content/media/encoder/TrackEncoder.cpp
@@ +239,5 @@
> +  mReentrantMonitor.NotifyAll();
> +}
> +
> +void
> +VideoTrackEncoder::CreateMutedFrame(nsTArray<uint8_t>* aOutputBuffer)

As we discussed, using nsTArray<uint8_t> as output is good enough, but we'll see how it goes once we get to OmxTrackEncoder.

Comment 86

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

Comment on attachment 8336612 [details] [diff] [review]
Part2: Add VideoTrackEncoder

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

::: content/media/encoder/TrackEncoder.cpp
@@ +175,5 @@
> +    }
> +  }
> +
> +  if (mInitialized) {
> +    AppendVideoSegment(video);

Hmm. Does this mean if 'video' contains a null chunk followed by a real chunk, we'll add the duration of the null chunk to mNullDuration *and* append both chunks in AppendVideoSegment? I think so. We need to avoid that.

::: content/media/encoder/TrackEncoder.h
@@ +274,5 @@
> +   */
> +  TrackRate mTrackRate;
> +
> +  /**
> +   * The total duration of frames in TrackTicks, keeps track in its subclasses.

"The total duration of frames in encoded video in TrackTicks, kept track of in subclasses."

Comment 87

[Shelly Lin [:shelly]]

Comment on attachment 8336612 [details] [diff] [review]
Part2: Add VideoTrackEncoder

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

::: content/media/encoder/TrackEncoder.cpp
@@ +175,5 @@
> +    }
> +  }
> +
> +  if (mInitialized) {
> +    AppendVideoSegment(video);

Why do we need to avoid that? Shouldn't we want to keep the muted video? (If there's any before a real chunk.)

::: content/media/encoder/TrackEncoder.h
@@ +274,5 @@
> +   */
> +  TrackRate mTrackRate;
> +
> +  /**
> +   * The total duration of frames in TrackTicks, keeps track in its subclasses.

Thanks for correcting :)

Comment 88

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

(In reply to Shelly Lin [:shelly] from comment #87)
> Why do we need to avoid that? Shouldn't we want to keep the muted video? (If
> there's any before a real chunk.)

The null data will be appended twice. Once here: " mRawSegment->AppendNullData(mNullDuration);" and once in  "mRawSegment->AppendFrame(image.forget(), chunk.GetDuration(),
		chunk.mFrame.GetIntrinsicSize());"

Comment 89

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

Comment on attachment 8336505 [details] [diff] [review]
part3:MediaEncoder Change v3

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

::: content/media/DOMMediaStream.cpp
@@ +325,5 @@
>  DOMMediaStream::CheckTracksAvailable()
>  {
> +  if (mTrackTypesAvailable == 0) {
> +    return;
> +  }

Put this change in its own patch.

::: content/media/MediaRecorder.cpp
@@ +302,5 @@
> +    mRecorder->mStream->OnTracksAvailable(tracksAvailableCallback);
> +    // if there is no tracks, keep waiting
> +    uint8_t trackType = mRecorder->mStream->GetHintContents();
> +    if (!mRecorder->mStream->GetHintContents()) {
> +      mReentrantMonitor.Wait();

You can't call Wait; we're on the main thread.

Instead, put the rest of this method in its own method that is called by TracksAvailableCallback, so it runs as soon as the tracks are available.

@@ +316,5 @@
> +#endif
> +    }
> +    if (mimeType.IsEmpty()) {
> +      mimeType = NS_LITERAL_STRING(AUDIO_OGG);
> +    }

Put the code to select the right default MIME type in its own patch.

Comment 90

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

Attachment: part 3-1 mediastream change v1

Comment 91

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

Attachment: part 3-2 mediarecorder change v1

Comment 92

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

Attachment: part 3-3 mediaencoder change v1

Comment 93

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

Comment on attachment 8336993 [details] [diff] [review]
part 3-1 mediastream change v1

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

Please add a comment to DOMMediaStream.h explaining that when the expected tracks mask is 0, the callback will fire as soon as there are any tracks.

Comment 94

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

Comment on attachment 8336997 [details] [diff] [review]
part 3-2 mediarecorder change v1

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

::: content/media/MediaRecorder.cpp
@@ +283,3 @@
>    }
>  
> +  void AfterSetupStreams(uint8_t aTrackType)

call it aTrackTypes

@@ +289,5 @@
> +  #if defined(MOZ_WIDGET_GONK) && ANDROID_VERSION >= 17 && defined(MOZ_OMX_ENCODER)
> +      mimeType = NS_LITERAL_STRING(VIDEO_MP4);
> +  #endif
> +  #if defined(MOZ_VP8_ENCODER) && defined(MOZ_VORBIS_ENCODER)
> +      mimeType = NS_LITERAL_STRING(VIDEO_WEBM);

Shouldn't there be an #else here?

@@ +297,5 @@
> +      mimeType = NS_LITERAL_STRING(AUDIO_OGG);
> +    }
> +    // Allocate encoder and bind with union stream.
> +    mEncoder = MediaEncoder::CreateEncoder(mimeType, aTrackType);
> +

The way the type is chosen is a little confusing. The default choices made here may choose an encoder that's actually disabled by MediaDecoder::IsOggEnabled etc. Also, I don't see how the MIME type specified by the author is being respected.

I think it would be simpler if we don't do any MIME type selection here, just pass the MIME type specified by the author (if any) into CreateEncoder and do all MIME type decisions there.

Comment 95

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

Attachment: part 3-2 mediarecorder change v2

This API don't allow UA to set mimeType, So pass the mimeType as "" to encoder and let the encoder to decide proper mimeTpye by platform.

Comment 96

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

Attachment: part 3-1 mediastream change v2

Add commnet in content/media/DOMMediaStream.h

Comment 97

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

Comment on attachment 8337469 [details] [diff] [review]
part 3-1 mediastream change v2

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

::: content/media/DOMMediaStream.h
@@ +164,4 @@
>    // We only care about track additions, we'll fire the notification even if
>    // some of the tracks have been removed.
>    // Takes ownership of aCallback.
> +  // Run aCallback if track types have been added

This isn't enough. Use this:
  // If GetExpectedTracks() returns 0, the callback will be fired as soon as there are any tracks.

Comment 98

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

Comment on attachment 8337468 [details] [diff] [review]
part 3-2 mediarecorder change v2

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

In the commit message:
"framework"
"Start recording only after tracks have been added"

::: content/media/MediaRecorder.cpp
@@ +121,5 @@
> +    {
> +      mSession->AfterSetupStreams(aStream->GetHintContents());
> +    }
> +  private:
> +    Session *mSession;

I think you should make this an nsRefPtr<Session> to ensure it doesn't get destroyed while we're waiting for tracks.

@@ +280,5 @@
> +    TracksAvailableCallback* tracksAvailableCallback = new TracksAvailableCallback(mRecorder->mSession);
> +    mRecorder->mStream->OnTracksAvailable(tracksAvailableCallback);
> +  }
> +
> +  void AfterSetupStreams(uint8_t aTrackTypes)

Call this AfterTracksAdded

@@ +306,5 @@
> +    // shutdown notification and stop Read Thread.
> +    nsContentUtils::RegisterShutdownObserver(this);
> +
> +    mReadThread->Dispatch(new ExtractRunnable(this), NS_DISPATCH_NORMAL);
> +

Remove blank line

Comment 99

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

Comment on attachment 8336999 [details] [diff] [review]
part 3-3 mediaencoder change v1

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

::: content/media/encoder/MediaEncoder.cpp
@@ +6,5 @@
>  #include "MediaDecoder.h"
>  #include "nsIPrincipal.h"
> +#include "nsMimeTypes.h"
> +
> +#define WEBM_ENCODER defined(MOZ_VP8_ENCODER) && defined(MOZ_VORBIS_ENCODER)

I think you should write

#if defined(MOZ_VP8_ENCODER) && defined(MOZ_VORBIS_ENCODER)
#define WEBM_ENCODER
#endif

@@ +68,3 @@
>  
>    } else {
>      // Type video is not supported for now.

Remove this comment

@@ +86,4 @@
>  
>  /* static */
>  already_AddRefed<MediaEncoder>
> +MediaEncoder::CreateEncoder(const nsAString& aMIMEType, uint8_t aContentHinet)

aTrackTypes

@@ +119,5 @@
> +    writer = new WebMWriter(aContentHinet);  //TEST
> +#endif
> +  } else {
> +    NS_ASSERTION(false, "Can't support this mimeType");
> +    return nullptr;

The code that handles an empty MIME type has disappeared :-). It should be here. Probably the best way to write this is probably
  if (Ogg enabled and (Ogg MIME type or (no MIME type and no video track))) {
    ... generate Ogg and use Ogg MIME type ...
  } else if (OMX enabled and (MP4 MIME type or no MIME type)) {
    ... generate MP4 and use MP4 MIME type ...
  } else if (WebM enabled and (WebM MIME type or no MIME type)) {
    ... generate WebM and use WebM MIME type ...
  }

::: content/media/encoder/MediaEncoder.h
@@ +123,4 @@
>    }
>  
>  private:
> +  template<class T> nsresult WrtieEncodedDataToMuxer(T aEncoder);

WriteEncodedDataToMuxer

Comment 100

[Shelly Lin [:shelly]]

Attachment: Part1: Refactor of TrackEncoder

(Delete some deprecated attachments.)

Hi Roc, thanks for pointing out the problem in previous comment! I guess the |AppendAudioSegment| was skipping the null chunks before. Anyway, now that I think we don't need |mNullDuration| anymore, but need a flag to prevent re-setting EOS to codec-encoder in TrackEncdoer.

Comment 101

[Shelly Lin [:shelly]]

Attachment: Part1: Refactor of TrackEncoder

Update comment in |AudioTrackEncoder::NotifyEndOfStream()|.

Comment 102

[Shelly Lin [:shelly]]

Attachment: Part2: Add VideoTrackEncoder

Implementation of VideoTrackEncoder.

Comment 103

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

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #98)
> Comment on attachment 8337468 [details] [diff] [review]
> part 3-2 mediarecorder change v2
> 
> Review of attachment 8337468 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In the commit message:
> "framework"
> "Start recording only after tracks have been added"
> 
> ::: content/media/MediaRecorder.cpp
> @@ +121,5 @@
> > +    {
> > +      mSession->AfterSetupStreams(aStream->GetHintContents());
> > +    }
> > +  private:
> > +    Session *mSession;
> 
> I think you should make this an nsRefPtr<Session> to ensure it doesn't get
> destroyed while we're waiting for tracks.
oh...OK. I will modify this part at this patch...It seems not easy to change :(
> @@ +280,5 @@
> > +    TracksAvailableCallback* tracksAvailableCallback = new TracksAvailableCallback(mRecorder->mSession);
> > +    mRecorder->mStream->OnTracksAvailable(tracksAvailableCallback);
> > +  }
> > +
> > +  void AfterSetupStreams(uint8_t aTrackTypes)
> 
> Call this AfterTracksAdded
> 
> @@ +306,5 @@
> > +    // shutdown notification and stop Read Thread.
> > +    nsContentUtils::RegisterShutdownObserver(this);
> > +
> > +    mReadThread->Dispatch(new ExtractRunnable(this), NS_DISPATCH_NORMAL);
> > +
> 
> Remove blank line

Comment 104

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

Comment on attachment 8337556 [details] [diff] [review]
Part2: Add VideoTrackEncoder

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

::: content/media/encoder/TrackEncoder.h
@@ +287,5 @@
> +
> +  /**
> +   * A segment queue of audio track data, protected by mReentrantMonitor.
> +   */
> +  nsAutoPtr<VideoSegment> mRawSegment;

Actually ... this can just be a VideoSegment. It doesn't need to be heap-allocated.

Comment 105

[Shelly Lin [:shelly]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #104)
> Comment on attachment 8337556 [details] [diff] [review]
> Part2: Add VideoTrackEncoder
> 
> Review of attachment 8337556 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/encoder/TrackEncoder.h
> @@ +287,5 @@
> > +
> > +  /**
> > +   * A segment queue of audio track data, protected by mReentrantMonitor.
> > +   */
> > +  nsAutoPtr<VideoSegment> mRawSegment;
> 
> Actually ... this can just be a VideoSegment. It doesn't need to be
> heap-allocated.
Yeah, I thought so too, thanks!

Comment 106

[Shelly Lin [:shelly]]

Attachment: Part1: Refactor of TrackEncoder

Change in this rev:
- Use AudioSegment for mRawSegment in AudioTrackEncoder and AudioSegment for mSourceSegment in OpusTrackEncoder, for the purpose of avoiding head-allocation.

Comment 107

[Shelly Lin [:shelly]]

Attachment: Part1: Refactor of TrackEncoder

Removed an extra line of space.

Comment 108

[Shelly Lin [:shelly]]

Attachment: Part2: Add VideoTrackEncoder

Use VideoSegment for mRawSegment in VideoTrackEncoder, to avoid heap-allocation.

Comment 109

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

Attachment: part 3-2 mediarecorder change v3

Fix nits.

Comment 110

[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 111

[Shelly Lin [:shelly]]

Attachment: Part1: Refactor of TrackEncoder

Rebased to the change set of bug 935774.

Comment 112

[Shelly Lin [:shelly]]

Attachment: Part1: Refactor of TrackEncoder

Fix compiling error.

Comment 113

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

Attachment: part 3-3 mediaencoder change v2 .0

Comment 114

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

Attachment: part 3-3 mediaencoder change v2 .0

Fix conflict.

Comment 115

[Shelly Lin [:shelly]]

Attachment: Part1: Refactor of TrackEncoder

Fix memory leak problem on mochitest.

Comment 116

[Shelly Lin [:shelly]]

Attachment: Part2: Add VideoTrackEncoder

Comment 117

[Shelly Lin [:shelly]]

Attachment: Part2: Add VideoTrackEncoder

Comment 118

[Shelly Lin [:shelly]]

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

Comment 119

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

Attachment: iso_metadata patch

Let OMX encoder, muxer to reference.

Comment 120

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

Comment on attachment 8337679 [details] [diff] [review]
part 3-2 mediarecorder change v3

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

Fix typo in commit message ("framewrok")

Comment 121

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

Comment on attachment 8339191 [details] [diff] [review]
Part1: Refactor of TrackEncoder

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

::: content/media/encoder/OpusTrackEncoder.h
@@ +29,5 @@
>  class OpusTrackEncoder : public AudioTrackEncoder
>  {
>  public:
>    OpusTrackEncoder();
> +  ~OpusTrackEncoder();

This should still be declared virtual.

Comment 122

[Shelly Lin [:shelly]]

Attachment: Part1: Refactor of TrackEncoder

Thanks Roc :).

Comment 123

[Shelly Lin [:shelly]]

Comment on attachment 8340861 [details] [diff] [review]
Part1: Refactor of TrackEncoder

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

::: content/media/encoder/OpusTrackEncoder.cpp
@@ +140,5 @@
>    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
>    // This version of encoder API only support 1 or 2 channels,
>    // So set the mChannels less or equal 2 and
>    // let InterleaveTrackData downmix pcm data.
> +  mChannels = aChannels > MAX_CHANNELS ? MAX_CHANNELS : aChannels;

Although we will need to support more than two channels eventually, put the #define back instead of writing a magic number 2 here.

Comment 124

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

Comment on attachment 8339193 [details] [diff] [review]
Part2: Add VideoTrackEncoder

obsolete those patch and it will land on another bug.
Bug 945135 - [MediaEncoder] Refactor TrackEncoder and add VideoTrackEncoder

Comment 125

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

Comment on attachment 8340861 [details] [diff] [review]
Part1: Refactor of TrackEncoder

obsolete those patch and it will land on another bug.
Bug 945135 - [MediaEncoder] Refactor TrackEncoder and add VideoTrackEncoder

Comment 126

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

Hi Roc, 
I found if I registe onTracksAvailable, start after track available, and it would make media recorder become a async function. Should we add a onstart callback and change the whole test case?

Comment 127

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

Yes, I think so.

Comment 128

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

Attachment: part 3-3 mediaencoder change v3

New one for fix some problems on b2g platform.

Comment 129

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

Comment on attachment 8341601 [details] [diff] [review]
part 3-3 mediaencoder change v3

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

::: content/media/encoder/MediaEncoder.cpp
@@ +89,5 @@
> +#ifdef MOZ_OMX_ENCODER
> +    aMIMEType = NS_LITERAL_STRING(VIDEO_MP4);
> +#elif WEBM_ENCODER
> +    aMIMEType = NS_LITERAL_STRING(VIDEO_WEBM);
> +#endif

This is going back to the problem you had earlier in the patch history, where here you're not taking account of IsOggEnabled etc. I think you really need to do that. I think the approach in comment #99 is still the right approach.

@@ +251,5 @@
>        }
> +      LOG(PR_LOG_DEBUG, ("Video encoded TimeStamp = %f", GetEncodeTimeStamp()));
> +      bool isAudioCompleted = mAudioEncoder != nullptr ?
> +        mAudioEncoder->IsEncodingComplete() : true;
> +      bool isVideoCompleted = mVideoEncoder != nullptr ?

Write this as
  bool isAudioCompeted = mAudioEncoder && mAudioEncoder->IsEncodingComplete().

Basically, if "true" or "false" appear as one of the values in a ?: operator, something's wrong :-).

@@ +280,5 @@
>    }
>  }
>  
> +template<class T> nsresult
> +MediaEncoder::WrtieEncodedDataToMuxer(T aEncoder)

If aEncoder is a pointer, make it a T*.

@@ +290,5 @@
> +    return NS_OK;
> +  }
> +  EncodedFrameContainer encodedVideoData;
> +  nsresult rv = NS_OK;
> +  rv = aEncoder->GetEncodedTrack(encodedVideoData);

Just
  nsresult rv = aEncoder->GetEncodedTrack(...);

@@ +309,4 @@
>  }
> +
> +template<class T> nsresult
> +MediaEncoder::SetEncodedMetadataToMuxer(T aEncoder)

Call this CopyMetadataToMuxer?

::: content/media/encoder/MediaEncoder.h
@@ +132,5 @@
>      return mState == ENCODE_ERROR;
>    }
>  
>  private:
> +  template<class T> nsresult WrtieEncodedDataToMuxer(T aEncoder);

Fix typo in "Write"

@@ +133,5 @@
>    }
>  
>  private:
> +  template<class T> nsresult WrtieEncodedDataToMuxer(T aEncoder);
> +  template<class T> nsresult SetEncodedMetadataToMuxer(T aEncoder);

Please document what T is in these templates.

::: content/media/ogg/OggWriter.h
@@ +31,5 @@
>  
>    // Check metadata type integrity and reject unacceptable track encoder.
>    nsresult SetMetadata(TrackMetadataBase* aMetadata) MOZ_OVERRIDE;
>  
> +  bool IsWritingComplete() { return mIsWritingComplete; };

Shouldn't this be in ContainerWriter?

Comment 130

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

Attachment: part 3-3 mediaencoder change v4

Fix nits
1. Remove template (don't need it...)
2. Rename some function
3. add isXXXEnable in CreateEncoder function. 
BTW, should we add the preferences for all codecs ?
omx audio/video vp8/vorbis /mp4 muxer, webm muxer.

Comment 131

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

Comment on attachment 8342131 [details] [diff] [review]
part 3-3 mediaencoder change v4

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

::: content/media/encoder/MediaEncoder.cpp
@@ +34,1 @@
>  #endif

Add #undef LOG to the end of the file to help with unified builds.

@@ +90,5 @@
> +    aMIMEType = NS_LITERAL_STRING(VIDEO_MP4);
> +#elif WEBM_ENCODER
> +    aMIMEType = NS_LITERAL_STRING(VIDEO_WEBM);
> +#endif
> +    return;

The first part of my comment in comment #129 still applies. We shouldn't default to a type which is disabled.

Comment 132

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

For this part, should we check on both mimeType and create track encoder/writer? 
Right now I set check point on create track encoder/writer section.

Comment 134

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

Attachment: part 3-3 mediaencoder change v4 .1

1. Add undef LOG, (others file add near the header..)
2. for mimeType choose, 
The follow would be 
1. If client pass "" as mimeType into encoder, then call GetDefaultEncodeMIMEType, 
It would return platform supported mimeType based on tracktypes
We still try to enable audio track if the media stream contains video track.

2. create track/writer base on mimeType and build flag and perf enabled.
For WebM/MP4, should we add all of the codec/muxer perf items?

Comment 135

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

What's wrong with the code structure I suggested in comment #99?

Comment 136

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

Attachment: part 3-3 mediaencoder change v4 .2

follow by comment 99 suggestion. :-)

Comment 137

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

Hi roc, 
About the start method, the w3c autohr Jim said the start function would return when the encoder is ready to encoding.
So I think the start should be a synchronize function, is it right? 

But when I try to use the OnTracksAvailable to notify the encoder can start to record. I register the callback and I let the function just return without wait, As before I used mutex to wait the media stream ready and then return the function result. But you said we should not block the main thread. So I got confused with the Jim's response.

Comment 138

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

Attachment: part 3-3 mediaencoder change v4 .3

Fix try to record video frame only and notify wrong audio chunk to encoder issue.

Comment 139

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Randy Lin [:rlin] from comment #137)
> Hi roc, 
> About the start method, the w3c autohr Jim said the start function would
> return when the encoder is ready to encoding.
> So I think the start should be a synchronize function, is it right? 
> 
> But when I try to use the OnTracksAvailable to notify the encoder can start
> to record. I register the callback and I let the function just return
> without wait, As before I used mutex to wait the media stream ready and then
> return the function result. But you said we should not block the main
> thread. So I got confused with the Jim's response.

Where did jim say this?  Synchronous functions that aren't guaranteed to be fast are a problem from a "jank"/blocking-main-thread point-of-view.  People who work on Chrome tend to forget this, due to their multi-process architecture and UI split from content processes.

Comment 140

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

I don't believe start() is supposed to be synchronous. If it was, there wouldn't need to be an onstart event.

Comment 141

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

Comment on attachment 8344575 [details] [diff] [review]
part 3-3 mediaencoder change v4 .3

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

::: content/media/encoder/MediaEncoder.cpp
@@ +7,5 @@
>  #include "nsIPrincipal.h"
> +#include "nsMimeTypes.h"
> +#include "prlog.h"
> +
> +#define WEBM_ENCODER defined(MOZ_VP8_ENCODER) && defined(MOZ_VORBIS_ENCODER) && defined(MOZ_WEBM)

For consistency, WEBM_ENCODER should be either defined or undefined, instead of always being defined to 0 or 1.

@@ +102,3 @@
>    }
> +#if WEBM_ENCODER
> +  else if (MediaDecoder::IsWebMEnabled() && (mimeType.Equals(NS_LITERAL_STRING(VIDEO_WEBM)) ||

Use EqualsLiteral. But shouldn't you be checking aMIMEType instead of mimeType?

@@ +102,4 @@
>    }
> +#if WEBM_ENCODER
> +  else if (MediaDecoder::IsWebMEnabled() && (mimeType.Equals(NS_LITERAL_STRING(VIDEO_WEBM)) ||
> +           aTrackTypes & ContainerWriter::HAS_VIDEO)) {

Fix line breaking. You should have parentheses around the & subexpression.

@@ +114,5 @@
>    }
> +#endif //WEBM_ENCODER
> +#if MOZ_OMX_ENCODER
> +  else if (mimeType.Equals(NS_LITERAL_STRING(VIDEO_MP4)) ||
> +           aTrackTypes & ContainerWriter::HAS_VIDEO)) {

() around & subexpression

@@ +129,5 @@
> +#ifdef MOZ_OGG
> +  else if (MediaDecoder::IsOggEnabled() && (mimeType.Equals(NS_LITERAL_STRING(AUDIO_OGG)) ||
> +           aTrackTypes & ContainerWriter::HAS_AUDIO)) {
> +    writer = new OggWriter();
> +    if (MediaDecoder::IsOpusEnabled()) {

We should check IsOpusEnabled in the if condition above, instead of IsOggEnabled. (Actually we should probably check both.) No point in creating an empty OggWriter.

@@ +147,2 @@
>    encoder = new MediaEncoder(writer.forget(), audioEncoder.forget(),
> +                             videoEncoder.forget(), mimeType);

Need to set mimeType to the correct type in each case. Right now I think you're not setting it anywhere?

Comment 142

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

I start a discuss in media-capture called
"Re: Expect behavior of calling stop before receiving the onstart event on MediaRecorder API?"
And some people start to discuss this behavior. 
On the w3c spec, this sentence make me confuse.
==>
https://dvcs.w3.org/hg/dap/raw-file/tip/media-stream-capture/MediaRecorder.html 
start method:
If the state is not "inactive", raise a DOM InvalidState error and terminate these steps. Otherwise:
Set state to 'recording' and wait until media becomes available from stream.

Comment 143

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

Attachment: part 3-3 mediaencoder change v4 .4

Thanks roc's detail view..
Fix nits and mimeType issue :| , also let the build flag to be the same style.

Comment 144

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

Comment on attachment 8345164 [details] [diff] [review]
part 3-3 mediaencoder change v4 .4

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

r+ with those fixed.

::: content/media/encoder/MediaEncoder.cpp
@@ +100,3 @@
>    }
> +#ifdef MOZ_WEBM_ENCODER
> +  else if (MediaDecoder::IsWebMEnabled() &&

Remove "else" since there's a return in the previous if block.

@@ +100,5 @@
>    }
> +#ifdef MOZ_WEBM_ENCODER
> +  else if (MediaDecoder::IsWebMEnabled() &&
> +          (aMIMEType.EqualsLiteral(VIDEO_WEBM) ||
> +          (aTrackTypes & ContainerWriter::HAS_VIDEO))) {

Indent this last line one space so the two subexpressions of || are aligned

@@ +114,5 @@
> +  }
> +#endif //MOZ_WEBM_ENCODER
> +#ifdef MOZ_OMX_ENCODER
> +  else if (aMIMEType.EqualsLiteral(VIDEO_MP4) ||
> +          (aTrackTypes & ContainerWriter::HAS_VIDEO))) {

Same here.

You probably want to put the OMX case above the WEBM case so if we support both, there is no MIME type provided, and there is a video track, we default to OMX.

@@ +130,4 @@
>  #ifdef MOZ_OGG
> +  else if (MediaDecoder::IsOggEnabled() && MediaDecoder::IsOpusEnabled() &&
> +           (aMIMEType.EqualsLiteral(AUDIO_OGG) ||
> +           (aTrackTypes & ContainerWriter::HAS_AUDIO))) {

and here

@@ +148,2 @@
>    encoder = new MediaEncoder(writer.forget(), audioEncoder.forget(),
> +                             videoEncoder.forget(), mimeType);

I think we will need to do something to be more flexible with the MIME types we accept here. Although I guess right now we don't actually have an API to choose the MIME type, so we can fix that later.

@@ +229,5 @@
>        }
> +      LOG(PR_LOG_DEBUG, ("Video encoded TimeStamp = %f", GetEncodeTimeStamp()));
> +      // In audio only or video only case, let unavailable track's flag to be true.
> +      bool isAudioCompleted = (mAudioEncoder && mAudioEncoder->IsEncodingComplete()) || !mAudioEncoder;
> +      bool isVideoCompleted = (mVideoEncoder && mVideoEncoder->IsEncodingComplete()) || !mVideoEncoder;

Lines too long, break them.

Comment 145

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

Attachment: part 3-2 mediarecorder change v4

Fix crash problem.

Comment 146

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

Attachment: part 3-4 record_immediate_stop test case change v1

Comment 147

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

Attachment: part 3-4 record_immediate_stop test case change v1

found a typo...

Comment 149

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

Attachment: part 3-4 record_immediate_stop test case change v2

hmm, found OS X 10.8 Debug stop method run slower than others, so the encoder can start to encode and output blob with data. Remove the blob's mimeType or length check.

Comment 150

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

Attachment: part 3-4 record_immediate_stop test case change v3

Change to check the blob item case by case.

Comment 151

[Jason Smith [:jsmith]]

Comment on attachment 8348487 [details] [diff] [review]
part 3-4 record_immediate_stop test case change v3

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

review- mainly because one of the tests was removed & the size test isn't ensuring that the size isn't negative.

::: content/media/test/test_mediarecorder_record_immediate_stop.html
@@ +67,5 @@
>  
>        ok(evt instanceof BlobEvent,
>           'Events fired from ondataavailable should be BlobEvent');
>        is(evt.type, 'dataavailable',
>           'Event type should dataavailable');

Nit - insert a blank line here

@@ -71,5 @@
>           'Event type should dataavailable');
> -      ok(evt.data.size > 0,
> -         'Blob data received should be greater than zero');
> -      is(evt.data.type, expectedMimeType,
> -         'Blob data received should have type = ' + expectedMimeType);

Why is this removed?

@@ +73,5 @@
> +      // On some platforms, the stop method may run after media stream track
> +      // available, so the blob can contain the header data.
> +      if (evt.data.size > 0) {
> +        info(evt.data.size > 0,
> +           'Blob data received should be greater than zero');

I'd recommend changing this to:

info('Blob data size is greater than zero');

Given that you already know the blob size is greater than zero at this point.

@@ +76,5 @@
> +        info(evt.data.size > 0,
> +           'Blob data received should be greater than zero');
> +        is(evt.data.type, expectedMimeType,
> +           'Blob data received should have type = ' + expectedMimeType);
> +      } else {

An else statement won't work here, as this allows for negative numbers.

@@ +78,5 @@
> +        is(evt.data.type, expectedMimeType,
> +           'Blob data received should have type = ' + expectedMimeType);
> +      } else {
> +        info(evt.data.size == 0,
> +           'Blob data received should be zero');

I'd recommend changing this to:

info('Blob data size should be zero');

Given that you already know the blob size is zero at this point.

Comment 152

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

Comment on attachment 8348487 [details] [diff] [review]
part 3-4 record_immediate_stop test case change v3

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

::: content/media/test/test_mediarecorder_record_immediate_stop.html
@@ -71,5 @@
>           'Event type should dataavailable');
> -      ok(evt.data.size > 0,
> -         'Blob data received should be greater than zero');
> -      is(evt.data.type, expectedMimeType,
> -         'Blob data received should have type = ' + expectedMimeType);

Because in some platforms, the encoder haven't started and encoder should choose what kind of mimeType can be used for output blob.

Comment 153

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

Attachment: part 3-4 record_immediate_stop test case change v3 .1

Fix nits.

Comment 154

[Jason Smith [:jsmith]]

(In reply to Randy Lin [:rlin] from comment #152)
> Comment on attachment 8348487 [details] [diff] [review]
> part 3-4 record_immediate_stop test case change v3
> 
> Review of attachment 8348487 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/media/test/test_mediarecorder_record_immediate_stop.html
> @@ -71,5 @@
> >           'Event type should dataavailable');
> > -      ok(evt.data.size > 0,
> > -         'Blob data received should be greater than zero');
> > -      is(evt.data.type, expectedMimeType,
> > -         'Blob data received should have type = ' + expectedMimeType);
> 
> Because in some platforms, the encoder haven't started and encoder should
> choose what kind of mimeType can be used for output blob.

Right - but you need to handle this in that in your if statements respectively in the updated code. Meaning:

if (evt.data.size > 0) {
  is(evt.data.type, expectedMimeType, 'Blob data received..');
} else if (evt.data.size === 0) {
  is(evt.data.type, '', 'Blob data mime type is empty...');
}

Comment 155

[Jason Smith [:jsmith]]

Comment on attachment 8348495 [details] [diff] [review]
part 3-4 record_immediate_stop test case change v3 .1

Clearing review to address the comment above.

Comment 156

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

Attachment: part 3-4 record_immediate_stop test case change v3 .2

Hi Jason, the v3-1 already have the blob check as my comment.
But I still updated new one. Please help to review it, thanks.

Comment 157

[Jason Smith [:jsmith]]

Comment on attachment 8348847 [details] [diff] [review]
part 3-4 record_immediate_stop test case change v3 .2

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

review- - make sure you are testing evt.data.type & mediaRecorder.type in the if statement & else if statement for what you are expecting in those blocks respectively.

::: content/media/test/test_mediarecorder_record_immediate_stop.html
@@ +74,5 @@
> +      // On some platforms, the stop method may run after media stream track
> +      // available, so the blob can contain the header data.
> +      if (evt.data.size > 0) {
> +        is(evt.data.type, expectedMimeType,
> +           'Blob data received and should have mime type');

So within this if statement & the else if statement, I think you should be testing evt.data.type & mediaRecorder.type. Right now, you are only testing one of the two them.

@@ +78,5 @@
> +           'Blob data received and should have mime type');
> +      } else if (evt.data.size === 0) {
> +        is(mediaRecorder.mimeType, '',
> +           'Blob data mime type is empty');
> +      }

Nit - include an else statement here to report an error if the size is negative.

Comment 158

[Jason Smith [:jsmith]]

For clarity - here's what I'd expect the code to look like (ignoring the string messages):

if (evt.data.size > 0) {
  is(evt.data.type, expectedMimeType, '...');
  is(mediaRecorder.type, expectedMimeType, '...');
} else if (evt.data.size === 0) {
  is(evt.data.type, '', '...');
  is(mediaRecorder.type, '', '...');
} else {
  ok(false, '...')
}

Comment 159

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

oh, you reqeust to check the mediaRecorder.mimeType
hmm, I miss it.

Comment 160

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

Attachment: part 3-4 record_immediate_stop test case change v4

Comment 161

[Jason Smith [:jsmith]]

Comment on attachment 8349176 [details] [diff] [review]
part 3-4 record_immediate_stop test case change v4

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

review+ with the one fix included

::: content/media/test/test_mediarecorder_record_immediate_stop.html
@@ +83,5 @@
> +           'Blob data mime type is empty');
> +        is(mediaRecorder.mimeType, '',
> +           'Media Recorder mime type in ondataavailable is empty');
> +      } else {
> +        is(false, 'Blob size can not be negative');

This should be ok, not is.

Comment 162

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

Attachment: mediastream change, check-in patch p1

carry reviewer and fix comment.

Comment 163

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

Attachment: MediaEncoder change, check-in patch p2

carry reviewer.

Comment 164

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

Attachment: MediaRecoder change, check-in patch p3

check-in patch, carry reviewer.

Comment 165

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

Attachment: test case change, check-in patch p4

carry reviewer, check-in patch

Comment 166

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

try result
https://tbpl.mozilla.org/?tree=Try&rev=84380e85f9c6

Comment 167

[Ivan Tsay (:ITsay)]

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

Comment 168

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

Attachment: MediaRecoder change, check-in patch p3

Fix minor bug, should return nullptr when create encoder fail.

Comment 169

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/907dc12d8698
https://hg.mozilla.org/integration/b2g-inbound/rev/8517500c5728
https://hg.mozilla.org/integration/b2g-inbound/rev/055ade51ac29
https://hg.mozilla.org/integration/b2g-inbound/rev/10238ed91dd7

Comment 170

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/907dc12d8698
https://hg.mozilla.org/mozilla-central/rev/8517500c5728
https://hg.mozilla.org/mozilla-central/rev/055ade51ac29
https://hg.mozilla.org/mozilla-central/rev/10238ed91dd7

Comment 171

[Eric Shepherd [:sheppy]]

+ dev-doc-needed

Comment 172

[Eric Shepherd [:sheppy]]

(In reply to Eric Shepherd [:sheppy] from comment #171)
> + dev-doc-needed

For writers: The main thing here is the addition of video recording support to Firefox in Firefox 29. We need to update all pages that say this isn't implemented yet.