891705 - [MediaEncoder] Implement WebM 1.0 container writer

Status: VERIFIED FIXED

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

Description

[u459114]

Receive audio(vobis)/video(vp8) track encoded data and multiplex into WebM container.
Receive audio(opus)/video(vp9) track encoded data and multiplex into WebM container.

Comment 1

[u459114]

Implement a muxer to contain VP8 video track and Vobis audio track. Platform independent. WIP need

Comment 2

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

We will implement WebM 2.0 directly. 
904363 - [MediaEncoder] Implement WebM 2.0 container writer. 
Set this to be invalid.

Comment 3

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

Reopen it, we will implement WebM 1 first, then move to WebM 2.

Comment 4

[Ralph Giles (:rillian)]

Sounds good.

Comment 5

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

Attachment: WIP1

WIP for wemM container, including the vp8 encoder part from bug 881840.
Can build & test via people.mozilla.com/~rlin 
(GetAVUserMedia->Start->Pause->PlayVideoBack...)
Many bugs & no audio & need clean-up.

Comment 6

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

Attachment: WIP1-1

remove some debug log /file dump

Comment 7

[u459114]

Comment on attachment 805207 [details] [diff] [review]
WIP1-1

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

::: content/media/encoder/TrackEncoder.h
@@ +116,5 @@
>     * mReentrantMonitor will be notified if other methods is waiting for encoder
>     * to be completely initialized. This method is called on the MediaStreamGraph
>     * thread.
>     */
> +  virtual nsresult Init(int aWidth, int aHeifht) = 0;

typo Height
And, is there any reason to change the name of these parameters?

@@ +256,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.
> +   */

Notifies the _video_ encoder

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

Trailer SPACE!!

::: content/media/webm/EbmlWriterHelper.h
@@ +4,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef EbmlWriterHelper_h_
> +#define EbmlWriterHelper_h_
> +#include "EbmlIDs.h"

Move to cpp

@@ +8,5 @@
> +#include "EbmlIDs.h"
> +#include "nsTArray.h"
> +
> +#define LITERALU64(hi,lo) ((((uint64_t)hi)<<32)|lo)
> +

Move to cpp

@@ +14,5 @@
> +/* This is the WebM Ebml format helper, reference WebM project
> + *
> + */
> +class EbmlWriterHelper {
> +public:

Have a constructor to initiate all data members.

::: content/media/webm/WebMWriter.h
@@ +6,5 @@
> +#ifndef WebMWriter_h_
> +#define WebMWriter_h_
> +
> +#include "ContainerWriter.h"
> +#include "EbmlWriterHelper.h"

Is that possible to include this header in compile unit instead of header?

@@ +10,5 @@
> +#include "EbmlWriterHelper.h"
> +
> +namespace mozilla {
> +/**
> + * WriteEncodedTrack inserts raw packets into webM stream (ogg_stream_state), and

remove ogg_stream_state

@@ +28,5 @@
> +  nsresult GetContainerData(nsTArray<nsTArray<uint8_t> >* aOutputBufs,
> +                            uint32_t aFlags = 0) MOZ_OVERRIDE;
> +
> +private:
> +  nsresult Init();

MOZ_OVERRIDE

Comment 8

[Ralph Giles (:rillian)]

(In reply to C.J. Ku[:CJKu] from comment #7)

> > +  virtual nsresult Init(int aWidth, int aHeifht) = 0;
> 
> typo Height
> And, is there any reason to change the name of these parameters?

FWIW I like to have argument names in the declaration as documentation.

Comment 9

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

ah.....Thanks for feedback for this WIP.
Next one will contain the audio part.

Comment 10

[Kai-Chih Hu [:khu]]

Comment on attachment 805207 [details] [diff] [review]
WIP1-1

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

::: content/media/encoder/MediaEncoder.cpp
@@ +127,5 @@
>    // Return null if we fail to create the audio encoder.
>    NS_ENSURE_TRUE(audioEncoder, nullptr);
>  
>    encoder = new MediaEncoder(writer.forget(), audioEncoder.forget(),
> +                             videoEncoder.forget(), NS_LITERAL_STRING("video/webm"));

Should we define "video/webm" somewhere else rather than specifying the value here?

@@ +184,1 @@
>        if (NS_FAILED(rv)) {

I am curious. Looks like the statement inside this "if" clause won't be executed always, because rv was already assigned as NS_OK earlier.

::: content/media/encoder/TrackEncoder.cpp
@@ +187,5 @@
> +   }
> +
> +   // Append and consume this raw segment.
> +   if (mInitialized) {
> +     LOG("before AppendVideoSegment")

Should we need to add ";" at the end of this line? o.O

Comment 11

[Paul Adenot (:padenot)]

(In reply to khu from comment #10)
> ::: content/media/encoder/MediaEncoder.cpp
> @@ +127,5 @@
> >    // Return null if we fail to create the audio encoder.
> >    NS_ENSURE_TRUE(audioEncoder, nullptr);
> >  
> >    encoder = new MediaEncoder(writer.forget(), audioEncoder.forget(),
> > +                             videoEncoder.forget(), NS_LITERAL_STRING("video/webm"));
> 
> Should we define "video/webm" somewhere else rather than specifying the
> value here?

This should use http://mxr.mozilla.org/mozilla-central/source/netwerk/mime/nsMimeTypes.h#145.

Comment 12

[u459114]

(In reply to Paul Adenot (:padenot) from comment #11)
> (In reply to khu from comment #10)
> > ::: content/media/encoder/MediaEncoder.cpp
> > @@ +127,5 @@
> > >    // Return null if we fail to create the audio encoder.
> > >    NS_ENSURE_TRUE(audioEncoder, nullptr);
> > >  
> > >    encoder = new MediaEncoder(writer.forget(), audioEncoder.forget(),
> > > +                             videoEncoder.forget(), NS_LITERAL_STRING("video/webm"));
> > 
> > Should we define "video/webm" somewhere else rather than specifying the
> > value here?
> 
> This should use
> http://mxr.mozilla.org/mozilla-central/source/netwerk/mime/nsMimeTypes.h#145.
That's a good catch, thanks!

Comment 13

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

Attachment: wip-part1

Comment 14

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

Attachment: wip-part 2

Merge vp8/vorbis encoder and generate a concept proof patch on Linux and can record media stream from gUM(A/V), produce a playable webM blob with A/V.
Need huge clean!

Comment 15

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

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

Attachment: workable patch

Include all things, but still need big refine. :)

Comment 17

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

Attachment: workable patch

miss some files, workable patch for record video from media stream.

Comment 18

[Maire Reavy [:mreavy]]

Hi Randy -- Is the most recent patch for this supposed to build and work?  Also, do you have an ETA for when you will finish this bug?  Thanks.

Comment 19

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

Hi Maire,
This is a workable patch, but it is not ready for reviewing.
We are working on b2g part currently.
For the ETA, We wish we can land all of the code for desktop at this end of year.
Including 879669, 883749, 881840.

Comment 20

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

Attachment: webm muxer patch v1

Hi Ralph, 
Here is my first version of webm muxer, 
Could you help to review it?
Thanks a lot.

Comment 21

[Ralph Giles (:rillian)]

Comment on attachment 8335950 [details] [diff] [review]
webm muxer patch v1

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

Thanks, Randy. This is a good start and the code is clean. Please address the comments below and submit a new patch for review.

It looks like you've copied the EbmlWriter internals from libvpx, not just the EbmlIDs.h header. There is no license conflict so it is fine to use that code, but you need to give proper attribution. I would suggest that we import libvpx/libmkv into media/libmkv (or media/libvpx/libmkv) and call that directly, so it is easier to maintain our copy of the code as the WebM project updates theirs.

::: content/media/webm/EbmlWriterHelper.h
@@ +8,5 @@
> +#include "EbmlIDs.h"
> +#include "nsTArray.h"
> +#include "ContainerWriter.h"
> +
> +#define LITERALU64(hi,lo) ((((uint64_t)hi)<<32)|lo)

This can move to the cpp file.

@@ +29,5 @@
> +  , mChannels(0)
> +    {}
> +  void Write_webm_file_header();
> +  // Insert video buffer into muxer and it would be package into SimpleBlock
> +  // If there is a video key frame comes and new cluster will start for writing/

Period at the end of the sentence please.

@@ +30,5 @@
> +    {}
> +  void Write_webm_file_header();
> +  // Insert video buffer into muxer and it would be package into SimpleBlock
> +  // If there is a video key frame comes and new cluster will start for writing/
> +  void Write_webm_video_block(EncodedFrame* aFrame, bool aIs_keyframe);

'aIsKeyframe'

@@ +68,5 @@
> +
> +  // The temporary storage for cluster data
> +  nsTArray<uint8_t> mBuff;
> +  // Cluster data can be output to encoder
> +  nsTArray<uint8_t> mOutPutBuff;

'mOutputBuffer'

@@ +71,5 @@
> +  // Cluster data can be output to encoder
> +  nsTArray<uint8_t> mOutPutBuff;
> +  // Audio codec specific header data
> +  nsTArray<uint8_t> mCodecPrivateData;
> +  // The position of the cluster lenght in mBuff, for update cluster final size

'length'

@@ +72,5 @@
> +  nsTArray<uint8_t> mOutPutBuff;
> +  // Audio codec specific header data
> +  nsTArray<uint8_t> mCodecPrivateData;
> +  // The position of the cluster lenght in mBuff, for update cluster final size
> +  unsigned long mStartClusterPos;

This will limit us to 4GB files on 32 bit systems. Not ideal.

If you don't want to use a uint64_t here, at least check for overflow so we don't create invalid files.

::: content/media/webm/WebMWriter.cpp
@@ +6,5 @@
> +#include "WebMWriter.h"
> +
> +namespace mozilla {
> +
> +WebMWriter::WebMWriter(uint32_t aCoutentHint) : ContainerWriter()

'aContentHint'

::: content/media/webm/WebMWriter.h
@@ +8,5 @@
> +
> +#include "ContainerWriter.h"
> +#include "EbmlWriterHelper.h"
> +#include "VorbisTrackEncoder.h"
> +#include "VP8TrackEncoder.h"

It looks like you only need to include ContainerWriter.h here; the others can be moved the to .cpp file.

@@ +12,5 @@
> +#include "VP8TrackEncoder.h"
> +
> +namespace mozilla {
> +/**
> + * WriteEncodedTrack inserts raw packets into webM stream (ogg_stream_state), and

'WebM' not 'webM' and remove reference to ogg_stream_state. Better to drop the ', and' and just have one sentence for each method.

@@ +21,5 @@
> + */
> +class WebMWriter : public ContainerWriter
> +{
> +public:
> +  WebMWriter(uint32_t aCoutentHint);

'aContentHint'

::: content/media/webm/moz.build
@@ +5,4 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  EXPORTS += [
> +    'EbmlIDs.h',

I think you don't need to export EbmlIDs.h. Leave it out until you have code which requires it?

Comment 22

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

Hi Ralph, 
hmm, I will try and think about if it possible to directly use the libmkv module and to see if the interface can be used by our design, thanks.

Comment 23

[u459114]

Comment on attachment 8335950 [details] [diff] [review]
webm muxer patch v1

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

::: content/media/webm/EbmlWriterHelper.cpp
@@ +12,5 @@
> +  { \
> +    x = (char)(*(const s *)buffer_in >> (i * 8)); \
> +    Ebml_Write(&x, 1); \
> +  }
> +

#define WRITE_BUFFER(s) \
{ \
  unsigned char x; \
  for(int i = len-1; i>=0; i--)\
  { \
    x = (char)(*(const s *)buffer_in >> (i * 8)); \
    Ebml_Write(&x, 1); \
  } \
}

@@ +16,5 @@
> +
> +void
> +EbmlWriterHelper::Ebml_Serialize(const void *buffer_in, int buffer_size, unsigned long len) {
> +  unsigned char x;
> +  int i;

Remove these two lines

@@ +75,5 @@
> +EbmlWriterHelper::Serialize(const unsigned char *p, const unsigned char *q)
> +{
> +  while (q != p) {
> +    --q;
> +  mBuff.AppendElements(q, 1);

alignment

::: content/media/webm/EbmlWriterHelper.h
@@ +4,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef EbmlWriterHelper_h_
> +#define EbmlWriterHelper_h_
> +#include "EbmlIDs.h"

Is there need to include this EbmlIDs.h in this header?

@@ +14,5 @@
> +namespace mozilla {
> +/*
> + * A webM muxer helper for package the valid webM 1.0 format
> + */
> +class EbmlWriterHelper {

We have ISOComposer in MP4 muxer, how about rename this class to EbmlComposer?

@@ +26,5 @@
> +  , mFrameRate(0)
> +  , mSampleFreq(0)
> +  , mBitDepth(0)
> +  , mChannels(0)
> +    {}

Suggest move to cpp, make class definition here cleaner

@@ +27,5 @@
> +  , mSampleFreq(0)
> +  , mBitDepth(0)
> +  , mChannels(0)
> +    {}
> +  void Write_webm_file_header();

WriteFileHeader, remove webm in function names

@@ +40,5 @@
> +  // Assign the parameter which header required.
> +  void SetVideoConfig(uint32_t aWidth, uint32_t aHeight, float aFrameRate);
> +  void SetAudioConfig(uint32_t aSampleFreq, uint32_t bitDepth, uint32_t aChannels);
> +  // Set the CodecPrivateData for writing in header
> +  void SetAudioCodecPrivateData(nsTArray<uint8_t>& aBufs) { mCodecPrivateData.AppendElements(aBufs); };

Move declaration of these three functions SetVideoConfig SetAudioConfig SetAudioCodecPrivateData ahead(before WriteFileHeader)

Comment 24

[u459114]

Comment on attachment 8335950 [details] [diff] [review]
webm muxer patch v1

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

::: content/media/webm/EbmlWriterHelper.cpp
@@ +384,5 @@
> +{
> +  unsigned long  block_length;
> +  unsigned char  track_number;
> +  unsigned short block_timecode = 0;
> +  unsigned long int            localstart_cluster = 1;

Redundant Space

@@ +414,5 @@
> +}
> +
> +void
> +EbmlWriterHelper::Write_webm_file_footer() {
> +

Move down {

@@ +423,5 @@
> +}
> +
> +void
> +EbmlWriterHelper::SetVideoConfig(uint32_t aWidth, uint32_t aHeight, float aFrameRate)
> +{

Parameter assertions

@@ +431,5 @@
> +}
> +
> +void
> +EbmlWriterHelper::SetAudioConfig(uint32_t aSampleFreq, uint32_t bitDepth, uint32_t aChannels)
> +{

Parameter assertions

::: content/media/webm/EbmlWriterHelper.h
@@ +77,5 @@
> +  // The timecode of the cluster
> +  unsigned long mCluster_timecode;
> +  // A flag to indicate if this processing cluser is keep in open
> +  int mCluster_IsOpen;
> +  // Video config

// Video configuration

@@ +81,5 @@
> +  // Video config
> +  int mWidth;
> +  int mHeight;
> +  float mFrameRate;
> +  // Audio config

// Audio configuration

::: content/media/webm/WebMWriter.cpp
@@ +48,5 @@
> +    MOZ_ASSERT(meta->mEncodedFrameRate > 0, "mFrameRate should > 0");
> +    MOZ_ASSERT(meta->mHeight > 0, "Height should > 0");
> +    MOZ_ASSERT(meta->mWidth > 0, "mWidth should > 0");
> +    mEbmlWriterHelper->SetVideoConfig(meta->mWidth, meta->mHeight, meta->mEncodedFrameRate);
> +    mEbmlWriterHelper->Write_webm_file_header();

Move this line(mEbmlWriterHelper->Write_webm_file_header()) to #61

Comment 25

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

Hi Ralph, 
I found libvpx implement has two different design
One is webmem.h/c, it's file base operation and used for libvpxenc.
Another one is directly use the memcpy to manipulate the memory and it is written by c.
So I would like to keep the original one. :)

Comment 26

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

Attachment: webm muxer patch v2

Patch v2 for reviewing. Fix cj & Ralph nits.

Comment 27

[u459114]

Comment on attachment 8340730 [details] [diff] [review]
webm muxer patch v2

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

::: content/media/webm/EbmlComposer.h
@@ +30,5 @@
> +  // End the cluser and ready to close the webm file.
> +  void WriteFileFooter();
> +  // Output buffer to encoder.
> +  void ExtractBuffer(nsTArray<uint8_t>& aDestBufs, uint32_t aFlag = 0);
> +private:

Suggestion Use pimpl pattern. Move all these Embl function into EmblComposerImp class, and move the definition of EmblComposerImp into cpp.

::: content/media/webm/WebMWriter.cpp
@@ +7,5 @@
> +#include "EbmlComposer.h"
> +
> +namespace mozilla {
> +
> +WebMWriter::WebMWriter(uint32_t aContentHint) : ContainerWriter()

Why we need to pass aContentHint in constructor?
How does MediaCodec, ContainerWriter creator, know this value?
What aContentHint means?

@@ +17,5 @@
> +nsresult
> +WebMWriter::WriteEncodedTrack(const EncodedFrameContainer& aData,
> +                             uint32_t aFlags)
> +{
> +  EncodedFrame* frame;

EncodedFrame* frame = nullptr;

@@ +19,5 @@
> +                             uint32_t aFlags)
> +{
> +  EncodedFrame* frame;
> +  for (uint32_t i = 0 ; i < aData.GetEncodedFrames().Length(); i++) {
> +    frame = aData.GetEncodedFrames().ElementAt(i).get();

Or
EncodedFrame* frame = aData.GetEncodedFrames().ElementAt(i).get();

@@ +53,5 @@
> +    mEbmlComposer->SetVideoConfig(meta->mWidth, meta->mHeight, meta->mEncodedFrameRate);
> +    mEbmlComposer->WriteFileHeader();
> +  }
> +
> +  if (aMetadata->GetKind() == TrackMetadataBase::METADATA_VORBIS)

else if

@@ +59,5 @@
> +    VorbisMetadata* meta = static_cast<VorbisMetadata*>(aMetadata);
> +    MOZ_ASSERT(meta, "no vorbis encoder metadata");
> +    MOZ_ASSERT(meta->mData.Length() > 0, "private codec data should has data");
> +    mEbmlComposer->SetAudioCodecPrivateData(meta->mData);
> +  }

else { ASSERT(false); }

::: content/media/webm/WebMWriter.h
@@ +47,5 @@
> +  // Assign Metaddata into muxer
> +  nsresult SetMetadata(TrackMetadataBase* aMetadata) MOZ_OVERRIDE;
> +
> +private:
> +  nsAutoPtr<EbmlComposer> mEbmlComposer;

mComposer

Comment 28

[u459114]

Comment on attachment 8340730 [details] [diff] [review]
webm muxer patch v2

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

::: content/media/webm/EbmlComposer.h
@@ +15,5 @@
> +class EbmlComposer {
> +public:
> +  EbmlComposer();
> +  // Assign the parameter which header required.
> +  void SetVideoConfig(uint32_t aWidth, uint32_t aHeight, float aFrameRate);

Nit.
Please coordinate with other engineers in encoder pipeline and define the rule of function naming in this module.
John name this function as ConfigVideo and you name this function as SetVideoConfig.

@@ +53,5 @@
> +  void Ebml_EndSubElement(unsigned long *ebmlLoc);
> +  void WriteSeekInfo();
> +
> +  // The temporary storage for cluster data.
> +  nsTArray<uint8_t> mBuff;

Rename to mClusterCacheBuffer?

@@ +55,5 @@
> +
> +  // The temporary storage for cluster data.
> +  nsTArray<uint8_t> mBuff;
> +  // Cluster data can be output to encoder.
> +  nsTArray<uint8_t> mOutPutBuffer;

mOut"p"utBuffer

@@ +57,5 @@
> +  nsTArray<uint8_t> mBuff;
> +  // Cluster data can be output to encoder.
> +  nsTArray<uint8_t> mOutPutBuffer;
> +  // Audio codec specific header data.
> +  nsTArray<uint8_t> mCodecPrivateData;

mAudioPrviateData?

@@ +63,5 @@
> +  uint64_t mStartClusterPos;
> +  // The timecode of the cluster
> +  uint64_t mCluster_timecode;
> +  // A flag to indicate if this processing cluser is keep in open.
> +  int mCluster_IsOpen;

Suggestion. You may create a Cluser inline class and put operation and data into this class
mCluster_IsOpen: rename to mOpen
mCluster_timecod: rename to mTimecode
mStartClusterPos" rename to mPosition
mBuff: rename to mCacheBuffer

Make these data be private and add public setter/getter functions to access/ manipulate these data

Comment 29

[u459114]

Comment on attachment 8340730 [details] [diff] [review]
webm muxer patch v2

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

::: content/media/webm/EbmlComposer.h
@@ +14,5 @@
> + */
> +class EbmlComposer {
> +public:
> +  EbmlComposer();
> +  // Assign the parameter which header required.

Separate each function with an empty line.

@@ +19,5 @@
> +  void SetVideoConfig(uint32_t aWidth, uint32_t aHeight, float aFrameRate);
> +  void SetAudioConfig(uint32_t aSampleFreq, uint32_t bitDepth, uint32_t aChannels);
> +  // Set the CodecPrivateData for writing in header.
> +  void SetAudioCodecPrivateData(nsTArray<uint8_t>& aBufs) { mCodecPrivateData.AppendElements(aBufs); };
> +  void WriteFileHeader();

More comment need. Such as
User must call this function before encoding?

@@ +22,5 @@
> +  void SetAudioCodecPrivateData(nsTArray<uint8_t>& aBufs) { mCodecPrivateData.AppendElements(aBufs); };
> +  void WriteFileHeader();
> +  // Insert video buffer into muxer and it would be package into SimpleBlock.
> +  // If there is a video key frame comes and new cluster will start for writing.
> +  void WriteVideoBlock(EncodedFrame* aFrame, bool aIskeyframe);

Should we pass aIsKeyFrame? or it should be a member of EncodedFrame?

And, aIsKeyFrame

@@ +26,5 @@
> +  void WriteVideoBlock(EncodedFrame* aFrame, bool aIskeyframe);
> +  // Insert audio buffer into muxer and it would be package into SimpleBlock.
> +  // If no cluster is opened, new cluster will start for writing.
> +  void WriteAudioBlock(EncodedFrame* frame);
> +  // End the cluser and ready to close the webm file.

More comment need. Such as
User must call this function in the end of encoding?

Comment 30

[u459114]

Comment on attachment 8340730 [details] [diff] [review]
webm muxer patch v2

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

::: content/media/webm/EbmlComposer.h
@@ +22,5 @@
> +  void SetAudioCodecPrivateData(nsTArray<uint8_t>& aBufs) { mCodecPrivateData.AppendElements(aBufs); };
> +  void WriteFileHeader();
> +  // Insert video buffer into muxer and it would be package into SimpleBlock.
> +  // If there is a video key frame comes and new cluster will start for writing.
> +  void WriteVideoBlock(EncodedFrame* aFrame, bool aIskeyframe);

What if user call WriteVideo/AudioBlock before WriteFileHeader?

@@ +27,5 @@
> +  // Insert audio buffer into muxer and it would be package into SimpleBlock.
> +  // If no cluster is opened, new cluster will start for writing.
> +  void WriteAudioBlock(EncodedFrame* frame);
> +  // End the cluser and ready to close the webm file.
> +  void WriteFileFooter();

I didn't find any place call this function, is that right?

@@ +49,5 @@
> +  void Ebml_SerializeString(unsigned long class_id, const char* s);
> +  void Ebml_SerializeData(unsigned long class_id, uint8_t *data, unsigned long data_length);
> +  void Ebml_WriteVoid(unsigned long vSize);
> +  void Ebml_StartSubElement(unsigned long *ebmlLoc, unsigned long class_id);
> +  void Ebml_EndSubElement(unsigned long *ebmlLoc);

Make all serialize function as a static free function in cpp

Comment 31

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

Attachment: workable.patch, base on 158707 Dec.4

Comment 32

[Ralph Giles (:rillian)]

Comment on attachment 8340730 [details] [diff] [review]
webm muxer patch v2

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

EbmlComposer still looks like a modified version of libmkv/EmblWriter. I think you should just import the full source from libvpx and call the C code directly from your WebMWriter object. This just makes a new copy of the code which is difficult share updates to.

Now that libvpx 1.3.0 has landed, you can import libmkv from libvpx/third_party/libmkv. It makes more sense to put it its own media/libmkv directory rather than under media/libvpx. I suggest making that a separate bug.

Comment 33

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

Attachment: WebMWriter v1

This is the WebM writer part, 
For the emblComposer, I will provide another one to link the libmkv library.
I also create a build flag "MOZ_WEBM_ENCODER" for this muxer.

Comment 34

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

For the libmkv, 
Can I modify the source to avoid build problem? Or should I have a patch to apply during building time?

Comment 35

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

Attachment: libmkv patch v1

This is the 1st version of libmkv and it's from libvpx project with some modification.

Comment 36

[Ralph Giles (:rillian)]

Comment on attachment 8345780 [details] [diff] [review]
libmkv patch v1

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

This is a better direction than the C++ version you had before. However, if you're going to do it this way you need to add the directory to media/libvpx/update.py (or perhaps remove it from the ignore list) so it can be updated with the rest of the code in this directory.

It would be better treat this as a separate library in /media/libmkv, with it's own moz.build and update script, I think. After all libvpx treats this as a 'third party' library, even though the libvpx repository seems to be the upstream source for it. This means we have to remember to update them both when now libvpx releases come out, but it's less confusing as we're using this library for Opus and Vorbis in WebM, not just vp8 and vp9.

We should also ask the webm maintainers to put a license header on EbmlBufferWriter. I will do that.

Comment 37

[Ralph Giles (:rillian)]

(In reply to Randy Lin [:rlin] from comment #34)
> For the libmkv, 
> Can I modify the source to avoid build problem? Or should I have a patch to
> apply during building time?

To clarify, the way we do this with the other media libraries is that we have a script with copies the relevant files from an upstream release, then applies any patches we have which aren't in that release. That way it's clear what version we're using, what we've changed, and what to check when we update to a new release.

see https://github.com/mozilla/gecko-dev/blob/master/media/libogg/update.sh for the simplest example of this. A more complicated example is media/libopus/update.sh which reads the makefiles to get the list of required files and updates README_MOZILLA with the imported revision number.

So of course you can change it to make it build, but you should also check in a patch and a script to apply it. Otherwise someone else may forget to apply the fix later.

Comment 38

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

Attachment: WebMWriter v2

WebMWriter v2
Directly use libmkv for packaging WebM format.

Comment 39

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

Comment on attachment 8345216 [details] [diff] [review]
WebMWriter v1

Cancel this patch review and those code is in WebMWriter v2.

Comment 40

[Ralph Giles (:rillian)]

Comment on attachment 8355494 [details] [diff] [review]
WebMWriter v2

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

I like this shape with the libmkv calls better. r- for the ctor issue.

::: content/media/webm/EbmlComposer.cpp
@@ +12,5 @@
> +void EbmlComposer::GenerateHeader()
> +{
> +  // Write the EBML header.
> +  EbmlGlobal ebml;
> +  nsAutoArrayPtr<uint8_t> buffer(new uint8_t[512000]);

The header should be much smaller than this, unless there's a cover art image in the A_VORBIS codec private (which shouldn't be in webm anyway). Can you use a smaller default buffer, and add mCodecPrivateData.Length() for the variable part?

@@ +58,5 @@
> +{
> +  EbmlLoc ebmlLoc;
> +  ebmlLoc.offset = 0;
> +  {
> +    Ebml_StartSubElement(aEbml, &ebmlLoc, SeekHead);

todo? :-)

@@ +61,5 @@
> +  {
> +    Ebml_StartSubElement(aEbml, &ebmlLoc, SeekHead);
> +    Ebml_EndSubElement(aEbml, &ebmlLoc);
> +  }
> +  writeSegmentInformation(aEbml, &ebmlLoc, 1000000, 0);

Please use a define or const unsigned long for the timescale.

::: content/media/webm/WebMWriter.cpp
@@ +9,5 @@
> +namespace mozilla {
> +
> +WebMWriter::WebMWriter(uint32_t aTrackTypes) : ContainerWriter()
> +{
> +  NS_ENSURE_FALSE_VOID(aTrackTypes == ContainerWriter::HAS_AUDIO);

nsDebug.h says to use:

  if (NS_WARN_IF(aTrackTypes == ContainerWriter::HAS_AUDIO) {
    return;
  }

to avoid hiding the flow control. There was a dev-platform thread on this recently.

I'm unclear what this check is trying to enforce; a comment would have been helpful. It catches audio-only hints. I'm in favour of not allowing recording audio-only webm files, since it's better for compatibility to use an audio-only format instead. Would it be more clear to check the bit flag explicitly, e.g. NS_WARN_IF(!(aTrackTypes & ContainerWriter::HAS_VIDEO))?

Finally, this is a bad place for this check, since constructors can't really fail except by throwing exceptions, which we don't use. If you return early for invalid arguments here you leave mEbmlComposer uninitialized, so you need to check it against nullptr in all the other methods. I think it would be better to enforce this at the caller, and only use MOZ_ASSERT here to crash if the caller violates the constraint. Or change the ContainerWriter API to not take aTrackTypes at all, and instead infer and initialize the required headers based on the sequence of calls to SetMetadata.

@@ +12,5 @@
> +{
> +  NS_ENSURE_FALSE_VOID(aTrackTypes == ContainerWriter::HAS_AUDIO);
> +  mMetadataRequiredFlag = aTrackTypes;
> +  mEbmlComposer = new EbmlComposer();
> +  MOZ_COUNT_CTOR(WebMWriter);

Don't forgot to remove this if you end up leaving out the MOZ_COUNT_DTOR macro in the destructor.

@@ +52,5 @@
> +  if (aMetadata->GetKind() == TrackMetadataBase::METADATA_VORBIS) {
> +    VorbisMetadata* meta = static_cast<VorbisMetadata*>(aMetadata);
> +    MOZ_ASSERT(meta, "Cannot find vorbis encoder metadata");
> +    // BitDepth set 10bit as default
> +    mEbmlComposer->SetAudioConfig(meta->mSamplingFrequency, meta->mChannels, 10);

Why 10 bit? Couldn't this come from WebAudio or desktop GetUserMedia and have full bit depth?

::: content/media/webm/WebMWriter.h
@@ +45,5 @@
> +  WebMWriter(uint32_t aTrackTypes);
> +  /*
> +  ~WebMWriter()
> +  {
> +    MOZ_COUNT_DTOR(WebMWriter);

I assume you'll remove or uncomment this before commit.

@@ +58,5 @@
> +  // queue data.
> +  nsresult GetContainerData(nsTArray<nsTArray<uint8_t> >* aOutputBufs,
> +                            uint32_t aFlags = 0) MOZ_OVERRIDE;
> +
> +  // Assign Metad data into muxer

'metadata', period at the end of the sentence, please.

Comment 41

[u459114]

Comment on attachment 8355494 [details] [diff] [review]
WebMWriter v2

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

::: content/media/webm/EbmlComposer.h
@@ +7,5 @@
> +#define EbmlComposer_h_
> +#include "nsTArray.h"
> +#include "ContainerWriter.h"
> +#include "libmkv/WebMElement.h"
> +

Why need to include WebMElement.h in header file?

Comment 42

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

Attachment: WebMWriter v3

Fix nits,
For the NS_WARN_IF(aTrackTypes == ContainerWriter::HAS_AUDIO).
I remove it and the writer can support the weba if we want to produce it and let encoder decide.

For this one. +#include "libmkv/WebMElement.h", 
Remove the only one used function in the header.

Comment 43

[Ralph Giles (:rillian)]

Comment on attachment 8357672 [details] [diff] [review]
WebMWriter v3

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

r=me with nits addressed.

::: content/media/webm/EbmlComposer.cpp
@@ +6,5 @@
> +#include "EbmlComposer.h"
> +#include "libmkv/EbmlIDs.h"
> +#include "libmkv/EbmlWriter.h"
> +#include "libmkv/WebMElement.h"
> +

Please remove trailing whitespace here and between every function in this file.

@@ +18,5 @@
> +void EbmlComposer::GenerateHeader()
> +{
> +  /* Write the EBML header. */
> +  EbmlGlobal ebml;
> +  // The WEbM header default size usually small than 1k.

WebM...smaller than.

@@ +44,5 @@
> +          writeVideoTrack(&ebml, 0x1, 0, cid_string,
> +                          mWidth, mHeight, mFrameRate);
> +          // Audio
> +          if (mCodecPrivateData.Length() > 0) {
> +            strcpy(cid_string, "A_VORBIS");

This writes track elements for both VP8 and Vorbis regardless of whether the encoder will be supplying data for both sets of tracks. You need to conditionalize the video as well if since you're allowing audio-only files. Maybe check for mWidth > 0 and mHeight > 0?

@@ +56,5 @@
> +    }
> +    Ebml_EndSubElement(&ebml, &ebmlLoc);
> +  }
> +  MOZ_ASSERT_IF(ebml.offset > DEFAULT_HEADER_SIZE + mCodecPrivateData.Length(),
> +                "write more data > EBML_BUFFER_SIZE");

Yay! :-)

@@ +91,5 @@
> +    FinishCluster();
> +  }
> +
> +  mClusterBuffs.AppendElement();
> +  mClusterBuffs.LastElement().SetLength(aFrame->GetFrameData().Length()+ 100);

Where does the 100 come from?

@@ +100,5 @@
> +    Ebml_StartSubElement(&ebml, &ebmlLoc, Cluster);
> +    mClusterHeaderIndex = mClusterBuffs.Length() - 1; // current cluster header array index
> +    mClusterLengthLoc = ebmlLoc.offset;
> +    if (aFrame->GetFrameType() != EncodedFrame::FrameType::AUDIO_FRAME) {
> +      mClusterTimecode = aFrame->GetTimeStamp()/1000;

1000 is NSEC_PER_USEC I guess? EncodedFrame doesn't document units for this function. Please use a descriptive name, either in this file or add it it to VideoUtils.h or prtime.h.

@@ +117,5 @@
> +                     0, 0, (unsigned char*)aFrame->GetFrameData().Elements(),
> +                     aFrame->GetFrameData().Length());
> +  }
> +
> +  mClusterBuffs.LastElement().SetLength(ebml.offset);

Please also assert ebml.offset didn't exceed the length you set at the start.

@@ +125,5 @@
> +EbmlComposer::SetVideoConfig(uint32_t aWidth, uint32_t aHeight,
> +                             float aFrameRate)
> +{
> +  MOZ_ASSERT(aWidth > 0, "Width should > 0");
> +  MOZ_ASSERT(aHeight > 0, "Height should 0");

Missing ">"

@@ +137,5 @@
> +EbmlComposer::SetAudioConfig(uint32_t aSampleFreq, uint32_t aChannels,
> +                             uint32_t aBitDepth)
> +{
> +  MOZ_ASSERT(aSampleFreq > 0, "SampleFreq should > 0");
> +  MOZ_ASSERT(aBitDepth > 0, "BitDepth should 0");

missing ">"

@@ +167,5 @@
> + , mHeight(0)
> + , mFrameRate(0)
> + , mSampleFreq(0)
> + , mBitDepth(0)
> + , mChannels(0) {}

per style, you should indent two spaces before the , and put the {} on a line by itself.

::: content/media/webm/EbmlComposer.h
@@ +8,5 @@
> +#include "nsTArray.h"
> +#include "ContainerWriter.h"
> +
> +namespace mozilla {
> +

Trailing whitespace here.

@@ +15,5 @@
> + */
> +class EbmlComposer {
> +public:
> +  EbmlComposer();
> +  ~EbmlComposer() {};

You should be able to leave this out and use the default constructor.

@@ +21,5 @@
> +   * Assign the parameter which header required.
> +   */
> +  void SetVideoConfig(uint32_t aWidth, uint32_t aHeight, float aFrameRate);
> +  /*
> +   * Assign the parameter which header required.

There's no point in duplicating the comment here. The first one can serve for both declarations.

@@ +42,5 @@
> +   * into SimpleBlock. If no cluster is opened, new cluster will start for writing.
> +   */
> +  void WriteSimpleBlock(EncodedFrame* aFrame);
> +  /*
> +   * Get valid cluster data

Period at the end of the sentence, please. Here and below.

::: content/media/webm/WebMWriter.h
@@ +4,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef WebMWriter_h_
> +#define WebMWriter_h_
> +

Trailing whitespace here and on other blank lines. Please check your whole patch for this. Setting your editor to highlight it can also be helpful.

Comment 44

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

Attachment: WebMWriter v3.1

Thanks Ralph. :)
Fix nits. carry reviewer. 
Need to wait vp8 patch and re-base again.

Comment 45

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

Comment on attachment 8358310 [details] [diff] [review]
WebMWriter v3.1

ah...wrong patch.

Comment 46

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

Attachment: WebMWriter v3.1

This one is correct.

Comment 47

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

Attachment: bug.patch

Hi Gregory, 
If I just want to enable this future on firefox browser on windows/mac/linux platform. Is my patch correct?  (except fennec/b2g/b2g desktop..)

Comment 48

[Gregory Szorc [:gps]]

Comment on attachment 8360216 [details] [diff] [review]
bug.patch

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

::: configure.in
@@ +3981,5 @@
>  MOZ_SAMPLE_TYPE_FLOAT32=
>  MOZ_SAMPLE_TYPE_S16=
>  MOZ_OPUS=1
>  MOZ_WEBM=1
> +MOZ_WEBM_ENCODER=1

This will enable MOZ_WEBM_ENCODER globally. You probably want to move this line to browser/confvars.sh so it only takes effect on Firefox desktop.

Comment 49

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

Attachment: enable build flag for webm muxer

thanks, only enable this flag on browser.

Comment 50

[Benjamin Chen [:bechen]]

Attachment: integrate.patch

It's a workable patch which integrates wemb, libmkv, vp8, vorbis patches.

Known issue:recording a video from "fakeUserMedia", the file's audio is abnormal played by vlc. But it is fine in our browser.

Comment 51

[Benjamin Chen [:bechen]]

Attachment: integrate.patch

fix try server errors.

https://tbpl.mozilla.org/?tree=Try&rev=6a44a307e530

Comment 52

[Ryan VanderMeulen [:RyanVM]]

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

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a331400db013

Comment 54

[Daniel Holbert [:dholbert]]

This is causing local build failures on my machine, with this error:
> media/libmkv/EbmlWriter.h:18:10: fatal error: 'vpx/vpx_integer.h' file not found
> #include "vpx/vpx_integer.h"
>          ^

Is this header supposed to be provided by some 3rd-party library?

If so, we should check for it up-front in configure.in (like we do for other build dependencies like e.g. pulseaudio) and immediately fail, with a helpful message, rather than failing halfway through the build.

Comment 55

[Daniel Holbert [:dholbert]]

(For anyone else hitting the build error in Comment 54: "sudo apt-get install libvpx-dev" fixes the issue for me on Ubuntu 13.10.)

Still, we absolutely should be checking for this library in configure.in and failing early. I'd expect this is going to be breaking pretty much every linux user's builds (except folks who happen to have libvpx-dev already installed), in the next day or so, so the sooner we can add a helpful message, the better.

Comment 56

[Ralph Giles (:rillian)]

Are you building --with-system-libvpx? I have a media/libvpx/vpx/vpx_integer.h in my tree.

Comment 57

[Daniel Holbert [:dholbert]]

No, I'm not.

I am, however, building with
 ac_add_options --disable-ogg
 ac_add_options --disable-webm
which maybe is what keeps us from finding that file.

(This maybe means my predictions about "every linux user" may have been overblown. It appears maybe this only broke --disable-webm builds.)

Comment 58

[Daniel Holbert [:dholbert]]

I spun my issue off as bug 964484; we can continue the discussion there.

Comment 59

[Jason Smith [:jsmith]]

I just finished off an initial exploratory test pass on this today - see dependencies for followups.