[MediaEncoder] Implement Vorbis encoding

RESOLVED DUPLICATE of bug 891705

Status

()

Core
Audio/Video: Recording
RESOLVED DUPLICATE of bug 891705
5 years ago
4 years ago

People

(Reporter: u459114, Assigned: bechen)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla27
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [ft:multimedia-platform])

Attachments

(1 attachment, 9 obsolete attachments)

Comment hidden (empty)
(Reporter)

Updated

5 years ago
Blocks: 879688
I would suggest separate bugs for the vorbis and mp3 implementations; they'll be quite different.
Er, unless you're planning to use android libraries for both?
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.
(Reporter)

Updated

5 years ago
Summary: [MediaEncoder] Implement Vorbis/ MP3 encoding format → [MediaEncoder] Implement Vorbis encoding
(Reporter)

Updated

5 years ago
Assignee: nobody → bechen
(Reporter)

Comment 4

5 years ago
Support Vobis audio sample encoding on all platforms, except B2G
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.
(Reporter)

Comment 6

5 years ago
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.
(Reporter)

Updated

5 years ago
Blocks: 912380
(Reporter)

Updated

5 years ago
Blocks: 914104

Updated

5 years ago
No longer blocks: 912380
Blocks: 879669
No longer blocks: 879688
(Assignee)

Comment 7

5 years ago
Created attachment 810997 [details] [diff] [review]
WIP-bug-883749-update_libvobis.part1.patch

Add files from libvorbis svn r18077 for encoder.
(Assignee)

Comment 8

5 years ago
Created attachment 810999 [details] [diff] [review]
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.
(Assignee)

Comment 9

5 years ago
Created attachment 811001 [details] [diff] [review]
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.
Great, I will start to integrate this with WebM 1 muxer.
(Assignee)

Comment 11

5 years ago
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?
Flags: needinfo?(tterribe)
Flags: needinfo?(giles)
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.
Flags: needinfo?(giles)
(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.
Flags: needinfo?(tterribe)
(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
hmm, Let us try the encoder performance on arm device.
(Assignee)

Comment 16

5 years ago
Created attachment 811789 [details] [diff] [review]
WIP:bug-883749-vorbisEncoder.v01.patch
Attachment #811001 - Attachment is obsolete: true
It seems vorbisEncoder already package the encode data in ogg container, 
We should let oggwriter to do this job.
(Assignee)

Comment 18

5 years ago
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%.
(Assignee)

Comment 19

5 years ago
(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.
Well, you have our opinions. Monty, can you comment on how long a fixed-point vorbis encoder would take?
Flags: needinfo?(monty)
(Assignee)

Comment 21

5 years ago
Created attachment 819527 [details] [diff] [review]
WIP-bug-883749-vorbisEncoder.v02.patch

sync to bug 919905 latest patch.
Attachment #811789 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 929910
(Reporter)

Comment 22

5 years ago
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
Attachment #819527 - Flags: feedback?(slin)
Attachment #819527 - Flags: feedback?(rlin)
(Reporter)

Comment 23

5 years ago
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 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 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..

Updated

5 years ago
Attachment #810997 - Attachment description: bug-883749-update_libvobis.part1.patch → WIP-bug-883749-update_libvobis.part1.patch

Updated

5 years ago
Attachment #819527 - Attachment description: bug-883749-vorbisEncoder.v02.patch → WIP-bug-883749-vorbisEncoder.v02.patch
Benjamin, please name your patches with "WIP" to remind people that don't waste time to review it. XD
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.
Attachment #819527 - Attachment description: WIP-bug-883749-vorbisEncoder.v02.patch → bug-883749-vorbisEncoder.v02.patch
Comment on attachment 819527 [details] [diff] [review]
WIP-bug-883749-vorbisEncoder.v02.patch

Put back Thinker's change.
Attachment #819527 - Attachment description: bug-883749-vorbisEncoder.v02.patch → WIP-bug-883749-vorbisEncoder.v02.patch
(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.
Flags: needinfo?(monty)
Nice to hear that. Could we have a bug for this one?

Updated

5 years ago
blocking-b2g: --- → 1.3+
Whiteboard: [ft:media-recording]

Updated

5 years ago
No longer blocks: 879669
Depends on: 879669

Updated

5 years ago
Blocks: 923038

Updated

5 years ago
Target Milestone: --- → 1.3 Sprint 5 - 11/22
(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.
(Assignee)

Comment 32

5 years ago
Created attachment 824426 [details] [diff] [review]
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.
Attachment #819527 - Attachment is obsolete: true
Attachment #819527 - Flags: feedback?(slin)
Attachment #819527 - Flags: feedback?(rlin)
Attachment #824426 - Flags: feedback?(slin)
Attachment #824426 - Flags: feedback?(rlin)
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..
Attachment #824426 - Flags: feedback?(rlin) → feedback+
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.
(Reporter)

Comment 35

5 years ago
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 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.

Updated

5 years ago
Blocks: 879688

Comment 37

5 years ago
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.
No longer blocks: 923038
blocking-b2g: 1.3+ → ---
Target Milestone: 1.3 Sprint 5 - 11/22 → mozilla27
(Reporter)

Comment 38

5 years ago
(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.
(Assignee)

Comment 39

5 years ago
Created attachment 832821 [details] [diff] [review]
bug-883749-vorbisEncoder.v03.patch

sync to latest mc.
Attachment #810997 - Attachment is obsolete: true
Attachment #810999 - Attachment is obsolete: true
Attachment #824426 - Attachment is obsolete: true
Attachment #824426 - Flags: feedback?(slin)
(Assignee)

Comment 40

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

Hi Ralph:
Would you please help to review this patch?
Attachment #832821 - Flags: review?(giles)
(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 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.
Attachment #832821 - Flags: review?(giles) → review+
(Assignee)

Comment 43

5 years ago
(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 )
Hi Benjamin, 
Please merge with shelly patch and make it build pass, thanks a lot.
Flags: needinfo?(bechen)
(Assignee)

Comment 45

5 years ago
Created attachment 8341622 [details] [diff] [review]
bug-883749-vorbisEncoder.v04.patch

sync to latest codebase. r=rillian
1. add AudioTrackEncoder::DeInterleaveTrackData
2. address comments.
Attachment #832821 - Attachment is obsolete: true
Attachment #8341622 - Flags: review+
Flags: needinfo?(bechen)
Whiteboard: [ft:media-recording] → [ft:multimedia-platform]
(Reporter)

Comment 46

5 years ago
Hi Benjamin, what are we waiting for?
Flags: needinfo?(bechen)
(Assignee)

Comment 47

5 years ago
(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
Flags: needinfo?(bechen)
(Assignee)

Comment 48

5 years ago
Created attachment 8351304 [details] [diff] [review]
bug-883749-vorbisEncoder.v04.patch

Fix tryserver error.
Attachment #8341622 - Attachment is obsolete: true
Attachment #8351304 - Flags: review+
(Assignee)

Comment 49

5 years ago
Created attachment 8360830 [details] [diff] [review]
bug-883749-vorbisEncoder.v05.patch

1. let DeInterleaveTrackData as static fuinction.
2. fix segmentation fault when access the buffer returned by vorbis_analysis_buffer.
Attachment #8351304 - Attachment is obsolete: true
Attachment #8360830 - Flags: review+
(Assignee)

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 891705
Component: Video/Audio → Video/Audio: Recording
You need to log in before you can comment on or make changes to this bug.