Closed Bug 936981 Opened 8 years ago Closed 8 years ago

[MediaEncoder::Gtest] Implement unit test case of OpusTrackEncoder

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: u459114, Assigned: shelly)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

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
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
Attached patch gtest.patch (obsolete) — Splinter Review
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
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
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)
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)
Added the missing files back.
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 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 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+
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?
Flags: needinfo?(giles)
Attachment #8349837 - Attachment is obsolete: true
Attachment #8350525 - Flags: review?(giles)
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?
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.
(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)
(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 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+
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 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+
Revised w/ nit fix and carry+ from Ralph.
Attachment #8351091 - Attachment is obsolete: true
Attachment #8356408 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/52a40373eff3
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
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 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.
Component: Video/Audio → Video/Audio: Recording
You need to log in before you can comment on or make changes to this bug.