881840 - [MediaEncoder] Implement VP8 video track encoder

Status: RESOLVED FIXED

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

Description

[u459114]

Software encoding path, which should be work on all platform

Comment 1

[Benjamin Chen [:bechen]]

http://www.webmproject.org/docs/vp8-sdk/example__simple__encoder.html

Reference sample code.

Comment 2

[Benjamin Chen [:bechen]]

Attachment: WIP- VP8TrackEncoder

This is a WIP patch for VP8TrackEncoder:

1. Add VP8TrackEncoder files
- |VP8TrackEncoder::Init| init the VP8 encoder reference by webRTC
- |VP8TrackEncoder::GetHeader| Get the IVF header
- |VP8TrackEncoder::GetEncodedTrack| Get the IVF frame header and VP8 encoded data

2. Modify MediaEncoder and TrackEncoder for testing VP8 encode
- Mark some code to tes video encoder path only
- Basic VideoTrackEncoder implementation

Current flow:
1. Create VP8TrackEncoder and set the framerate in constructor
2. The data comes from MediaStreamGraph and though |VideoTrackEncoder::NotifyQueuedTrackChanges| send into VP8TrackEncoder
3.By calling |rackEncoder::GetEncodedTrack| to get the encoded data

#define VP8DUMPTOFILE
This flag dump the encoded file to "/system/b2g/vp8test" in IVF format that can played by vlc.

Some problem:
1. the incoming video is 640x480 and the configuration is reference by webRTC
- keyframe
- target bitrate

2. in |VP8TrackEncoder::GetEncodedTrack|, we should process one output frame or the whole data queued in input |mSourceSegment|
- If we process only one output frame, the input queue will accumulate rapidly.
- If we process the all data in intput queue, the system is extremely slow.

Comment 3

[Benjamin Chen [:bechen]]

Attachment: allow-gUM-video.patch

patch provided by Shelly to skip permission check.

Comment 4

[Benjamin Chen [:bechen]]

Attachment: video_test.tar

testing app provided by Shelly.

Comment 5

[Benjamin Chen [:bechen]]

Attachment: WIP-vp8encoder.v02.patch

The function VP8TrackEncoder::GetEncodedTrack:

1. At first of the function, we check the |mInitialized| to see if the encoder is initialized and also ensure the data append in mRawSegment and mSourceSegment are enough for one output frame duration.
ex: mFrameRate(FPS) = 10, the total frame duration in mRawSegment and mSourceSegment must larger the 0.1 second.

2. We append the mRawSegment to mSourceSegment then go into a while loop to find the "correct sample frame". The while loop will consume all frames in mSourceSegment.
ex: Assume mFrameRate = 1, the total frame duration mSourceSegment is 2 second
and each frame duration is fixed to 0.1s. So there are about 20 raw frames in mSourceSegment. The "correct sample frame" should be the 10th and 20th frame.

3. After step 2, we pick a frame then send into VP8 encoder.
The struct |encoded_image_| is a input buffer for encoder and also output buffer. There is a color convertion from NV21 to I420 before encode.

4. After encode one frame, at the end of while loop, we use a heuristic to set |mSkipNextFrame| flag to skip some frames.
ex: Assume FPS = 1, the total duration of mSourceSegment is 5 second.
Ideally we should output 5 encoded frames and the process time of these 5 frames must faster than 5 second. Or the mSourceSegment will accumulate infinitely.
In order to avoid this situation, we calculate the process time of each frame and then set the flag mSkipNextFrame if neccessary. Once the flag mSkipNextFrame is set, the next iteration won't encode any frame.

Comment 6

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

Comment on attachment 803481 [details] [diff] [review]
WIP-vp8encoder.v02.patch

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

::: content/media/encoder/VP8TrackEncoder.cpp
@@ +12,5 @@
> +
> +#define VP8DUMPTOFILE
> +
> +namespace mozilla {
> +#define LOG1(args...)  __android_log_print(ANDROID_LOG_INFO, "bentest", args)

Please have build flag for this android log.

Comment 7

[u459114]

Hi Benjamin,
I think what you did in TrackEncoder is a generic term, very video track encoder need this base implementation. Please move this code to bug 879669.

Comment 8

[Benjamin Chen [:bechen]]

Attachment: bug-881840-vp8encoder.v02.patch

Comment 9

[Benjamin Chen [:bechen]]

Comment on attachment 817708 [details] [diff] [review]
bug-881840-vp8encoder.v02.patch

Hi :derf 
would you please give me some feedback about the init configuration of vp8 encoder? codes in |VP8TrackEncoder::Init|.

Ex: For real-time media encoder, the target platform are desktop build and b2g device. There might need at least 2 different parameter sets for desktop and mobile devices.

1. How to set the bitrate?
2. constant bitrate or variable bitrate?
3. cpu speed? |vpx_codec_control(&mVP8, VP8E_SET_CPUUSED, mCPUSpeed);|
...

And there is another question:
When I call the encode function
|vpx_codec_encode(&mVP8, mVPXImageWrapper,
            mEncodedTimestamp, outputDuration, flags, VPX_DL_REALTIME)| with the VPX_DL_REALTIME flag.
What happened if the encoder exceed the duration |outputDuration|?
And then I call |vpx_codec_get_cx_data|, sometimes it returns previous encoded frame with previous timestamp. Should I output the duplicated encoded frame to WebM container?

Comment 10

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

Hi Benjamin,
Could you rebase vp8 track encoder to latest mc version?

Comment 11

[Ivan Tsay (:ITsay)]

Per discussion with engineers, this one is mainly for the desktop version instead of B2G, no need to be tracked in b2g. Remove blocking-b2g flag and blocking on user story.

Comment 12

[Benjamin Chen [:bechen]]

Attachment: bug-881840-vp8encoder.v03.patch

Comment 13

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

Comment on attachment 827906 [details] [diff] [review]
bug-881840-vp8encoder.v03.patch

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

Some comment on header files. :)

::: content/media/encoder/VP8TrackEncoder.h
@@ +8,5 @@
> +
> +#include "TrackEncoder.h"
> +#include "vpx/vpx_codec.h"
> +#include "vpx/vpx_encoder.h"
> +#include <stdio.h>

add line

@@ +17,5 @@
> +typedef struct vpx_codec_enc_cfg vpx_codec_enc_cfg_t;
> +typedef struct vpx_image vpx_image_t;
> +typedef struct vpx_ref_frame vpx_ref_frame_t;
> +struct vpx_codec_cx_pkt;
> +

document this.

@@ +49,5 @@
> +  int32_t mHeight;
> +  int32_t mEncodedFrameRate;
> +  MetadataKind GetKind() const MOZ_OVERRIDE { return METADATA_VP8; }
> +};
> +

document this. thread model, use library or flag, apply to which platform, encoding flow, input/output format.

@@ +65,5 @@
> +  nsresult Init(int32_t aWidth, int32_t aHeight,
> +                TrackRate aTrackRate) MOZ_FINAL MOZ_OVERRIDE;
> +
> +private:
> +

remove this line

@@ +70,5 @@
> +  // Get the encoded data from encoder
> +  nsresult GetEncodedPartitions(uint64_t aTimestamp,
> +                                EncodedFrameContainer& aData);
> +
> +  // Prepare the input data for encoder, fill the mImageBuffer

Could you describe the processing step more detail?

@@ +75,5 @@
> +  nsresult PrepareEncodedFrame(VideoChunk &aChunk);
> +
> +  // target output frame rate
> +  uint32_t mEncodedFrameRate;
> +  uint32_t mDuration;

Is this means one video frame's duration?

@@ +77,5 @@
> +  // target output frame rate
> +  uint32_t mEncodedFrameRate;
> +  uint32_t mDuration;
> +  uint64_t mEncodedTimestamp;
> +  uint64_t mNextFrameTimeTick;

What's this purpose?

@@ +80,5 @@
> +  uint64_t mEncodedTimestamp;
> +  uint64_t mNextFrameTimeTick;
> +  bool mSkipNextFrame;
> +  /**
> +   * A local segment queue which stores the raw segments.

Is this used to store the VideoSegment from rawSegment?

@@ +84,5 @@
> +   * A local segment queue which stores the raw segments.
> +   */
> +  nsAutoPtr<VideoSegment> mSourceSegment;
> +
> +  // vp8

VP8 encoder specific data structure? Maybe have a url link to reference those usage, or directly document this.

@@ +90,5 @@
> +  vpx_codec_enc_cfg_t mConfig;
> +  vpx_image_t* mVPXImageWrapper;
> +  int32_t mCPUSpeed;
> +
> +  // Input/output buffer for VP8 encoder.

When does the input buffer do the memcopy on desktop version?

Comment 14

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

Comment on attachment 827906 [details] [diff] [review]
bug-881840-vp8encoder.v03.patch

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

::: content/media/encoder/VP8TrackEncoder.cpp
@@ +111,5 @@
> +  // set the maximum target size of any key-frame.
> +  //rc_max_intra_target_ = MaxIntraTarget(config_->rc_buf_optimal_sz);
> +
> +  mConfig.kf_mode = VPX_KF_AUTO;
> +  mConfig.kf_max_dist = 1000;

Please set this value less than 30, I may need to have one key frame every 1sec...

Comment 15

[u459114]

Comment on attachment 827906 [details] [diff] [review]
bug-881840-vp8encoder.v03.patch

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

::: content/media/encoder/VP8TrackEncoder.cpp
@@ +247,5 @@
> +        dst_v[half_width - 1] = src_uv[1];
> +      }
> +      dst_u += stride_uv;
> +      dst_v += stride_uv;
> +    }

SIMD improvement here? please discuss with JW

::: content/media/encoder/VP8TrackEncoder.h
@@ +27,5 @@
> +  , mBuffer(nullptr)
> +  , mLength(0)
> +  , mSize(0) {}
> +
> +  virtual ~ImageBuffer()

No need to be virtual

@@ +35,5 @@
> +      mBuffer = nullptr;
> +    }
> +  }
> +  EncodedFrame::FrameType mFrameType;
> +  uint8_t* mBuffer;

Why not use nsTArray here. You don't need mLength at all if you use vector here

@@ +38,5 @@
> +  EncodedFrame::FrameType mFrameType;
> +  uint8_t* mBuffer;
> +  uint32_t mLength;
> +  uint32_t mSize;
> +};

Predeclare class ImageBuffer here. Move class definition to cpp. 
In VP8TrackEncoder, change mImageBuffer from object to smart pointer
nsAutoPtr<ImageBuffer> mImageBuffer;

Comment 16

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

Hi Benjamin, 
Please merge with shelly patch and make it build pass, thanks a lot.

Comment 17

[Benjamin Chen [:bechen]]

Attachment: bug-881840-vp8encoder.v04.patch

Sync to latest codebase.
1. add comments.
2. handle mute frames.
3. add |MergeSameVideoChunk| in videoSegment
4. refine the |GetEncodedTrack|.
5. |mConfig.kf_max_dist = mEncodedFrameRate| to ensure that we will output 1 i-frame per second.

Comment 18

[u459114]

Comment on attachment 8341569 [details] [diff] [review]
bug-881840-vp8encoder.v04.patch

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

::: content/media/encoder/VP8TrackEncoder.cpp
@@ +160,5 @@
> +  mCPUSpeed = -12;
> +#endif
> +
> +  vpx_codec_control(&mVP8, VP8E_SET_STATIC_THRESHOLD, 1);
> +  vpx_codec_control(&mVP8, VP8E_SET_CPUUSED, mCPUSpeed);

#if defined(WEBRTC_ARCH_ARM)
// On mobile platform, always set to -12 to leverage between cpu usage
// and video quality
const VPX_CPUUSED = -12;
#else
const VPX_CPUUSED = -6;
#endif

vpx_codec_control(&mVP8, VP8E_SET_CPUUSED, VPX_CPUUSED);

And remove mCPUSpeed member

@@ +247,5 @@
> +nsresult VP8TrackEncoder::PrepareEncodedFrame(VideoChunk &aChunk)
> +{
> +  if (aChunk.mFrame.GetForceBlack()) {
> +    if (!mIsMuteFrameCreated) {
> +      CreateMutedFrame(&mMuteFrame);

You can just check Length of mMutedFrame to tell whether you had create dummy frame. Remove mIsMuteFrameCreated

::: content/media/encoder/VP8TrackEncoder.h
@@ +45,5 @@
> +
> +/**
> + * VP8TrackEncoder implements VideoTrackEncoder by using libvpx library.
> + *
> + * Encoding flow in GetEncodedTrack():

Move comments here to the definition of VP8TrackEncoder::GetEncodedTrack()

@@ +120,5 @@
> +  // EncodeState for next iteration.
> +  EncodeState mNextFrameEncodeState;
> +
> +  // Mute frame flag, we only create it once.
> +  bool mIsMuteFrameCreated;

Remove

@@ +128,5 @@
> +  nsAutoPtr<VideoSegment> mSourceSegment;
> +
> +  // VP8 relative members.
> +  // Codec context structure.
> +  vpx_codec_ctx_t mVP8;

Rename to mVPXContext

@@ +130,5 @@
> +  // VP8 relative members.
> +  // Codec context structure.
> +  vpx_codec_ctx_t mVP8;
> +  // Encoder configuration structure.
> +  vpx_codec_enc_cfg_t mConfig;

It can be a local variable in VP8TrackEncoder::Init. You don't need a object data to keep this configuration

@@ +133,5 @@
> +  // Encoder configuration structure.
> +  vpx_codec_enc_cfg_t mConfig;
> +  // Image Descriptor.
> +  vpx_image_t* mVPXImageWrapper;
> +  int32_t mCPUSpeed;

Remove this member data

Comment 19

[u459114]

Comment on attachment 8341569 [details] [diff] [review]
bug-881840-vp8encoder.v04.patch

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

::: content/media/encoder/VP8TrackEncoder.cpp
@@ +165,5 @@
> +  vpx_codec_control(&mVP8, VP8E_SET_TOKEN_PARTITIONS, VP8_ONE_TOKENPARTITION);
> +
> +  mInitialized = true;
> +  mon.NotifyAll();
> +

Only NotifyAll() in success case?

@@ +412,5 @@
> +  mNextFrameEncodeState = NORMAL_FRAME;
> +
> +  while (!iter.IsEnded()) {
> +    // find the videoChunk by mNextFrameDuration
> +    VideoChunk chunk = *iter;

VideoChunk &chunk = *iter;

::: content/media/encoder/VP8TrackEncoder.h
@@ +118,5 @@
> +  // Duration to the next encode frame. The range is 0 ~ mDuration.
> +  uint64_t mNextFrameDuration;
> +  // EncodeState for next iteration.
> +  EncodeState mNextFrameEncodeState;
> +

Remove this data. It can be a local varaiable

Comment 20

[u459114]

Comment on attachment 8341569 [details] [diff] [review]
bug-881840-vp8encoder.v04.patch

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

::: content/media/encoder/VP8TrackEncoder.cpp
@@ +38,5 @@
> +      delete [] mBuffer;
> +      mBuffer = nullptr;
> +    }
> +  }
> +

Make all data be private. Remove mLength.

Add the following public functions
bool Allocate(uint32_t aWidth, uint32_t aHeight)
{
  if (aWidth == 0 or aHeight ==0)
  {
    MOZ_ASSERT(false);
    return false;
  }
  
  // Explain the reason for this size.(uv plan)
  int half_width = (aWidth + 1) >> 1;
  int half_height = (aHeight + 1) >> 1;
  mSize = aWidth * aHeight + half_width * half_height * 2;
  mBuffer = new uint8_t[mSize];
 
  return true;
}

uint32_t GetSize() const  { return mSize; }
uint8_t *GetBuffer() const { return mBuffer;}

@@ +93,5 @@
> +
> +  int half_width = (mFrameWidth + 1) >> 1;
> +  int half_height = (mFrameHeight + 1) >> 1;
> +  mImageBuffer->mSize = mFrameWidth * mFrameHeight +
> +                       half_width * half_height * 2;

Why mImageBuffer->mSize need to take extra "half_width * half_height * 2" bytes?
Please write comment here.

@@ +201,5 @@
> +{
> +  vpx_codec_iter_t iter = nullptr;
> +  mImageBuffer->mLength = 0;
> +  mImageBuffer->mFrameType = EncodedFrame::P_FRAME;
> +

You don't really need these two member data, ImageBuffer::mLength/ mFrameType.
These data can be a local variable

@@ +228,5 @@
> +  }
> +
> +  if (mImageBuffer->mLength && ((uint64_t)pkt->data.frame.pts == aTimestamp)) {
> +    // Copy the encoded data to aOutput.
> +    EncodedFrame* videodata = new EncodedFrame();

videoData

@@ +232,5 @@
> +    EncodedFrame* videodata = new EncodedFrame();
> +    videodata->SetFrameType(mImageBuffer->mFrameType);
> +    nsTArray<uint8_t> frameData;
> +    frameData.SetLength(mImageBuffer->mLength);
> +    memcpy(frameData.Elements(), mImageBuffer->mBuffer, mImageBuffer->mLength);

This buffer copy can be prevent if you assign this buffer, frameData, into mImageBuffer at the begin of this function

@@ +251,5 @@
> +      CreateMutedFrame(&mMuteFrame);
> +      mIsMuteFrameCreated = true;
> +    }
> +
> +    int size_y = mFrameWidth * mFrameHeight;

rename to yPlanSize

@@ +252,5 @@
> +      mIsMuteFrameCreated = true;
> +    }
> +
> +    int size_y = mFrameWidth * mFrameHeight;
> +    int half_width = (mFrameWidth + 1) / 2;

halfWith

@@ +253,5 @@
> +    }
> +
> +    int size_y = mFrameWidth * mFrameHeight;
> +    int half_width = (mFrameWidth + 1) / 2;
> +    int half_height = (mFrameHeight + 1) / 2;

halfHeight

@@ +254,5 @@
> +
> +    int size_y = mFrameWidth * mFrameHeight;
> +    int half_width = (mFrameWidth + 1) / 2;
> +    int half_height = (mFrameHeight + 1) / 2;
> +    int size_uv = half_width * half_height;

rename to uvPlanSize

Comment 21

[u459114]

Comment on attachment 8341569 [details] [diff] [review]
bug-881840-vp8encoder.v04.patch

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

::: content/media/encoder/VP8TrackEncoder.cpp
@@ +460,5 @@
> +      }
> +    } else {
> +      // Keep accumulate chunk's duration until over mNextFrameDuration.
> +      iter.Next();
> +    }

Remove line #455, 461~464.

Change while loop to for loop
for (; !iter.IsEnd(); iter.Next())

@@ +468,5 @@
> +
> +  // End of stream, pull the rest frames in encoder
> +  if (mEndOfStream && !mEosSetInEncoder) {
> +    mEncodingComplete = true;
> +    mEosSetInEncoder = true;

Why you need mEosSetInEncoder? There is no code refer to this data.

@@ +471,5 @@
> +    mEncodingComplete = true;
> +    mEosSetInEncoder = true;
> +    int flags = 0;
> +    if (vpx_codec_encode(&mVP8, nullptr, mEncodedTimestamp, mDuration, flags,
> +                         VPX_DL_REALTIME)) {

flags variable is not need here, use "0" literal direct

Comment 22

[u459114]

Comment on attachment 8341569 [details] [diff] [review]
bug-881840-vp8encoder.v04.patch

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

::: content/media/encoder/VP8TrackEncoder.cpp
@@ +367,5 @@
> +    return NORMAL_FRAME;
> +  }
> +}
> +
> +int

Return uint64_t

@@ +405,5 @@
> +
> +  mSourceSegment->MergeSameVideoChunk();
> +
> +  VideoSegment::ChunkIterator iter(*mSourceSegment);
> +  uint64_t frameCopied = 0;

Rename to proceededFrameDuration?

@@ +475,5 @@
> +                         VPX_DL_REALTIME)) {
> +      return NS_ERROR_FAILURE;
> +    }
> +    GetEncodedPartitions(mEncodedTimestamp, aData);
> +    mEncodedTimestamp += mDuration;

You don't need to set mEncodedTimestamp here, since EOS.
Instead, you might need to clear all data member into initial state.

::: content/media/encoder/VP8TrackEncoder.h
@@ +41,5 @@
> +  NORMAL_FRAME,
> +  I_FRAME,
> +  SKIP_FRAME
> +};
> +

Define this enum in VP8TrackEncoder

@@ +111,5 @@
> +
> +  // Output frame rate.
> +  uint32_t mEncodedFrameRate;
> +  // Duration for the output frame, reciprocal to mEncodedFrameRate.
> +  uint32_t mDuration;

Rename to mFrameDuration

@@ +115,5 @@
> +  uint32_t mDuration;
> +  // Encoded timestamp.
> +  uint64_t mEncodedTimestamp;
> +  // Duration to the next encode frame. The range is 0 ~ mDuration.
> +  uint64_t mNextFrameDuration;

Should be a local variable in GetEncodedTrack

Comment 23

[u459114]

Comment on attachment 8341569 [details] [diff] [review]
bug-881840-vp8encoder.v04.patch

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

::: content/media/encoder/VP8TrackEncoder.cpp
@@ +425,5 @@
> +
> +      // Calculate encodedDuration and mNextFrameDuration for next iteration.
> +      int encodedDuration =
> +        CalculateEncodedDurationAndNextFrameDuration(frameCopied);
> +

Move line #427/428 to #421.
Combine three "if statements"(#420/ #431/ #442) into one

@@ +439,5 @@
> +
> +      // Get the encoded frame from VP8 encoder if the mNextFrameEncodeState is
> +      // not SKIP_FRAME.
> +      if (mNextFrameEncodeState != SKIP_FRAME) {
> +        GetEncodedPartitions(mEncodedTimestamp, aData);

Move line #443 to #438, and remove if statement at #442

Comment 24

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

Attachment: WebMWriter.h

The VP8Metadata or VorbisMetadata have move to this include file, 
Please refer this patch.
https://bug891705.bugzilla.mozilla.org/attachment.cgi?id=8342239
Thanks.

Comment 25

[Benjamin Chen [:bechen]]

Attachment: bug-881840-vp8encoder.v05.patch

Address most comments from :CJKu.
1. remove MergeSameVideoChunk for now.
2. change the while-loop to for-loop

Comment 26

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

Comment on attachment 8347021 [details] [diff] [review]
bug-881840-vp8encoder.v05.patch

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

::: content/media/encoder/VP8TrackEncoder.cpp
@@ +9,5 @@
> +#include "VideoUtils.h"
> +#ifdef MOZ_WIDGET_GONK
> +#include "GrallocImages.h"
> +#endif
> +

Remove this, also use NSPR_LOG to logging all of the important check pointer.

@@ +20,5 @@
> + * A buffer for VP8's input and output frame.
> + * mBuffer: buffer pointer.
> + * mSize: buffer size.
> + * */
> +class ImageBuffer

Do we really need that? It seems used to copy buffer from vpx to this object and copy to FrameData.

@@ +41,5 @@
> +  {
> +    if (aWidth <= 0 || aHeight <= 0)
> +    {
> +      MOZ_ASSERT(false);
> +      return false;

Does this return value be handled?

@@ +45,5 @@
> +      return false;
> +    }
> +
> +    int halfWidth = (aWidth + 1) / 2;
> +    int halfHeight = (aHeight + 1) / 2;

The odd size of image should handle by Bug 948815 - [MediaEncoder] Handle odd size video resolutions.

@@ +56,5 @@
> +  uint32_t GetSize() const  { return mSize; }
> +  uint8_t *GetBuffer() const { return mBuffer; }
> +
> +private:
> +  uint8_t* mBuffer;

nsTArray? and remove the mSize

@@ +94,5 @@
> +  ReentrantMonitorAutoEnter mon(mReentrantMonitor);
> +
> +  mTrackRate = aTrackRate;
> +  mEncodedFrameRate = DEFAULT_ENCODE_FRAMERATE;
> +  mEncodedFrameDuration = mTrackRate / mEncodedFrameRate;

Calculation deviation?

@@ +140,5 @@
> +    config.g_threads = 1;  // 1 thread for QVGA.
> +  }
> +
> +  // rate control settings
> +  config.rc_dropframe_thresh = 0;//inst->codecSpecific.VP8.frameDroppingOn ?30 : 0;

What is the comment for? //inst->codecSpecific

@@ +143,5 @@
> +  // rate control settings
> +  config.rc_dropframe_thresh = 0;//inst->codecSpecific.VP8.frameDroppingOn ?30 : 0;
> +  config.rc_end_usage = VPX_CBR;
> +  config.g_pass = VPX_RC_ONE_PASS;
> +  config.rc_resize_allowed = 1;//inst->codecSpecific.VP8.automaticResizeOn ?1 : 0;

The same.

@@ +145,5 @@
> +  config.rc_end_usage = VPX_CBR;
> +  config.g_pass = VPX_RC_ONE_PASS;
> +  config.rc_resize_allowed = 1;//inst->codecSpecific.VP8.automaticResizeOn ?1 : 0;
> +  //config.rc_min_quantizer = 2;
> +  //config.rc_max_quantizer = inst->qpMax;

Comment for what reason?

@@ +150,5 @@
> +  config.rc_undershoot_pct = 100;
> +  config.rc_overshoot_pct = 15;
> +  config.rc_buf_initial_sz = 500;
> +  config.rc_buf_optimal_sz = 600;
> +  config.rc_buf_sz = 1000;

Would you like to have a structure to put those magic numbers?
Or where do you refer those data?

@@ +169,5 @@
> +#if defined(WEBRTC_ARCH_ARM)
> +  // On mobile platform, always set to -12 to leverage between cpu usage
> +  // and video quality
> +  CPUSpeed = -12;
> +#endif

This module isn't used for webRTC on this stage.

@@ +213,5 @@
> +  uint32_t encodedFrameSize = 0;
> +  EncodedFrame::FrameType frameType = EncodedFrame::P_FRAME;
> +
> +  const vpx_codec_cx_pkt_t *pkt = nullptr;
> +  while ((pkt = vpx_codec_get_cx_data(&mVPXContext, &iter)) != nullptr) {

Does the vpx_codec_get_cx_data need to retry several times?
While looks like it should try several times and exit until get data.

It seems this function only generate 1 encoded frame.

@@ +264,5 @@
> +    }
> +
> +    int yPlanSize = mFrameWidth * mFrameHeight;
> +    int halfWidth = (mFrameWidth + 1) / 2;
> +    int halfHeight = (mFrameHeight + 1) / 2;

odd frame problem. Bug 948815

@@ +282,5 @@
> +  } else {
> +    layers::Image* img = aChunk.mFrame.GetImage();
> +    NS_ENSURE_TRUE(img, NS_ERROR_NULL_POINTER);
> +    ImageFormat format = img->GetFormat();
> +  #ifdef MOZ_WIDGET_GONK

b2g now out policy would like to output mp4 format.

@@ +345,5 @@
> +      uint8_t *y = data->mYChannel;
> +      uint8_t *cb = data->mCbChannel;
> +      uint8_t *cr = data->mCrChannel;
> +
> +      mVPXImageWrapper->planes[PLANE_Y] = y;

= data->mYChannel;？

@@ +353,5 @@
> +      mVPXImageWrapper->stride[VPX_PLANE_U] = data->mCbCrStride;
> +      mVPXImageWrapper->stride[VPX_PLANE_V] = data->mCbCrStride;
> +    } else {
> +      //MOZ_MTLOG(ML_ERROR, "Unsupported video format");
> +      MOZ_ASSERT(PR_FALSE);

Let MediaEncoder to handle this. Logging it.

@@ +369,5 @@
> +{
> +  TimeDuration processTime = TimeStamp::Now() - aBaseTime;
> +  uint64_t durationInUsec =
> +    (aProcessedDuration + mEncodedFrameDuration) * USECS_PER_S / mTrackRate;
> +  if (processTime.ToMicroseconds() > (durationInUsec * 3 / 4)) {

How do you get the 3 / 4

@@ +393,5 @@
> +  while (temp > mEncodedFrameDuration) {
> +    temp -= mEncodedFrameDuration;
> +    encodedDuration += mEncodedFrameDuration;
> +  }
> +  mNextFrameDuration = mEncodedFrameDuration - temp;

It seems do two work on this function.
Could we have a function like GetNextFrameDuration and remove the mNextFrameDuration member?

::: content/media/encoder/VP8TrackEncoder.h
@@ +37,5 @@
> + * We implement a fixed FPS encoder
> + */
> +class VP8TrackEncoder : public VideoTrackEncoder
> +{
> +  enum EncodeState {

EncodeState should be the state of the whole trackEncoder, like start, stop, error and etc..
Maybe EncoderOperation ? ENCODE_NORMAL_FRAME/ENCODE_I_FRAME/SKIP_FRAME

@@ +57,5 @@
> +                TrackRate aTrackRate) MOZ_FINAL MOZ_OVERRIDE;
> +
> +private:
> +  // Calculate the target encoded frame's duration and mNextFrameDuration.
> +  int CalculateEncodedDurationAndNextFrameDuration(uint64_t aFrameCopied);

Just do the CalculateEncodedDuration, and uint64_t.

@@ +67,5 @@
> +  // Get the encoded data from encoder to aData.
> +  nsresult GetEncodedPartitions(EncodedFrameContainer& aData);
> +
> +  // Prepare the input data to the mVPXImageWrapper for encoding.
> +  nsresult PrepareEncodedFrame(VideoChunk &aChunk);

PrepareRawFrame? This frame haven't encoded, right?

@@ +72,5 @@
> +
> +  // Output frame rate.
> +  uint32_t mEncodedFrameRate;
> +  // Duration for the output frame, reciprocal to mEncodedFrameRate.
> +  uint32_t mEncodedFrameDuration;

uint64_t

@@ +78,5 @@
> +  uint64_t mEncodedTimestamp;
> +  // Duration to the next encode frame. The range is 0 ~ mEncodedFrameDuration.
> +  uint64_t mNextFrameDuration;
> +
> +  // Mute frame, we only create it once.

// Muted

::: content/media/encoder/moz.build
@@ +9,5 @@
>      'EncodedFrameContainer.h',
>      'MediaEncoder.h',
>      'TrackEncoder.h',
>      'TrackMetadataBase.h',
> +    'VP8TrackEncoder.h',

Build flag. MOZ_VPX

Comment 27

[Benjamin Chen [:bechen]]

(In reply to Randy Lin [:rlin] from comment #26)

> @@ +56,5 @@
> > +  uint32_t GetSize() const  { return mSize; }
> > +  uint8_t *GetBuffer() const { return mBuffer; }
> > +
> > +private:
> > +  uint8_t* mBuffer;
> 
> nsTArray? and remove the mSize

The size of encoded frame output by vp8 is not fixed. So I use a buffer instead nsTArray.

> @@ +369,5 @@
> > +{
> > +  TimeDuration processTime = TimeStamp::Now() - aBaseTime;
> > +  uint64_t durationInUsec =
> > +    (aProcessedDuration + mEncodedFrameDuration) * USECS_PER_S / mTrackRate;
> > +  if (processTime.ToMicroseconds() > (durationInUsec * 3 / 4)) {
> 
> How do you get the 3 / 4
> 

Just a heuristic.

> @@ +393,5 @@
> > +  while (temp > mEncodedFrameDuration) {
> > +    temp -= mEncodedFrameDuration;
> > +    encodedDuration += mEncodedFrameDuration;
> > +  }
> > +  mNextFrameDuration = mEncodedFrameDuration - temp;
> 
> It seems do two work on this function.
> Could we have a function like GetNextFrameDuration and remove the
> mNextFrameDuration member?
> 

I'll try to separate them into 2 functions.
mNextFrameDuration should be a class member for next |GetEncodedTrack|.

Comment 28

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

(In reply to Benjamin Chen [:bechen] from comment #27)
> (In reply to Randy Lin [:rlin] from comment #26)
> 
> > @@ +56,5 @@
> > > +  uint32_t GetSize() const  { return mSize; }
> > > +  uint8_t *GetBuffer() const { return mBuffer; }
> > > +
> > > +private:
> > > +  uint8_t* mBuffer;
> > 
> > nsTArray? and remove the mSize
> 
> The size of encoded frame output by vp8 is not fixed. So I use a buffer
> instead nsTArray.
mdn..
Will your array store non-refcounted objects and need automatic resizing? If so, use nsTArray<T>.
> 
> > @@ +369,5 @@
> > > +{
> > > +  TimeDuration processTime = TimeStamp::Now() - aBaseTime;
> > > +  uint64_t durationInUsec =
> > > +    (aProcessedDuration + mEncodedFrameDuration) * USECS_PER_S / mTrackRate;
> > > +  if (processTime.ToMicroseconds() > (durationInUsec * 3 / 4)) {
> > 
> > How do you get the 3 / 4
> > 
> 
> Just a heuristic.
DEFINE those. :)
> 
> > @@ +393,5 @@
> > > +  while (temp > mEncodedFrameDuration) {
> > > +    temp -= mEncodedFrameDuration;
> > > +    encodedDuration += mEncodedFrameDuration;
> > > +  }
> > > +  mNextFrameDuration = mEncodedFrameDuration - temp;
> > 
> > It seems do two work on this function.
> > Could we have a function like GetNextFrameDuration and remove the
> > mNextFrameDuration member?
> > 
> 
> I'll try to separate them into 2 functions.
> mNextFrameDuration should be a class member for next |GetEncodedTrack|.
Thanks.

Comment 29

[Benjamin Chen [:bechen]]

Attachment: bug-881840-vp8encoder.v06.patch

Address most comments from :rlin.
1. Remove class ImageBuffer, replaced by nsTArray
2. Remove gonk relative codes.
3. Separate CalculateEncodedDurationAndNextFrameDuration to CalculateEncodedDuration and CalculateNextFrameDuration.

TODO:
1. The build flag will consist with webm

Regarding the odd size of image, I think we should keep the codes right now until bug 948815 is resolved.

Comment 30

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

Comment on attachment 8348577 [details] [diff] [review]
bug-881840-vp8encoder.v06.patch

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

LGTM, Could you request Ralph to review that?

::: content/media/encoder/VP8TrackEncoder.cpp
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "VP8TrackEncoder.h"
> +#include "vpx/vpx_encoder.h"

already include in "VP8TrackEncoder.h"

@@ +378,5 @@
> +        NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);
> +
> +        // Encode the data with VP8 encoder
> +        int flags = (nextEncodeOperation == ENCODE_NORMAL_FRAME) ?
> +                    0 : VPX_EFLAG_FORCE_KF;

nit: 0 align to (     :-)

::: content/media/encoder/VP8TrackEncoder.h
@@ +27,5 @@
> +  int32_t mWidth;
> +  int32_t mHeight;
> +  int32_t mEncodedFrameRate;
> +  MetadataKind GetKind() const MOZ_OVERRIDE { return METADATA_VP8; }
> +};

Would remove when integrate WebM writer.

@@ +45,5 @@
> +  enum EncodeOperation {
> +    ENCODE_NORMAL_FRAME, // VP8 track encoder works normally.
> +    ENCODE_I_FRAME, // The next frame will be encoded as I-Frame.
> +    SKIP_FRAME // Skip the next frame.
> +  };

nit: space

::: content/media/encoder/moz.build
@@ +9,5 @@
>      'EncodedFrameContainer.h',
>      'MediaEncoder.h',
>      'TrackEncoder.h',
>      'TrackMetadataBase.h',
> +    'VP8TrackEncoder.h',

Would add build flag for this.

Comment 31

[Shelly Lin [:shelly]]

Comment on attachment 8348577 [details] [diff] [review]
bug-881840-vp8encoder.v06.patch

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

::: content/media/encoder/VP8TrackEncoder.h
@@ +60,5 @@
> +  nsresult Init(int32_t aWidth, int32_t aHeight,
> +                TrackRate aTrackRate) MOZ_FINAL MOZ_OVERRIDE;
> +
> +private:
> +  // Calculate the target encoded frame's duration.

Is the result returns in microseconds?

@@ +65,5 @@
> +  uint64_t CalculateEncodedDuration(uint64_t aDurationCopied);
> +
> +  // Calculate the mNextFrameDuration.
> +  void CalculateNextFrameDuration(uint64_t aDurationCopied,
> +                                  uint64_t aEncodedDuration);

What are aDurationCopied and aEncodedDuration?

@@ +69,5 @@
> +                                  uint64_t aEncodedDuration);
> +
> +  // Get the EncodeOperation for next iteration.
> +  EncodeOperation GetNextEncodeOperation(TimeStamp aBaseTime,
> +                                         uint64_t aProcessedDuration);

What are aBaseTime and aProcessedDuration?

@@ +82,5 @@
> +  uint32_t mEncodedFrameRate;
> +  // Duration for the output frame, reciprocal to mEncodedFrameRate.
> +  uint64_t mEncodedFrameDuration;
> +  // Encoded timestamp.
> +  uint64_t mEncodedTimestamp;

What is the time scale of your time stamp?

@@ +84,5 @@
> +  uint64_t mEncodedFrameDuration;
> +  // Encoded timestamp.
> +  uint64_t mEncodedTimestamp;
> +  // Duration to the next encode frame. The range is 0 ~ mEncodedFrameDuration.
> +  uint64_t mNextFrameDuration;

If this is scaled in TrackTicks, please declare as it is.

@@ +90,5 @@
> +  // Muted frame, we only create it once.
> +  nsTArray<uint8_t> mMuteFrame;
> +
> +  // A local segment queue which stores the raw segments.
> +  nsAutoPtr<VideoSegment> mSourceSegment;

No need to use nsAutoPtr here. Could you briefly state what mSourceSegment is used for?

Comment 32

[Benjamin Chen [:bechen]]

Attachment: bug-881840-vp8encoder.v07.patch

1. use TrackTicks as time unit.
2. add PR_GetNumberOfProcessors() to detect cpu cores.
3. use FramesToUsecs() for time conversion.

Comment 33

[Benjamin Chen [:bechen]]

Comment on attachment 8349981 [details] [diff] [review]
bug-881840-vp8encoder.v07.patch

Hi Ralph:
Could you please help to review this patch?

In this patch, I'm trying to implement a "realtime" and "fixed FPS" encoder.

For realtime encoding: 
If the frame's total duration in mSourceSegment is 100ms, I'll try to contorl the encoding time less than 100ms.
There are three EncodeOperations NORMAL, I_FRAME, SKIP_FRAME.
We assume that the encode speed for encoder is SKIP_FRAME > I_FRAME > NORMAL.
SKIP_FRAME: we don't encode the next frame.
I_FRAME: we force to encode an I-FRAME
NORMAL: Let encoder make the decision.

For fixed FPS encoding:
Because the input frame rate from MediaStreamGraph is 100, and it usually differs to output frame rate (30 FPS usually).
So I need to pick the video chunks in mSourceSegment and calculate the frame duration(mRemainingTicks) precisely.

Thanks.

Comment 34

[Ralph Giles (:rillian)]

Comment on attachment 8349981 [details] [diff] [review]
bug-881840-vp8encoder.v07.patch

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

Sorry Benjamin. I'm happy to look at this, but probably won't get to it before January 6.

Comment 35

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

Hi Ralph,
Do you have time to review this part?

Comment 36

[Ralph Giles (:rillian)]

Comment on attachment 8349981 [details] [diff] [review]
bug-881840-vp8encoder.v07.patch

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

Sorry for the delay. r+ with nits.

I notice you're using both the libvpx deadline mode and your own keyframe/dropframe control loop. Did you find you needed both to get good performance? How does your loop perform with VPX_DL_GOOD_QUALITY? Is the quality comparable on desktop?

Why did you pick a fixed framerate instead of using the track's native rate?

::: content/media/encoder/VP8TrackEncoder.cpp
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "VP8TrackEncoder.h"
> +#include "vpx/vp8cx.h"

You will need vpx/vpx_encode.h here too when you remove it from the header file.

@@ +8,5 @@
> +#include "VideoUtils.h"
> +#include "prsystem.h"
> +
> +#ifdef PR_LOGGING
> +PRLogModuleInfo* gVP8TrackEncoderLog;

Declare this inside the mozilla namespace, please.

@@ +12,5 @@
> +PRLogModuleInfo* gVP8TrackEncoderLog;
> +#define VP8_LOG(msg, ...) PR_LOG(gVP8TrackEncoderLog, PR_LOG_DEBUG, \
> +                                  (msg, ##__VA_ARGS__))
> +// Debug logging macro with object pointer and class name.
> +#define VP8LOG(msg, ...) \

You don't use the other macro at all, might as well make it include the object name and only have one.

@@ +30,5 @@
> +  : VideoTrackEncoder()
> +  , mEncodedFrameDuration(0)
> +  , mEncodedTimestamp(0)
> +  , mRemainingTicks(0)
> +  , mVPXImageWrapper(nullptr)

Make mVPXImageWrapper an nsAutoPtr<vpx_image_t> and you won't need the explicit initializer or conditional free in the dtor.

@@ +32,5 @@
> +  , mEncodedTimestamp(0)
> +  , mRemainingTicks(0)
> +  , mVPXImageWrapper(nullptr)
> +{
> +  memset(&mVPXContext, 0, sizeof(vpx_codec_ctx_t));

I would probably make mVPXContext an nsAutoPtr too, so you can new it here and not need the '&' everywhere you pass it as an argument. But if you prefer the lower allocation overhead of having the struct in the object, that's fine.

@@ +43,5 @@
> +}
> +
> +VP8TrackEncoder::~VP8TrackEncoder()
> +{
> +  vpx_codec_destroy(&mVPXContext);

This needs to be conditional on mInitialized. What happens if the dtor is called before Init?

@@ +193,5 @@
> +    EncodedFrame* videoData = new EncodedFrame();
> +    videoData->SetFrameType(frameType);
> +    // Convert the timestamp and duration to Usecs.
> +    videoData->SetTimeStamp(
> +      (uint64_t)FramesToUsecs(mEncodedTimestamp, mTrackRate).value());

There's no point in using this routine if you're going to cast away the CheckedInt. Please test the isValid() method to check for overflow.

@@ +228,5 @@
> +    mVPXImageWrapper->planes[PLANE_U] = cb;
> +    mVPXImageWrapper->planes[PLANE_V] = cr;
> +    mVPXImageWrapper->stride[VPX_PLANE_Y] = mFrameWidth;
> +    mVPXImageWrapper->stride[VPX_PLANE_U] = halfWidth;
> +    mVPXImageWrapper->stride[VPX_PLANE_V] = halfWidth;

I think it would be easier to read if you moved this clause to ::PrepareMutedFrame(). Then the else clause can become the main body of this function.

@@ +231,5 @@
> +    mVPXImageWrapper->stride[VPX_PLANE_U] = halfWidth;
> +    mVPXImageWrapper->stride[VPX_PLANE_V] = halfWidth;
> +  } else {
> +    layers::Image* img = aChunk.mFrame.GetImage();
> +    NS_ENSURE_TRUE(img, NS_ERROR_NULL_POINTER);

These macros are deprecated style because they hide flow control. See nsDebug.h.

if (NS_WARN_IF(!img)) {
  return NS_ERROR_NULL_POINTER;
}

@@ +238,5 @@
> +      // Cast away constness b/c some of the accessors are non-const
> +      layers::PlanarYCbCrImage* yuv =
> +      const_cast<layers::PlanarYCbCrImage *>(static_cast<const layers::PlanarYCbCrImage *>(img));
> +      // Big-time assumption here that this is all contiguous data coming
> +      // from getUserMedia or other sources.

And way to static_assert or MOZ_ASSERT that?

@@ +248,5 @@
> +      mVPXImageWrapper->stride[VPX_PLANE_Y] = data->mYStride;
> +      mVPXImageWrapper->stride[VPX_PLANE_U] = data->mCbCrStride;
> +      mVPXImageWrapper->stride[VPX_PLANE_V] = data->mCbCrStride;
> +    } else {
> +      VP8LOG("Unsupported video format\n");

Please check for != PLANAR_YCBCR instead so you can return early. Then unindent the perferred path above instead of having it inside the conditional.

@@ +388,5 @@
> +
> +      // Encode frame.
> +      if (nextEncodeOperation != SKIP_FRAME) {
> +        nsresult rv = PrepareRawFrame(chunk);
> +        NS_ENSURE_SUCCESS(rv, NS_ERROR_FAILURE);

if (NS_WARN_IF(NS_FAILED(rv)) {
  return NS_ERROR_FAILURE;
}

::: content/media/encoder/VP8TrackEncoder.h
@@ +7,5 @@
> +#define VP8TrackEncoder_h_
> +
> +#include "TrackEncoder.h"
> +#include "vpx/vpx_codec.h"
> +#include "vpx/vpx_encoder.h"

You shouldn't need to include these in the header file. You have opaque declarations below.

@@ +25,5 @@
> +{
> +public:
> +  int32_t mWidth;
> +  int32_t mHeight;
> +  int32_t mEncodedFrameRate;

This should be a TrackRate for compatibility with other code.

WebM uses a float here, instead of an int. The frame rate in WebM is nominal, with actual frame presentation determined by timestamps on the encoded data. WebRTC (RTP) streams use a 90 kHz timebase, which also has more resolution than in int. A number of legacy framerates used in broadcast video aren't representable as as integer number of frames a second, 30000/1001 for example. That's not important here, but I wanted to be sure you were aware of the issue.

@@ +40,5 @@
> +{
> +  // These two define value used in GetNextEncodeOperation to determine the
> +  // EncodeOperation for next target frame.
> +#define I_FRAME_RATIO (0.5)
> +#define SKIP_FRAME_RATIO (0.75)

Better if these are const float so they're namespaced to the class. Or just move them to the cpp file.

@@ +77,5 @@
> +  // Prepare the input data to the mVPXImageWrapper for encoding.
> +  nsresult PrepareRawFrame(VideoChunk &aChunk);
> +
> +  // Output frame rate.
> +  uint32_t mEncodedFrameRate;

TrackRate here as well?

Comment 37

[Benjamin Chen [:bechen]]

(In reply to Ralph Giles (:rillian) from comment #36)
> Comment on attachment 8349981 [details] [diff] [review]
> bug-881840-vp8encoder.v07.patch
> 
> Review of attachment 8349981 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry for the delay. r+ with nits.
> 
> I notice you're using both the libvpx deadline mode and your own
> keyframe/dropframe control loop. Did you find you needed both to get good
> performance? How does your loop perform with VPX_DL_GOOD_QUALITY? Is the
> quality comparable on desktop?
> 
Thanks for reviewing. I'll try VPX_DL_GOOD_QUALITY later. 

> Why did you pick a fixed framerate instead of using the track's native rate?
> 
Do you mean the rate of MediaStreamGraph?
If so, the rate of MediaStreamGraph is 100 fps for now and I think it's too high for encoding.

Comment 38

[Ralph Giles (:rillian)]

(In reply to Benjamin Chen [:bechen] from comment #37)

> Do you mean the rate of MediaStreamGraph?
> If so, the rate of MediaStreamGraph is 100 fps for now and I think it's too
> high for encoding.

Ok, that is generally too fast. I was thinking the MediaStream would run at some fixed rate based on the getUserMedia request or the camera hardware.

Comment 40

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

This wasn't a duplicate, it was a dependent bug