Support Video Encoder module in MediaEncoder framework

RESOLVED FIXED in Firefox 29

Status

()

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: u459114, Assigned: rlin)

Tracking

(Blocks 1 bug, {dev-doc-needed})

Trunk
1.3 C1/1.4 S1(20dec)
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:1.4+, firefox28 wontfix, firefox29 fixed, b2g-v1.4 fixed)

Details

(Whiteboard: [ft:multimedia-platform])

Attachments

(4 attachments, 71 obsolete attachments)

1.58 KB, patch
Details | Diff | Splinter Review
20.07 KB, patch
Details | Diff | Splinter Review
2.29 KB, patch
Details | Diff | Splinter Review
9.54 KB, patch
Details | Diff | Splinter Review
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)
Blocks: 879668
Blocks: 842243
Summary: Implement Video path in MediaEncoder/ MediaWriter → [MediaEncoder] Implement Video path in MediaEncoder/ MediaWriter
Blocks: MediaEncoder
No longer blocks: 842243
Blocks: 881840
Assignee: nobody → slin
Duplicate of this bug: 901277
No longer blocks: b2g-multimedia
No longer depends on: 919905
No longer blocks: 879668, 881840
Let it as meta bug
Assignee: slin → nobody
Summary: [MediaEncoder] Implement Video path in MediaEncoder/ MediaWriter → [Meta] Implement Video path in MediaEncoder
Alias: VideoEncoder
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.
Alias: VideoEncoder
Summary: [Meta] Implement Video path in MediaEncoder → Support Video Encoder module in MediaEncoder framewrok
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.
Flags: needinfo?(rlin)
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.
Assignee: nobody → rlin
Flags: needinfo?(rlin)
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;
                                                               ^^^^^^^^^^^^^^^^^^^
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.
(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?
Blocks: 923038
blocking-b2g: --- → 1.3+
Whiteboard: [ft:media-recording]
(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.
Does this parameter only required for vp8? 
Where can we get this parameter? from media stream?
No longer blocks: 923038
Target Milestone: --- → 1.3 Sprint 4 - 11/8
Posted patch WIP2 (obsolete) — Splinter Review
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
Attachment #811898 - Attachment is obsolete: true
The previous one was empty lol.
Attachment #824629 - Attachment is obsolete: true
Muxer needs to aware the the existence of A/V track during the init step.
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..
Posted patch WIP3 (obsolete) — Splinter Review
Remove the VideoTrackEncoder changed by slin.
Add TestWriter.h. for testing.
Attachment #823905 - Attachment is obsolete: true
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;
}
Things that implemented in this patch:
- Appending video frames from MediaStreamGraph.
- Deal with muted video chunks.

TODO:
- Documentation.
- Test in real world.
Attachment #825083 - Attachment is obsolete: true
This is actually Randy's patch too.
Update milestone to sprint 5 after discussing the implementation status with engineer
Target Milestone: 1.3 Sprint 4 - 11/8 → 1.3 Sprint 5 - 11/22
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 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
[WIP] Part3: A fake test of video track encoder.
The dummy muxer can write data into file system. (separate video/audio)
This is a testing patch, this would write A/V encoded data directly into file system.
Attachment #825730 - Attachment is obsolete: true
Attachment #826672 - Attachment is obsolete: true
Fix compiling error.
Attachment #827934 - Attachment is obsolete: true
Posted file TestWriter for dump TrackEncoder (obsolete) —
Attachment #827928 - Attachment is obsolete: true
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.
Major change:
In |AppendVideoSegment()|, use |mLastFrame| to hold the last different in-coming frame, and use |mLastFrameDuration| to accumulate duration of duplicated frames.
Attachment #827940 - Attachment is obsolete: true
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?
hmm. it should be. Let writer to decide this encoder session should be ended.
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 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.
Merge the same video chunk in VideoSegment.
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?
Flags: needinfo?(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.
Flags: needinfo?(bechen)
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.
Attachment #826671 - Attachment is obsolete: true
Attachment #829851 - Attachment is obsolete: true
With implementation of appending black frames if chunks are null.
Attachment #830570 - Attachment is obsolete: true
Attachment #830598 - Attachment is obsolete: true
Notify the wait of monitor when mRawSegment has appended some duration of chunks.
Attachment #830690 - Attachment is obsolete: true
(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 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 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)?
(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.
(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 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
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.
Attachment #830746 - Attachment is obsolete: true
Attachment #8333664 - Flags: review?(roc)
(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
Robert, please find my original comment in comment 53, this patch includes some nit fixes per Randy's comment.
Attachment #8333664 - Attachment is obsolete: true
Attachment #8333664 - Flags: review?(roc)
Attachment #8333669 - Flags: review?(roc)
(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.
Posted patch part2:MediaEncoder Change (obsolete) — Splinter Review
Attachment #830663 - Attachment is obsolete: true
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.
Attachment #8333669 - Flags: review?(roc) → review-
Posted patch Previous Part 1, for dev (obsolete) — Splinter Review
Attachment #8333669 - Attachment is obsolete: true
Attachment #8335006 - Flags: review?(roc)
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.
Posted patch Part2: Add VideoTrackEncoder (obsolete) — Splinter Review
Attachment #8335014 - Flags: review?(roc)
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 :-)
Attachment #8335006 - Flags: review?(roc) → review-
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 :-)
Attachment #8335014 - Flags: review?(roc) → review-
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.
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?
Flags: needinfo?(roc)
Replace video/mpv to video/m4v for video only mp4.
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.
Thanks, I will register the cb to media stream track's change and choose the proper of video mimeType.
Flags: needinfo?(roc)
Attachment #8335006 - Attachment is obsolete: true
Attachment #8335557 - Flags: review?(roc)
Attachment #8335557 - Attachment is obsolete: true
Attachment #8335557 - Flags: review?(roc)
Attachment #8335560 - Flags: review?(roc)
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 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 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.
Attachment #8335560 - Flags: review?(roc) → review+
- Added comment to the patch description.
- Nit fix.
Attachment #8335560 - Attachment is obsolete: true
Attachment #8336404 - Flags: review?(roc)
Attachment #8336404 - Attachment is obsolete: true
Attachment #8336404 - Flags: review?(roc)
Attachment #8336408 - Flags: review?(roc)
Posted patch Part2: Add VideoTrackEncoder (obsolete) — Splinter Review
Attachment #8335014 - Attachment is obsolete: true
Attachment #8336424 - Flags: review?(roc)
Posted patch part3:MediaEncoder Change v2 (obsolete) — Splinter Review
1. Get track information through TracksAvailableCallback.
2. because the error handle has been backout -.-..., this version is based on current mc version.
Attachment #8334440 - Attachment is obsolete: true
Attachment #8336427 - Flags: feedback?(roc)
Attachment #8336427 - Attachment description: part2:MediaEncoder Change v2 → part3:MediaEncoder Change v2
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.
Attachment #8336424 - Flags: review?(roc) → review-
Posted patch part3:MediaEncoder Change v3 (obsolete) — Splinter Review
Add build flags to decide which encoding format can be chosen.
Attachment #8336427 - Attachment is obsolete: true
Attachment #8336427 - Flags: feedback?(roc)
Attachment #8336505 - Flags: feedback?(roc)
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?
(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 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>?
Posted patch Part2: Add VideoTrackEncoder (obsolete) — Splinter Review
Attachment #8336424 - Attachment is obsolete: true
Attachment #8336612 - Flags: review?(roc)
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 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."
Attachment #8336612 - Flags: review?(roc) → review-
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 :)
Target Milestone: 1.3 Sprint 5 - 11/22 → 1.3 Sprint 6 - 12/6
Attachment #8336505 - Flags: feedback?(roc) → review?(roc)
(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 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.
Attachment #8336505 - Attachment is obsolete: true
Attachment #8336505 - Flags: review?(roc)
Attachment #8336993 - Flags: review?(roc)
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.
Attachment #8336993 - Flags: review?(roc) → review+
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.
Attachment #8336997 - Flags: review?(roc) → review-
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.
Attachment #8336997 - Attachment is obsolete: true
Attachment #8337468 - Flags: review?(roc)
Add commnet in content/media/DOMMediaStream.h
Attachment #8336993 - Attachment is obsolete: true
Attachment #8337469 - Flags: review?(roc)
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.
Attachment #8337469 - Flags: review?(roc) → review+
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
Attachment #8337468 - Flags: review?(roc) → review-
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
Attachment #8336999 - Flags: review?(roc) → review-
(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.
Attachment #828541 - Attachment is obsolete: true
Attachment #829852 - Attachment is obsolete: true
Attachment #830096 - Attachment is obsolete: true
Attachment #8334910 - Attachment is obsolete: true
Attachment #8336408 - Attachment is obsolete: true
Attachment #8337515 - Flags: review?(roc)
Update comment in |AudioTrackEncoder::NotifyEndOfStream()|.
Attachment #8337515 - Attachment is obsolete: true
Attachment #8337555 - Flags: review?(roc)
Posted patch Part2: Add VideoTrackEncoder (obsolete) — Splinter Review
Implementation of VideoTrackEncoder.
Attachment #8337556 - Flags: review?(roc)
(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 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.
Attachment #8337556 - Flags: review?(roc) → review+
(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!
Change in this rev:
- Use AudioSegment for mRawSegment in AudioTrackEncoder and AudioSegment for mSourceSegment in OpusTrackEncoder, for the purpose of avoiding head-allocation.
Attachment #8337555 - Attachment is obsolete: true
Attachment #8337644 - Flags: review?(roc)
Removed an extra line of space.
Attachment #8337644 - Attachment is obsolete: true
Attachment #8337644 - Flags: review?(roc)
Attachment #8337652 - Flags: review?(roc)
Posted patch Part2: Add VideoTrackEncoder (obsolete) — Splinter Review
Use VideoSegment for mRawSegment in VideoTrackEncoder, to avoid heap-allocation.
Attachment #8337556 - Attachment is obsolete: true
Attachment #8337653 - Flags: review?(roc)
Fix nits.
Attachment #8337468 - Attachment is obsolete: true
Attachment #8337679 - Flags: review?(roc)
Attachment #8336612 - Attachment is obsolete: true
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.
blocking-b2g: 1.3+ → -
Rebased to the change set of bug 935774.
Attachment #8337652 - Attachment is obsolete: true
Attachment #8337652 - Flags: review?(roc)
Attachment #8338320 - Flags: review?(roc)
Attachment #8337652 - Attachment is obsolete: false
Fix compiling error.
Attachment #8337652 - Attachment is obsolete: true
Attachment #8338320 - Attachment is obsolete: true
Attachment #8338320 - Flags: review?(roc)
Attachment #8338354 - Flags: review?(roc)
Attachment #8336999 - Attachment is obsolete: true
Fix conflict.
Attachment #8338982 - Attachment is obsolete: true
Fix memory leak problem on mochitest.
Attachment #8337653 - Attachment is obsolete: true
Attachment #8338354 - Attachment is obsolete: true
Attachment #8337653 - Flags: review?(roc)
Attachment #8338354 - Flags: review?(roc)
Attachment #8339191 - Flags: review?(roc)
Posted patch Part2: Add VideoTrackEncoder (obsolete) — Splinter Review
Attachment #8339192 - Flags: review?(roc)
Posted patch Part2: Add VideoTrackEncoder (obsolete) — Splinter Review
Attachment #8339192 - Attachment is obsolete: true
Attachment #8339192 - Flags: review?(roc)
Attachment #8339193 - Flags: review?(roc)
Posted patch iso_metadata patch (obsolete) — Splinter Review
Let OMX encoder, muxer to reference.
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")
Attachment #8337679 - Flags: review?(roc) → review+
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.
Attachment #8339191 - Flags: review?(roc) → review+
Thanks Roc :).
Attachment #8339191 - Attachment is obsolete: true
Attachment #8340861 - Flags: review?(roc)
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 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
Attachment #8339193 - Attachment is obsolete: true
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
Attachment #8340861 - Attachment is obsolete: true
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?
Flags: needinfo?(roc)
Yes, I think so.
Flags: needinfo?(roc)
Summary: Support Video Encoder module in MediaEncoder framewrok → Support Video Encoder module in MediaEncoder framework
New one for fix some problems on b2g platform.
Attachment #8339000 - Attachment is obsolete: true
Attachment #8341601 - Flags: review?(roc)
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?
Attachment #8341601 - Flags: review?(roc) → review-
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.
Attachment #8341601 - Attachment is obsolete: true
Attachment #8342131 - Flags: review?(roc)
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.
Attachment #8342131 - Flags: review?(roc) → review-
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.
Duplicate of this bug: 920934
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?
Attachment #8342131 - Attachment is obsolete: true
Attachment #8342273 - Flags: review?(roc)
What's wrong with the code structure I suggested in comment #99?
follow by comment 99 suggestion. :-)
Attachment #8342273 - Attachment is obsolete: true
Attachment #8342273 - Flags: review?(roc)
Attachment #8342827 - Flags: review?(roc)
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.
Flags: needinfo?(roc)
Fix try to record video frame only and notify wrong audio chunk to encoder issue.
Attachment #8342827 - Attachment is obsolete: true
Attachment #8342827 - Flags: review?(roc)
Attachment #8344575 - Flags: review?(roc)
(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.
I don't believe start() is supposed to be synchronous. If it was, there wouldn't need to be an onstart event.
Flags: needinfo?(roc)
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?
Attachment #8344575 - Flags: review?(roc) → review-
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.
Thanks roc's detail view..
Fix nits and mimeType issue :| , also let the build flag to be the same style.
Attachment #8344575 - Attachment is obsolete: true
Attachment #8345164 - Flags: review?(roc)
Whiteboard: [ft:media-recording] → [ft:multimedia-platform]
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.
Attachment #8345164 - Flags: review?(roc) → review+
Fix crash problem.
Attachment #8337679 - Attachment is obsolete: true
Attachment #8347691 - Flags: review?(roc)
Attachment #8347693 - Flags: review?(roc)
found a typo...
Attachment #8347693 - Attachment is obsolete: true
Attachment #8347693 - Flags: review?(roc)
Attachment #8347694 - Flags: review?(roc)
Duplicate of this bug: 949378
No longer depends on: 949378
Attachment #8347694 - Flags: review?(roc)
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.
Attachment #8347694 - Attachment is obsolete: true
Attachment #8347882 - Flags: review?(roc)
Target Milestone: 1.3 Sprint 6 - 12/6 → 1.3 C1/1.4 S1(20dec)
Change to check the blob item case by case.
Attachment #8347882 - Attachment is obsolete: true
Attachment #8347882 - Flags: review?(roc)
Attachment #8348487 - Flags: review?(roc)
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.
Attachment #8348487 - Flags: review-
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.
Fix nits.
Attachment #8348487 - Attachment is obsolete: true
Attachment #8348495 - Flags: review?(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 on attachment 8348495 [details] [diff] [review]
part 3-4 record_immediate_stop test case change v3 .1

Clearing review to address the comment above.
Attachment #8348495 - Flags: review?(jsmith)
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.
Attachment #8348495 - Attachment is obsolete: true
Attachment #8348847 - Flags: review?(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.
Attachment #8348847 - Flags: review?(jsmith) → review-
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, '...')
}
oh, you reqeust to check the mediaRecorder.mimeType
hmm, I miss it.
Attachment #8348847 - Attachment is obsolete: true
Attachment #8349176 - Flags: review?(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.
Attachment #8349176 - Flags: review?(jsmith) → review+
carry reviewer and fix comment.
check-in patch, carry reviewer.
carry reviewer, check-in patch
Plus it for v1.4 because it is the committed feature work
blocking-b2g: - → 1.4+
Fix minor bug, should return nullptr when create encoder fail.
Attachment #8349277 - Attachment is obsolete: true
Attachment #8349176 - Attachment is obsolete: true
Attachment #8337469 - Attachment is obsolete: true
Attachment #8340201 - Attachment is obsolete: true
Attachment #8340201 - Attachment is patch: true
Attachment #8345164 - Attachment is obsolete: true
Attachment #8347691 - Attachment is obsolete: true
Keywords: verifyme
Depends on: 952052
No longer blocks: 920934
Component: Video/Audio → Video/Audio: Recording
Keywords: verifyme
+ dev-doc-needed
Keywords: dev-doc-needed
(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.
You need to log in before you can comment on or make changes to this bug.