883749 - [MediaEncoder] Implement Vorbis encoding

Status: RESOLVED DUPLICATE of bug 891705

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

Comment 1

[Ralph Giles (:rillian)]

I would suggest separate bugs for the vorbis and mp3 implementations; they'll be quite different.

Comment 2

[Ralph Giles (:rillian)]

Er, unless you're planning to use android libraries for both?

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

I started a thread on mozilla.dev.media to discuss what formats we want to support for MediaEncoder:

http://groups.google.com/group/mozilla.dev.media/browse_frm/thread/cbe2a8b21b94478e#

We should get that decided before we invest a lot of engineering effort down a track that we may later decide we don't want to support.

Comment 4

[u459114]

Support Vobis audio sample encoding on all platforms, except B2G

Comment 5

[Timothy B. Terriberry (:derf)]

If you're excluding B2G because there is no fixed-point encoder, I think we should follow Andreas's suggestion and include it anyway, even if it is slow on devices with a bad/no FPU. It will be fast on some devices, and there's no new code to write.

Comment 6

[u459114]

Understand, and agree with you

On B2G, suppose we will choice platform encoder path(bug 879668) to leverage HW codec. VP8/ Vobis should be a fallback path, in case we do meet an device which has no HW codec at all.

Comment 7

[Benjamin Chen [:bechen]]

Attachment: WIP-bug-883749-update_libvobis.part1.patch

Add files from libvorbis svn r18077 for encoder.

Comment 8

[Benjamin Chen [:bechen]]

Attachment: libvorbis svn r18077bug-883749-update_libvobis.part2.patch

Modify moz.build and the update.sh. In update.sh we can see the new file list we add in
bug-883749-update_libvobis.part1.patch.

Comment 9

[Benjamin Chen [:bechen]]

Attachment: WIP:bug-883749-update_libvobis.v01.patch

It a WIP patch for VorbisTrackEncoder.
On desktop build, now we can encode an audio stream from MediaStreamGraph to ogg/vorbis format.
And output to file played by vlc.

Comment 10

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

Great, I will start to integrate this with WebM 1 muxer.

Comment 11

[Benjamin Chen [:bechen]]

Current the libvorbis only works on desktop build.
For B2G (and Fennec!?), the libtremor(fixed-point) will decode the vorbis stream.

Here is the problem, how to add the vorbis encoder into B2G (Fennec!?) ?

1. using libvorbis to replace libtremor?
libvorbis uses float but libtremor uses fixed-point.
Mobile device's audio sample is S16, not float.
2. libtremor has encoder ability?
3. using opus in webM1.0, drop vorbis

Hi :derf and :rillian:
Do you have any suggestions about the vorbis encoder on B2G?

Comment 12

[Ralph Giles (:rillian)]

There is no fixed-point vorbis encoder implementation. You'll have to use libvorbis for encode while (if at all possible) libtremor for decode. You'll have to convert the native S12 buffer to non-interleaved float and write this into the arrays returned by vorbis_analysis_buffer(); libvorbis could only support fixed-point input by doing the same thing internally.

I would prefer we try that first. If performance is unacceptable we can fall back to "webm 1.5" with vp8+opus.

Comment 13

[Timothy B. Terriberry (:derf)]

(In reply to Ralph Giles (:rillian) from comment #12)
> I would prefer we try that first. If performance is unacceptable we can fall
> back to "webm 1.5" with vp8+opus.

Unfortunately, very little will play that back today (including Firefox). The other option is Monty burns a month and writes a fixed-point Vorbis encoder. Whether he thinks that's a good use of his time I leave up to him.

But let's test speed first.

Comment 14

[Ralph Giles (:rillian)]

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

> Add files from libvorbis svn r18077 for encoder.

If you're touching media/libvorbis, you might as well update to the 1.3.3 release code. This will let you drop bug719612.patch and bug722924.patch which are fixed upstream. If you prefer I can do that in a separate bug.

1.3.3 release is also available as https://svn.xiph.org/tags/vorbis/libvorbis-1.3.3@18190

Comment 15

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

hmm, Let us try the encoder performance on arm device.

Comment 16

[Benjamin Chen [:bechen]]

Attachment: WIP:bug-883749-vorbisEncoder.v01.patch

Comment 17

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

It seems vorbisEncoder already package the encode data in ogg container, 
We should let oggwriter to do this job.

Comment 18

[Benjamin Chen [:bechen]]

I just make a quick test on unagi device.

1. In configure.in, define MOZ_SAMPLE_TYPE_FLOAT32=1 to include the libvorbis instead of libtremor. (of course it breaks the whole b2g device audio)
2. open browser, https://rawgithub.com/randylin/mediaRecorder/master/MediaRecorder.html
press getFakeUserMedia and then start

By top command in adb shell, we can observer the cpu usage for vorbis encoder is about 25%~30%.

Comment 19

[Benjamin Chen [:bechen]]

(In reply to Timothy B. Terriberry (:derf) from comment #13)
> (In reply to Ralph Giles (:rillian) from comment #12)
> > I would prefer we try that first. If performance is unacceptable we can fall
> > back to "webm 1.5" with vp8+opus.
> 
> Unfortunately, very little will play that back today (including Firefox).
> The other option is Monty burns a month and writes a fixed-point Vorbis
> encoder. Whether he thinks that's a good use of his time I leave up to him.
> 
> But let's test speed first.

Hi :derf,
It would be nice if we can have fixed-point vorbis encoder.
If libtremor encoder is faster than libvorbis encoder on mobile device, that means we can reserve more cpu utilization for video encoding when recording.

Comment 20

[Ralph Giles (:rillian)]

Well, you have our opinions. Monty, can you comment on how long a fixed-point vorbis encoder would take?

Comment 21

[Benjamin Chen [:bechen]]

Attachment: WIP-bug-883749-vorbisEncoder.v02.patch

sync to bug 919905 latest patch.

Comment 22

[u459114]

Comment on attachment 819527 [details] [diff] [review]
WIP-bug-883749-vorbisEncoder.v02.patch

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

Shelly/ Randy, please give feedback for part 3, thanks

Comment 23

[u459114]

Comment on attachment 819527 [details] [diff] [review]
WIP-bug-883749-vorbisEncoder.v02.patch

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

Shelly/ Randy, please give feedback for part 3, thanks

::: content/media/encoder/VorbisTrackEncoder.h
@@ +36,5 @@
> +   */
> +  nsAutoPtr<AudioSegment> mSourceSegment;
> +
> +  ogg_packet       mOggPacket; /* one raw packet of data for decode */
> +

Two questions
1. Do we really need this data member? Or, it can just be a local variable in GetEncodedTrack
2. Kind of weird to have "ogg" keyword in Vorbis encoder, do we really need this

Comment 24

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

Comment on attachment 819527 [details] [diff] [review]
WIP-bug-883749-vorbisEncoder.v02.patch

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

# Treat warnings as errors in directories with FAIL_ON_WARNINGS.
ac_add_options --enable-warnings-as-errors

::: content/media/encoder/MediaEncoder.cpp
@@ +116,5 @@
> +    //audioEncoder = new OpusTrackEncoder();
> +#endif
> +#ifdef MOZ_VORBIS
> +    printf("MaudioEncoder = new VorbisTrackEncoder()");
> +    audioEncoder = new VorbisTrackEncoder();

Let have another patch for adding this part in Media Encoder.

::: content/media/encoder/VorbisTrackEncoder.cpp
@@ +19,5 @@
> +
> +// http://tools.ietf.org/html/draft-ietf-codec-oggopus-00#section-4
> +// Second paragraph, " The granule position of an audio data page is in units
> +// of PCM audio samples at a fixed rate of 48 kHz."
> +static const int kOpusSamplingRate = 48000;

ah....opus...

@@ +43,5 @@
> +VorbisTrackEncoder::Init(int aChannels, int aSamplingRate)
> +{
> +  if (aChannels <= 0) {
> +    LOG("[Opus] Fail to create the AudioTrackEncoder! The input has"
> +        " %d channel(s), but expects no more than %d.", aChannels, );

Fix this comment.

@@ +67,5 @@
> +    Encoding using a VBR quality mode.  The usable range is -.1
> +    (lowest quality, smallest file) to 1. (highest quality, largest file).
> +    Example quality mode .4: 44kHz stereo coupled, roughly 128kbps VBR
> +
> +    ret = vorbis_encode_init_vbr(&vi,2,44100,.4);

Should we init this parameter by media segment config?

@@ +178,5 @@
> +    // Wait if mEncoder is not initialized, or when not enough raw data, but is
> +    // not the end of stream nor is being canceled.
> +    // TODO: how many sample need to be processed?
> +    printf("%d GetPacketDuration() %d\n",
> +        mRawSegment->GetDuration() + mSourceSegment->GetDuration(),

LOG..

@@ +184,5 @@
> +    while (!mCanceled && (mRawSegment->GetDuration() +
> +           mSourceSegment->GetDuration() < GetPacketDuration() &&
> +           !mEndOfStream)) {
> +      mon.Wait();
> +      printf("VorbisTrackEncoder::GetEncodedTrack wait \n");

LOG

@@ +251,5 @@
> +  /* vorbis does some data preanalysis, then divvies up blocks for
> +      more involved (potentially parallel) processing.  Get a single
> +      block for encoding now */
> +  while (vorbis_analysis_blockout(&mVorbisDsp, &mVorbisBlock) == 1) {
> +    printf("vorbis_analysis_blockout\n");

LOG

::: content/media/encoder/VorbisTrackEncoder.h
@@ +17,5 @@
> +public:
> +  VorbisTrackEncoder();
> +  virtual ~VorbisTrackEncoder();
> +
> +  CommonMetadata* GetMetadata() MOZ_FINAL MOZ_OVERRIDE;

nsRefPtr

Comment 25

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

Comment on attachment 819527 [details] [diff] [review]
WIP-bug-883749-vorbisEncoder.v02.patch

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

::: content/media/encoder/CommonMetadata.h
@@ +56,5 @@
>  // Vorbis meta data require for webM.
>  class VorbisMetadata: public CommonMetadata
>  {
>  public:
> +  virtual VorbisMetadata* AsVorbisMetadata() { return this; }

Remove on  bug 919905

::: content/media/encoder/VorbisTrackEncoder.cpp
@@ +9,5 @@
> +#include "AudioChannelFormat.h"
> +#undef LOG
> +#ifdef MOZ_WIDGET_GONK
> +#include <android/log.h>
> +#define LOG(args...) __android_log_print(ANDROID_LOG_INFO, "MediaEncoder", ## args);

Remove android log, maybe we can use the PRLogModuleInfo

@@ +66,5 @@
> +   /*********************************************************************
> +    Encoding using a VBR quality mode.  The usable range is -.1
> +    (lowest quality, smallest file) to 1. (highest quality, largest file).
> +    Example quality mode .4: 44kHz stereo coupled, roughly 128kbps VBR
> +

declare ret here.

@@ +88,5 @@
> +            vorbis_encode_setup_init(&vi));
> +
> +    *********************************************************************/
> +
> +  ret = vorbis_encode_init_vbr(&mVorbisInfo, mChannels, mSamplingRate, 0.4);

What's 0.4?

@@ +111,5 @@
> +{
> +  return mSamplingRate * kFrameDurationMs / 1000;
> +}
> +
> +CommonMetadata*

Interface changed.

@@ +126,5 @@
> +  if (mCanceled || mDoneEncoding) {
> +    return nullptr;
> +  }
> +
> +  mMeta = new VorbisMetadata();

no mMeta any more..

Comment 26

[Thinker Li [:sinker]]

Benjamin, please name your patches with "WIP" to remind people that don't waste time to review it. XD

Comment 27

[Shelly Lin [:shelly]]

Comment on attachment 819527 [details] [diff] [review]
WIP-bug-883749-vorbisEncoder.v02.patch

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

Hi Benjamin, looks like this is still in WIP? (or not?)

::: content/media/encoder/CommonMetadata.h
@@ +62,5 @@
>    nsTArray<uint8_t> mCodecPrivate;
>    // Sampling frequency in Hz.
>    uint32_t mSamplingFrequency;
>    // Numbers of channels in the track.
>    uint32_t mChannels;

Why do you need mSamplingFrequency and mChannels here again? Aren't they defined in AudioTrackEncoder already?

@@ +64,5 @@
>    uint32_t mSamplingFrequency;
>    // Numbers of channels in the track.
>    uint32_t mChannels;
>    // Bits per sample, mostly used for PCM.
>    uint32_t mBitDepth;

Btw, what are these three parameters declared for (mSamplingFrequency, mChannels, mBitDepth)? I don't see them get used beside being set.

::: content/media/encoder/VorbisTrackEncoder.cpp
@@ +9,5 @@
> +#include "AudioChannelFormat.h"
> +#undef LOG
> +#ifdef MOZ_WIDGET_GONK
> +#include <android/log.h>
> +#define LOG(args...) __android_log_print(ANDROID_LOG_INFO, "MediaEncoder", ## args);

Seems like the module of media encoder is getting bigger, we might want to take bug 899030 into consideration now.

@@ +22,5 @@
> +// of PCM audio samples at a fixed rate of 48 kHz."
> +static const int kOpusSamplingRate = 48000;
> +
> +// The duration of an Opus frame, and it must be 2.5, 5, 10, 20, 40 or 60 ms.
> +static const int kFrameDurationMs  = 20;

Does Vorbis do the same thing?

@@ +86,5 @@
> +    ret = ( vorbis_encode_setup_managed(&vi,2,44100,-1,128000,-1) ||
> +            vorbis_encode_ctl(&vi,OV_ECTL_RATEMANAGE2_SET,NULL) ||
> +            vorbis_encode_setup_init(&vi));
> +
> +    *********************************************************************/

This is a very good documentation, but don't use your own free-style comment. And for comment like this should move it outside the implementation, put it above the Init() function and embrace them with 
/**  
 */

@@ +92,5 @@
> +  ret = vorbis_encode_init_vbr(&mVorbisInfo, mChannels, mSamplingRate, 0.4);
> +
> +  /* add a comment */
> +  vorbis_comment_init(&mVorbisComment);
> +  vorbis_comment_add_tag(&mVorbisComment,"ENCODER","VorbisTrackEncoder.cpp");

Move this to GetMetadata() since it is setting up header for Vorbis.

@@ +161,5 @@
> +         header_comm.packet, header_comm.bytes);
> +  memcpy(vorbisSpecificData.AppendElements(header_code.bytes),
> +         header_code.packet, header_code.bytes);
> +
> +  mMeta->AsVorbisMetadata()->mCodecPrivate = vorbisSpecificData;

(I'm commenting from line 134 to line 165.)
It looks like you are doing two things here: Setting the metadata to container writer, and getting the final container data out from writer, and I don't understand what vorbisSpecificData is for. 

I would suggest you move mVorbisDsp and mVorbisComment (or is there anymore) to VorbisMetadata, and move the above implementation to OggWriter::GetContainerData(), I think it's okay to have OggWriter using |vorbis_analysis_headerout|.

@@ +249,5 @@
> +
> +
> +  /* vorbis does some data preanalysis, then divvies up blocks for
> +      more involved (potentially parallel) processing.  Get a single
> +      block for encoding now */

Nit: Use //.

::: content/media/encoder/VorbisTrackEncoder.h
@@ +33,5 @@
> +   * A local segment queue which stores the raw segments. Opus encoder only
> +   * takes GetPacketDuration() samples from mSourceSegment in every encoding
> +   * cycle, thus it needs to store the raw track data.
> +   */
> +  nsAutoPtr<AudioSegment> mSourceSegment;

Do you need this for Vorbis? If so, please update the comment here.

@@ +35,5 @@
> +   * cycle, thus it needs to store the raw track data.
> +   */
> +  nsAutoPtr<AudioSegment> mSourceSegment;
> +
> +  ogg_packet       mOggPacket; /* one raw packet of data for decode */

Nit: Use 
/**
 */
For documentation, or use // for comment lines.

@@ +40,5 @@
> +
> +  vorbis_info mVorbisInfo;
> +  vorbis_comment mVorbisComment;
> +  vorbis_dsp_state mVorbisDsp; /* central working state for the packet->PCM decoder */
> +  vorbis_block     mVorbisBlock; /* local working space for packet->PCM decode */

Nit: No need to indent.

Comment 28

[Shelly Lin [:shelly]]

Comment on attachment 819527 [details] [diff] [review]
WIP-bug-883749-vorbisEncoder.v02.patch

Put back Thinker's change.

Comment 29

[Monty Montgomery (:xiphmont)]

(In reply to Ralph Giles (:rillian) from comment #20)
> Well, you have our opinions. Monty, can you comment on how long a
> fixed-point vorbis encoder would take?

I agree with derf that it would take about a month, much of that in initial conformance testing and debugging.  The largest pure coding time would be the forward MDCT, which is a relatively known quantity of work. 

The first cut of the Tremor decoder took two weeks; an encoder should be relatively simpler.  I should be able to shake off the rust in a week or so.  So a month sounds right.

As I'm a Moz employee now, Moz gets some additional say in whether or not it's a good use of my time.

Comment 30

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

Nice to hear that. Could we have a bug for this one?

Comment 31

[Ralph Giles (:rillian)]

(In reply to Randy Lin [:rlin] from comment #30)
> Nice to hear that. Could we have a bug for this one?

I filed bug 932390 for the fixed point encoder. I still think we should just use libvorbisenc for this on all platforms.

Comment 32

[Benjamin Chen [:bechen]]

Attachment: WIP-bug-883749-vorbisEncoder.v03.patch

1. modify the comment
2. replace the LOG to PR_LOG
3. remove |nsAutoPtr<AudioSegment> mSourceSegment| and |ogg_packet mOggPacket| from class member
4. handle the EOS case

There still some problems in this patch.
1. vorbis_encode_init_vbr(&mVorbisInfo, mChannels, mSamplingRate, 0.4)
The 0.4 represents the quality and the range -0.1 ~ 1, what number we should set?
2. How many audio sample we should write into encoder per GetEncodedTrack?
Opus use 20ms.

Comment 33

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

Comment on attachment 824426 [details] [diff] [review]
WIP-bug-883749-vorbisEncoder.v03.patch

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

Thanks, Looks better.

::: content/media/encoder/MediaEncoder.cpp
@@ +10,5 @@
>  #include "OggWriter.h"
>  #endif
>  #ifdef MOZ_OPUS
>  #include "OpusTrackEncoder.h"
> +

remove this line.

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

add line....

@@ +16,5 @@
> +        LOG("%p [VorbisTrackEncoder]: " msg, this, ##__VA_ARGS__)
> +#else
> +#define LOG(msg, ...)
> +#define VORBISLOG(msg, ...)
> +#endif

Do we need VORBISLOG? I think we can set the env with NSPR_LOG_MODULES=VorbisTrackEncoder:5 to output to console

@@ +62,5 @@
> +  // used: Encoding using a VBR quality mode. The usable range is -.1
> +  // (lowest quality, smallest file) to 1. (highest quality, largest file).
> +  // Example quality mode .4: 44kHz stereo coupled, roughly 128kbps VBR
> +  // ret = vorbis_encode_init_vbr(&vi,2,44100,.4);
> +  ret = vorbis_encode_init_vbr(&mVorbisInfo, mChannels, mSamplingRate, 0.4);

#DEFINE  BASE_QUALITY 0.4

@@ +76,5 @@
> +  return ret == 0 ? NS_OK : NS_ERROR_FAILURE;
> +}
> +
> +// TODO: how about 50ms data?
> +int

uint32_t ?

@@ +99,5 @@
> +  }
> +
> +  // Vorbis codec specific data
> +  // http://matroska.org/technical/specs/codecid/index.html
> +  VorbisMetadata* meta = new VorbisMetadata();

ASSERT meta

::: content/media/encoder/VorbisTrackEncoder.h
@@ +30,5 @@
> +
> +  nsresult GetEncodedTrack(EncodedFrameContainer& aData) MOZ_FINAL MOZ_OVERRIDE;
> +
> +protected:
> +  int GetPacketDuration() MOZ_FINAL MOZ_OVERRIDE;

document it.

@@ +41,5 @@
> +
> +  vorbis_info mVorbisInfo;
> +  vorbis_comment mVorbisComment;
> +  vorbis_dsp_state mVorbisDsp;
> +  vorbis_block mVorbisBlock;

document those..

Comment 34

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

Comment on attachment 824426 [details] [diff] [review]
WIP-bug-883749-vorbisEncoder.v03.patch

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

::: content/media/encoder/VorbisTrackEncoder.cpp
@@ +144,5 @@
> +{
> +  // vorbis does some data preanalysis, then divvies up blocks for
> +  // more involved (potentially parallel) processing. Get a single
> +  // block for encoding now.
> +  while (vorbis_analysis_blockout(&mVorbisDsp, &mVorbisBlock) == 1) {

Test and found this while loop would run more times on next next GetEncodedFrame(). 
Something wrong.

Comment 35

[u459114]

Comment on attachment 824426 [details] [diff] [review]
WIP-bug-883749-vorbisEncoder.v03.patch

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

::: content/media/encoder/VorbisTrackEncoder.h
@@ +15,5 @@
> +class VorbisMetadata : public TrackMetadataBase
> +{
> +public:
> +  nsTArray<uint8_t> mData;
> +

Container get this mData and put this data into META table without any handling? 
If you have only mData here, Container has no way to know what data that Encoder put into this object.

Comment 36

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

Comment on attachment 810999 [details] [diff] [review]
libvorbis svn r18077bug-883749-update_libvobis.part2.patch

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

This patch need to rebase because there is no media/libvorbis/include/vorbis/moz.build anymore.

Comment 37

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

[u459114]

(In reply to Ivan Tsay (:ITsay) from comment #37)
> 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.
For video recording, there are two choices on B2G 
1. VPX + Vorbis in WebM: software version
2. OmxVideo + OmxAudio in MP4: depend on HW design. Most devices support HW codec in this path.

For B2G, we put focus on enabling the second path.

Comment 39

[Benjamin Chen [:bechen]]

Attachment: bug-883749-vorbisEncoder.v03.patch

sync to latest mc.

Comment 40

[Benjamin Chen [:bechen]]

Comment on attachment 832821 [details] [diff] [review]
bug-883749-vorbisEncoder.v03.patch

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

Comment 41

[Jason Smith [:jsmith]]

(In reply to Benjamin Chen [:bechen] from comment #40)
> Comment on attachment 832821 [details] [diff] [review]
> bug-883749-vorbisEncoder.v03.patch
> 
> Hi Ralph:
> Would you please help to review this patch?

This really needs tests included on the patch.

Comment 42

[Ralph Giles (:rillian)]

Comment on attachment 832821 [details] [diff] [review]
bug-883749-vorbisEncoder.v03.patch

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

Looks good in general. Please address comments and re-submit.

::: content/media/encoder/VorbisTrackEncoder.cpp
@@ +49,5 @@
> +
> +nsresult
> +VorbisTrackEncoder::Init(int aChannels, int aSamplingRate)
> +{
> +  if (aChannels <= 0) {

Please reject aChannels > 8 as well.

@@ +68,5 @@
> +                               BASE_QUALITY);
> +
> +  // Set up the analysis state and auxiliary encoding storage
> +  vorbis_analysis_init(&mVorbisDsp, &mVorbisInfo);
> +  vorbis_block_init(&mVorbisDsp, &mVorbisBlock);

I am slightly worried that these are called even if vorbis_encode_init_vbr() fails. Looking at the code I think it is probably alright, but it may allocate unnecessary memory if mVorbisInfo has strange data.

It would be safer to track these and only call the corresponding _clear functions in the dtor if this call succeeds.

@@ +102,5 @@
> +  // Add comment
> +  vorbis_comment vorbisComment;
> +  vorbis_comment_init(&vorbisComment);
> +  vorbis_comment_add_tag(&vorbisComment, "ENCODER",
> +                         "Mozilla VorbisTrackEncoder.cpp");

I would leave off the .cpp, but please include MOZ_APP_UA_VERSION.

@@ +114,5 @@
> +  while (lacing > 255) {
> +    lacing -= 255;
> +    meta->mData.AppendElement(255);
> +  }
> +  meta->mData.AppendElement((uint8_t)lacing);

Can this be WriteLacing(meta->mData, header.bytes) or similar? Please use a helper function to avoid duplicating the lacing generator code below.

@@ +125,5 @@
> +  meta->mData.AppendElement((uint8_t)lacing);
> +
> +  // Append the three packets
> +  memcpy(meta->mData.AppendElements(header.bytes), header.packet,
> +         header.bytes);

Neat, it never occurred to me you could write to the pointer returned by AppendElements().

However, I think you can do:

meta->mData.AppendElements(header.packet, header.bytes);

Which achieves the same thing more succinctly and may avoid an unnecessary initialization depending on what the nsTArray Constructor for 'uint8_t' does.

@@ +142,5 @@
> +  // more involved (potentially parallel) processing. Get a single
> +  // block for encoding now.
> +  while (vorbis_analysis_blockout(&mVorbisDsp, &mVorbisBlock) == 1) {
> +    ogg_packet oggPacket;
> +    vorbis_analysis(&mVorbisBlock, &oggPacket);

Please check the return value here. It can only fail on memory corruption, so PR_LOG an error and return early.

@@ +148,5 @@
> +    EncodedFrame* audiodata = new EncodedFrame();
> +    audiodata->SetFrameType(EncodedFrame::AUDIO_FRAME);
> +    nsTArray<uint8_t> frameData;
> +    frameData.SetLength(oggPacket.bytes);
> +    memcpy(frameData.Elements(), oggPacket.packet, oggPacket.bytes);

frameData.AppendElements(oggPacket.packet, oggPacket.bytes);

That takes care of the SetLength for you as well.

@@ +183,5 @@
> +  if (mEndOfStream && (sourceSegment->GetDuration() == 0)) {
> +    mDoneEncoding = true;
> +    VORBISLOG("[Vorbis] Done encoding.");
> +    vorbis_analysis_wrote(&mVorbisDsp, 0);
> +    GetEncodedFrame(aData);

Can you return early here, so the main body of the code doesn't have to be inside the 'else' clause?

@@ +189,5 @@
> +    // Start encoding data.
> +    AudioSegment::ChunkIterator iter(*sourceSegment);
> +
> +    AudioDataValue **vorbisBuffer =
> +      vorbis_analysis_buffer(&mVorbisDsp, (int)sourceSegment->GetDuration());

I guess this will fail to compile if AudioDataValue is an int16_t?

How big can sourceSegment->GetDuration() be?

@@ +191,5 @@
> +
> +    AudioDataValue **vorbisBuffer =
> +      vorbis_analysis_buffer(&mVorbisDsp, (int)sourceSegment->GetDuration());
> +
> +    int frameCopied = 0;

framesCopied (plural)

@@ +192,5 @@
> +    AudioDataValue **vorbisBuffer =
> +      vorbis_analysis_buffer(&mVorbisDsp, (int)sourceSegment->GetDuration());
> +
> +    int frameCopied = 0;
> +    nsAutoTArray<AudioDataValue, 9600> pcm;

Where does the 9600 come from?

@@ +202,5 @@
> +        InterleaveTrackData(chunk, frameToCopy, mChannels,
> +                            pcm.Elements() + frameCopied * mChannels);
> +      } else { // empty data
> +        memset(pcm.Elements() + frameCopied * mChannels, 0,
> +               frameToCopy * mChannels * sizeof(AudioDataValue));

pcm.Clear()?

@@ +207,5 @@
> +      }
> +      frameCopied += frameToCopy;
> +      iter.Next();
> +    }
> +    // de-interleave the pcm

// De-interleave the pcm.

Please make this AudioTrackEncoder::DeinterleaveTrackData().

@@ +214,5 @@
> +        vorbisBuffer[i][j] = pcm.ElementAt(i + j * mChannels);
> +      }
> +    }
> +    // Now the vorbisBuffer contain the all data in non-interleaved.
> +    // Tell the library how much we actually submitted

Period at the end of the sentence, please.

@@ +219,5 @@
> +    vorbis_analysis_wrote(&mVorbisDsp, frameCopied);
> +    VORBISLOG("vorbis_analysis_wrote frameCopied %d\n", frameCopied);
> +    GetEncodedFrame(aData);
> +
> +    if (mEndOfStream) {

Can this be removed, relying on the same clause at the top to finish the stream on the next call?

::: content/media/encoder/VorbisTrackEncoder.h
@@ +33,5 @@
> +protected:
> +  // http://xiph.org/vorbis/doc/libvorbis/vorbis_analysis_buffer.html
> +  // 1024 is a reasonable choice.
> +  int GetPacketDuration() MOZ_FINAL MOZ_OVERRIDE {
> +    return 1024;

This is a bit confusing. Vorbis won't actually use this packet duration, but it will allocate space for the requested number of samples, section the submitted audio and encode from that buffer. Vorbis in fact uses variable duration packets. Perhaps the comment could say:

// We use 1024 samples for the write buffer; libvorbis will
// construct packets with the appropriate duration for the
// encoding mode internally.

@@ +40,5 @@
> +  nsresult Init(int aChannels, int aSamplingRate) MOZ_FINAL MOZ_OVERRIDE;
> +
> +private:
> +  // Get the encoded data from vorbis encoder and add into aData.
> +  void GetEncodedFrame(EncodedFrameContainer& aData);

This should be GetEncodedFrames. (plural)

@@ +45,5 @@
> +
> +  // vorbis codec members
> +  // Struct that stores all the static vorbis bitstream settings.
> +  vorbis_info mVorbisInfo;
> +  // Central working state for the packet->PCM decoder.

This comment should be changed to refer PCM->packet encoder.

@@ +47,5 @@
> +  // Struct that stores all the static vorbis bitstream settings.
> +  vorbis_info mVorbisInfo;
> +  // Central working state for the packet->PCM decoder.
> +  vorbis_dsp_state mVorbisDsp;
> +  // Local working space for packet->PCM decode.

This one too.

Comment 43

[Benjamin Chen [:bechen]]

(In reply to Ralph Giles (:rillian) from comment #42)
> Comment on attachment 832821 [details] [diff] [review]
> bug-883749-vorbisEncoder.v03.patch
> 
> Review of attachment 832821 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good in general. Please address comments and re-submit.
> 
> ::: content/media/encoder/VorbisTrackEncoder.cpp
> @@ +49,5 @@
> > +
> > +nsresult
> > +VorbisTrackEncoder::Init(int aChannels, int aSamplingRate)
> > +{
> > +  if (aChannels <= 0) {
> 
> Please reject aChannels > 8 as well.
> 
> @@ +68,5 @@
> > +                               BASE_QUALITY);
> > +
> > +  // Set up the analysis state and auxiliary encoding storage
> > +  vorbis_analysis_init(&mVorbisDsp, &mVorbisInfo);
> > +  vorbis_block_init(&mVorbisDsp, &mVorbisBlock);
> 
> I am slightly worried that these are called even if vorbis_encode_init_vbr()
> fails. Looking at the code I think it is probably alright, but it may
> allocate unnecessary memory if mVorbisInfo has strange data.
> 
> It would be safer to track these and only call the corresponding _clear
> functions in the dtor if this call succeeds.
> 
> @@ +102,5 @@
> > +  // Add comment
> > +  vorbis_comment vorbisComment;
> > +  vorbis_comment_init(&vorbisComment);
> > +  vorbis_comment_add_tag(&vorbisComment, "ENCODER",
> > +                         "Mozilla VorbisTrackEncoder.cpp");
> 
> I would leave off the .cpp, but please include MOZ_APP_UA_VERSION.
> 
> @@ +114,5 @@
> > +  while (lacing > 255) {
> > +    lacing -= 255;
> > +    meta->mData.AppendElement(255);
> > +  }
> > +  meta->mData.AppendElement((uint8_t)lacing);
> 
> Can this be WriteLacing(meta->mData, header.bytes) or similar? Please use a
> helper function to avoid duplicating the lacing generator code below.
> 
> @@ +125,5 @@
> > +  meta->mData.AppendElement((uint8_t)lacing);
> > +
> > +  // Append the three packets
> > +  memcpy(meta->mData.AppendElements(header.bytes), header.packet,
> > +         header.bytes);
> 
> Neat, it never occurred to me you could write to the pointer returned by
> AppendElements().
> 
> However, I think you can do:
> 
> meta->mData.AppendElements(header.packet, header.bytes);
> 
> Which achieves the same thing more succinctly and may avoid an unnecessary
> initialization depending on what the nsTArray Constructor for 'uint8_t' does.
> 
> @@ +142,5 @@
> > +  // more involved (potentially parallel) processing. Get a single
> > +  // block for encoding now.
> > +  while (vorbis_analysis_blockout(&mVorbisDsp, &mVorbisBlock) == 1) {
> > +    ogg_packet oggPacket;
> > +    vorbis_analysis(&mVorbisBlock, &oggPacket);
> 
> Please check the return value here. It can only fail on memory corruption,
> so PR_LOG an error and return early.
> 
> @@ +148,5 @@
> > +    EncodedFrame* audiodata = new EncodedFrame();
> > +    audiodata->SetFrameType(EncodedFrame::AUDIO_FRAME);
> > +    nsTArray<uint8_t> frameData;
> > +    frameData.SetLength(oggPacket.bytes);
> > +    memcpy(frameData.Elements(), oggPacket.packet, oggPacket.bytes);
> 
> frameData.AppendElements(oggPacket.packet, oggPacket.bytes);
> 
> That takes care of the SetLength for you as well.
> 
> @@ +183,5 @@
> > +  if (mEndOfStream && (sourceSegment->GetDuration() == 0)) {
> > +    mDoneEncoding = true;
> > +    VORBISLOG("[Vorbis] Done encoding.");
> > +    vorbis_analysis_wrote(&mVorbisDsp, 0);
> > +    GetEncodedFrame(aData);
> 
> Can you return early here, so the main body of the code doesn't have to be
> inside the 'else' clause?
> 
> @@ +189,5 @@
> > +    // Start encoding data.
> > +    AudioSegment::ChunkIterator iter(*sourceSegment);
> > +
> > +    AudioDataValue **vorbisBuffer =
> > +      vorbis_analysis_buffer(&mVorbisDsp, (int)sourceSegment->GetDuration());
> 
> I guess this will fail to compile if AudioDataValue is an int16_t?
For desktop build, AudioDataValue  is float only, right?

> 
> How big can sourceSegment->GetDuration() be?
> 
It depends on the frequency of calling GetEncodedTrack().
In normal case, there should be a thread keeping pull data via GetEncodedTrack(). So the duration of sourceSegment is usually a little more than GetPacketDuration() (1024 for now).

> 
> @@ +192,5 @@
> > +    AudioDataValue **vorbisBuffer =
> > +      vorbis_analysis_buffer(&mVorbisDsp, (int)sourceSegment->GetDuration());
> > +
> > +    int frameCopied = 0;
> > +    nsAutoTArray<AudioDataValue, 9600> pcm;
> 
> Where does the 9600 come from?
> 

I copy it from OpusTrackEncoder.
I think it's better to set it to "2048 * number of channel" to ensure that the whole data can store in inline storage. (2048 >GetPacketDuration 1024 )

Comment 44

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

[Benjamin Chen [:bechen]]

Attachment: bug-883749-vorbisEncoder.v04.patch

sync to latest codebase. r=rillian
1. add AudioTrackEncoder::DeInterleaveTrackData
2. address comments.

Comment 46

[u459114]

Hi Benjamin, what are we waiting for?

Comment 47

[Benjamin Chen [:bechen]]

(In reply to C.J. Ku[:CJKu] from comment #46)
> Hi Benjamin, what are we waiting for?

I think the vorbis and vp8 patches should be landed with webm muxer because it is the only way to access the object.
tryserver : error C2220: warning treated as error - no 'object' file generated

Comment 48

[Benjamin Chen [:bechen]]

Attachment: bug-883749-vorbisEncoder.v04.patch

Fix tryserver error.

Comment 49

[Benjamin Chen [:bechen]]

Attachment: bug-883749-vorbisEncoder.v05.patch

1. let DeInterleaveTrackData as static fuinction.
2. fix segmentation fault when access the buffer returned by vorbis_analysis_buffer.