Closed
Bug 936981
Opened 11 years ago
Closed 11 years ago
[MediaEncoder::Gtest] Implement unit test case of OpusTrackEncoder
Categories
(Core :: Audio/Video: Recording, defect)
Core
Audio/Video: Recording
Tracking
()
RESOLVED
FIXED
mozilla29
People
(Reporter: u459114, Assigned: shelly)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 5 obsolete files)
8.88 KB,
patch
|
shelly
:
review+
|
Details | Diff | Splinter Review |
We have more and more components in MediaRecorder modules.
1. TrackEncoder instances - video(vpx/ omx) audio(opus/vorbis/ omx)
2. Container writer instances - MP4 muxer/ WebM muxer
mochitest is a good testing framework for user scenario test, but we need a unit test case framework to test each component separately.
1. TrackEncoder unit test
Mock source streams with different channels/ rate to verify correctness of each encoder.
2. ContainerWriter unit test
META data test
Mock encoded frame test.
Assignee: nobody → rlin
Summary: MediaRecorder TestHarness framework → Construct MediaRecorder GTest framework
Updated•11 years ago
|
Summary: Construct MediaRecorder GTest framework → Construct MediaEncoder GTest framework
Let's do this. After you build up things we need to writing C++ native test case,
alfredo write webM test cases
randy write MP4 test cases
john write omxvideo and vp8 test cases
shelly write omxaudio and vorbis test cases
benjamin write media encoder test cases
Summary: Construct MediaEncoder GTest framework → Build up MediaEncoder GTest framework
Comment 3•11 years ago
|
||
Apply this patch,
You can build by ./mach build,
Run by ./mach gtest
debug by MOZ_RUN_GTEST=1 gdb obj-x86_64-unknown-linux-gnu/dist/bin/firefox-bin
refer to: https://developer.mozilla.org/en-US/docs/GTest
Comment 4•11 years ago
|
||
Retitle to be Test case for OpusTrackEncoder.
First stage can just test initialization and metadata test.
Assignee: rlin → slin
Summary: Build up MediaEncoder GTest framework → Test case for OpusTrackEncoder
Assignee | ||
Comment 5•11 years ago
|
||
Hi Jason, I've setup the GTest under content/media, and added a test case for OpusTrackEncoder, do you know who is the best fit to review this patch? Thanks.
Attachment #8334831 -
Attachment is obsolete: true
Attachment #8349345 -
Flags: feedback?(jsmith)
Assignee | ||
Comment 6•11 years ago
|
||
Comment on attachment 8349345 [details] [diff] [review]
Setup GTest for content/media, and add a gtest for OpusTrackEncoder
I actually forgot to add the new files, and JUST PURGED THEM!!!!!!!!!!!!!!!!!
Attachment #8349345 -
Attachment is obsolete: true
Attachment #8349345 -
Flags: feedback?(jsmith)
Assignee | ||
Comment 7•11 years ago
|
||
Attachment #8349837 -
Flags: feedback?(jsmith)
Assignee | ||
Comment 8•11 years ago
|
||
Added the missing files back.
Comment 9•11 years ago
|
||
Comment on attachment 8349837 [details] [diff] [review]
Setup GTest for content/media, and add a gtest for OpusTrackEncoder
Hmm...so for gtests, I'm probably not the right reviewer for these tests. Let me redirect to people that I think could help provide feedback here.
* gps - marking you for feedback for build config changes
* rillian - marking you for feedback on the opus track encoder changes & tests
Attachment #8349837 -
Flags: feedback?(jsmith)
Attachment #8349837 -
Flags: feedback?(gps)
Attachment #8349837 -
Flags: feedback?(giles)
Summary: Test case for OpusTrackEncoder → [MediaEncoder::Gtest] Implement unit test case of OpusTrackEncoder
Comment 10•11 years ago
|
||
Comment on attachment 8349837 [details] [diff] [review]
Setup GTest for content/media, and add a gtest for OpusTrackEncoder
Review of attachment 8349837 [details] [diff] [review]:
-----------------------------------------------------------------
Looks fine to me. FWIW I'd prefer content/media/gtest to content/media/test/gtest.
::: content/media/encoder/TrackEncoder.h
@@ +15,5 @@
> #include "VideoSegment.h"
>
> namespace mozilla {
>
> +#define MAX_SUPPORTED_AUDIO_CHANNELS 8
OpusTrackEncoder uses AudioChannelsDownMix, which implements the matricies from the webaudio spec. In particular it drops anything past the first six channels and treats the remainder is 5.1. This check still makes sense to establish expectations for Opus encoding, which only has surround mappings up to 8 channels.
::: content/media/test/gtest/TestTrackEncoder.cpp
@@ +54,5 @@
> + EXPECT_TRUE(TestOpusInit(1, 16000));
> + EXPECT_TRUE(TestOpusInit(1, 24000));
> + EXPECT_TRUE(TestOpusInit(1, 48000));
> +
> + // We re-sample the sampling rate if it is none of the above.
You could compare with GetOutputSampleRate() to better verify which rates trigger resampling.
Attachment #8349837 -
Flags: feedback?(giles) → feedback+
Comment 11•11 years ago
|
||
Comment on attachment 8349837 [details] [diff] [review]
Setup GTest for content/media, and add a gtest for OpusTrackEncoder
Review of attachment 8349837 [details] [diff] [review]:
-----------------------------------------------------------------
Build system stuff is fine. If we keep writing gtests, we'll probably want a better way to express gtest libraries in moz.build. Actually, this could probably happen today. But I'm not going to bloat scope. I imagine glandium will likely clean this all up in due time anyway.
Attachment #8349837 -
Flags: feedback?(gps) → feedback+
Assignee | ||
Comment 12•11 years ago
|
||
Comment on attachment 8349837 [details] [diff] [review]
Setup GTest for content/media, and add a gtest for OpusTrackEncoder
Review of attachment 8349837 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/media/encoder/TrackEncoder.h
@@ +15,5 @@
> #include "VideoSegment.h"
>
> namespace mozilla {
>
> +#define MAX_SUPPORTED_AUDIO_CHANNELS 8
Thanks for all the feedback :)
I wasn't sure on setting a constraint on maximum input channels, I remember someone told me it should be 8 but I couldn't found the comment now. So should it be a check specific to Opus encoder? or an overall constraint to other codec encoder as well?
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(giles)
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #8349837 -
Attachment is obsolete: true
Attachment #8350525 -
Flags: review?(giles)
Reporter | ||
Comment 14•11 years ago
|
||
Comment on attachment 8350525 [details] [diff] [review]
Setup GTest for content/media, and add a gtest for OpusTrackEncoder, v2
Review of attachment 8350525 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/media/gtest/TestTrackEncoder.cpp
@@ +29,5 @@
> + {
> + return mInitialized ? GetOutputSampleRate() : 0;
> + }
> +};
> +
Open question:
Is it good to inherit OpusTrackEncoder and intrusively access private data?
Assignee | ||
Comment 15•11 years ago
|
||
Comment on attachment 8350525 [details] [diff] [review]
Setup GTest for content/media, and add a gtest for OpusTrackEncoder, v2
Review of attachment 8350525 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/media/encoder/OpusTrackEncoder.h
@@ +47,2 @@
> */
> int GetOutputSampleRate();
I made this function protected instead of private. Although it is for testing purpose, but I don't think the change harms the class structure anyway.
Comment 16•11 years ago
|
||
(In reply to Shelly Lin [:shelly] from comment #12)
> I wasn't sure on setting a constraint on maximum input channels, I remember
> someone told me it should be 8 but I couldn't found the comment now. So
> should it be a check specific to Opus encoder? or an overall constraint to
> other codec encoder as well?
There are a couple of choices here. The Opus format supports surround up to 8 channels. It supports multitrack audio (like individual voice+instrument tracks without any specific speaker arrangement) up to 255 channels. The actually implementation currently supports only mono or stereo, and downmixes any more than that. I think 8 is a reasonable compromise based on what the code currently handles.
A bigger question is whether your test and implementation should share the same define. It's always safer to duplicate limits so the test is more likely the catch unintentional changes...but it makes for more work when you change the implementation. In this case I would probably duplicate the maximum, since 8 is a format limit independent of the implementation, but I don't feel strongly about it.
Flags: needinfo?(giles)
Comment 17•11 years ago
|
||
(In reply to C.J. Ku[:CJKu] from comment #14)
> Open question:
> Is it good to inherit OpusTrackEncoder and intrusively access private data?
On the other side, I thought it was pretty clever. Unit tests often need access to object internals to verify correct behaviour, so inheriting and making relevant calls protected seems like a reasonable solution to me.
Comment 18•11 years ago
|
||
Comment on attachment 8350525 [details] [diff] [review]
Setup GTest for content/media, and add a gtest for OpusTrackEncoder, v2
Review of attachment 8350525 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/media/gtest/TestTrackEncoder.cpp
@@ +41,5 @@
> +static int
> +TestOpusResampler(int aChannels, int aSamplingRate)
> +{
> + TestOpusTrackEncoder encoder;
> + encoder.TestOpusCreation(aChannels, aSamplingRate);
I think you need to EXPECT_TRUE on this return value too, to keep the same tests you had in the previous patch. Actually, I suppose TestGetOutputSampleRate will still fail to match in the later tests, but the error will be less diagnostic.
Attachment #8350525 -
Flags: review?(giles) → review+
Assignee | ||
Comment 19•11 years ago
|
||
Thanks for the full explanations, no big change here, I've moved the definition of max channels to be a local in OpusTrackEncoder, we can always change it later if we want to make it global, but a safer implementation does sound better.
Have a nice vacation :)
Attachment #8350525 -
Attachment is obsolete: true
Attachment #8351091 -
Flags: review?(giles)
Comment 20•11 years ago
|
||
Comment on attachment 8351091 [details] [diff] [review]
Correct the checking of input sampling rate, v3
Review of attachment 8351091 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks! r=me with grammar fix.
::: content/media/gtest/TestTrackEncoder.cpp
@@ +10,5 @@
> +
> +class TestOpusTrackEncoder : public OpusTrackEncoder
> +{
> +public:
> + // Return true if it has successfully initialize the Opus encoder.
'initialized'
Attachment #8351091 -
Flags: review?(giles) → review+
Assignee | ||
Comment 21•11 years ago
|
||
Revised w/ nit fix and carry+ from Ralph.
Attachment #8351091 -
Attachment is obsolete: true
Attachment #8356408 -
Flags: review+
Assignee | ||
Comment 22•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 23•11 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Comment 24•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Comment 25•11 years ago
|
||
Comment on attachment 8356408 [details] [diff] [review]
Unit test for Opus encoder
Review of attachment 8356408 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/media/gtest/TestTrackEncoder.cpp
@@ +79,5 @@
> + EXPECT_TRUE(TestOpusResampler(1, 24000) == 24000);
> + EXPECT_TRUE(TestOpusResampler(1, 48000) == 48000);
> +
> + // Otherwise, it should be resampled to 48kHz by resampler.
> + EXPECT_FALSE(TestOpusResampler(1, 9600) == 9600);
You can say EXPECT_TRUE(TestOpusResampler(1, 9600) == 48000); to make sure resampled frequency is actually 48kHz.
Comment 26•11 years ago
|
||
Comment on attachment 8356408 [details] [diff] [review]
Unit test for Opus encoder
Review of attachment 8356408 [details] [diff] [review]:
-----------------------------------------------------------------
::: content/media/gtest/TestTrackEncoder.cpp
@@ +79,5 @@
> + EXPECT_TRUE(TestOpusResampler(1, 24000) == 24000);
> + EXPECT_TRUE(TestOpusResampler(1, 48000) == 48000);
> +
> + // Otherwise, it should be resampled to 48kHz by resampler.
> + EXPECT_FALSE(TestOpusResampler(1, 9600) == 9600);
I see. If the resampled frequency is not necessarily 48kHz according to the spec, we should just check the output sample rate is different the input one.
Updated•11 years ago
|
Component: Video/Audio → Video/Audio: Recording
You need to log in
before you can comment on or make changes to this bug.
Description
•