Comment on attachment 8406702 [details] [diff] [review] bug-951040.gtestWebM.v01.patch Alfredo, please also help to feedback this test case, thanks

Comment on attachment 8406702 [details] [diff] [review] bug-951040.gtestWebM.v01.patch Review of attachment 8406702 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/gtest/TestWebMWriter.cpp @@ +75,5 @@ > + EncodedFrame::FrameType aFrameType, > + uint64_t aTimeStamp) > +{ > + nsTArray<uint8_t> frameData; > + EncodedFrame *videoData = new EncodedFrame(); It should be ref counted, right? If yes, it'd be better to use nsRefPtr. @@ +151,5 @@ > + writer.WriteEncodedTrack(encodedVideoData, 0); > + // Should have data because the second cluster is closed. > + writer.GetContainerData(&encodedBuf, 0); > + EXPECT_TRUE(encodedBuf.Length() > 0); > + } Above tests all have similar codes, it'd be better to rewrite them with a function like: TestEncoder(frame type, duration, expected_result) { ... } And call the function in different test cases: TestEncoder(EncodedFrame::VP8_P_FRAME, 2000000, 0); TestEncoder(EncodedFrame::VP8_I_FRAME, 3000000, 1); ... But since this is just testcase, I'm ok for that.

Comment on attachment 8406702 [details] [diff] [review] bug-951040.gtestWebM.v01.patch Review of attachment 8406702 [details] [diff] [review]: ----------------------------------------------------------------- Overall, looks good. 1. Please add more negative test(see comments) 2. Please add EOS test. ::: content/media/gtest/TestWebMWriter.cpp @@ +17,5 @@ > + { > + if (Init(aChannels, aSamplingRate) == NS_OK) { > + return true; > + } > + return false; if (NS_FAILED(Init(aChannels, aSamplingRate))) { return false; } return true; @@ +18,5 @@ > + if (Init(aChannels, aSamplingRate) == NS_OK) { > + return true; > + } > + return false; > + } void SetContainerMetadata(WebMWriter &writer,int aChannels, int aSamplingRate) { EXPECT_TRUE(TestVorbisCreation(aChannels, aSamplingRate)); nsRefPtr<TrackMetadataBase> meta = vorbisEncoder.GetMetadata(); writer.SetMetadata(meta); } And make TestVorbisCreation private @@ +32,5 @@ > + aTrackRate) == NS_OK) { > + return true; > + } > + return false; > + } void SetContainerMetadata(WebMWriter &writer,int32_t aWidth, int32_t aHeight, int32_t aDisplayWidth, int32_t aDisplayHeight,TrackRate aTrackRate) { EXPECT_TRUE(TestVP8Creation(aWidth, aHeight, aDisplayWidth, aDisplayHeight, aTrackRate)); nsRefPtr<TrackMetadataBase> meta = vp8Encoder.GetMetadata(); writer.SetMetadata(meta); } and make TestVP8Creation private @@ +42,5 @@ > + ContainerWriter::CREATE_VIDEO_TRACK); > + > + // The output should be empty since we didn't set any metadata in writer. > + nsTArray<nsTArray<uint8_t> > encodedBuf; > + writer.GetContainerData(&encodedBuf, ContainerWriter::GET_HEADER); In this case, since no header data at all, should GetContainerData return error code? EXPECT_TRUE(NS_FAILED(writer.GetContainerData(&encodedBuf, ContainerWriter::GET_HEADER))); This is an open question. @@ +51,5 @@ > + // Set vorbis metadata. > + WebMVorbisTrackEncoder vorbisEncoder; > + EXPECT_TRUE(vorbisEncoder.TestVorbisCreation(1, 44100)); > + nsRefPtr<TrackMetadataBase> vorbisMeta = vorbisEncoder.GetMetadata(); > + writer.SetMetadata(vorbisMeta); vorbisEncoder.SetContainerMetadata(1, 44100); @@ +65,5 @@ > + WebMVP8TrackEncoder vp8Encoder; > + EXPECT_TRUE(vp8Encoder.TestVP8Creation(640, 480, 640, 480, 90000)); > + nsRefPtr<TrackMetadataBase> vp8Meta = vp8Encoder.GetMetadata(); > + writer.SetMetadata(vp8Meta); > + vp8Encoder.SetContainerMetadata(640, 480, 640, 480, 90000); @@ +67,5 @@ > + nsRefPtr<TrackMetadataBase> vp8Meta = vp8Encoder.GetMetadata(); > + writer.SetMetadata(vp8Meta); > + > + writer.GetContainerData(&encodedBuf, ContainerWriter::GET_HEADER); > + EXPECT_TRUE(encodedBuf.Length() > 0); Is there a minimum size according to the spec(not your implementation)? @@ +72,5 @@ > +} > + > +static void createEncodedFrameContainer(EncodedFrameContainer *aOutput, > + EncodedFrame::FrameType aFrameType, > + uint64_t aTimeStamp) rename to AppendDummyFrameIntoContainer @@ +76,5 @@ > + uint64_t aTimeStamp) > +{ > + nsTArray<uint8_t> frameData; > + EncodedFrame *videoData = new EncodedFrame(); > + frameData.SetLength(500); Do a negative test by setting frameData.SetLength(0). In this case, container writer need to return error or skip that invalid data. @@ +78,5 @@ > + nsTArray<uint8_t> frameData; > + EncodedFrame *videoData = new EncodedFrame(); > + frameData.SetLength(500); > + videoData->SetFrameType(aFrameType); > + videoData->SetTimeStamp(aTimeStamp); Do a negative test by out of order time stamp given. Container writer need to handle this situation correctly. If not, file an issue @@ +95,5 @@ > + // Set vorbis metadata. > + WebMVorbisTrackEncoder vorbisEncoder; > + EXPECT_TRUE(vorbisEncoder.TestVorbisCreation(1, 44100)); > + nsRefPtr<TrackMetadataBase> vorbisMeta = vorbisEncoder.GetMetadata(); > + writer.SetMetadata(vorbisMeta); vorbisEncoder.SetContainerMetadata(1, 44100); Append an empty line @@ +100,5 @@ > + // Set vp8 metadata > + WebMVP8TrackEncoder vp8Encoder; > + EXPECT_TRUE(vp8Encoder.TestVP8Creation(640, 480, 640, 480, 90000)); > + nsRefPtr<TrackMetadataBase> vp8Meta = vp8Encoder.GetMetadata(); > + writer.SetMetadata(vp8Meta); vp8Encoder.SetContainerMetadata(640, 480, 640, 480, 90000); @@ +106,5 @@ > + writer.GetContainerData(&encodedBuf, ContainerWriter::GET_HEADER); > + EXPECT_TRUE(encodedBuf.Length() > 0); > + encodedBuf.Clear(); > + > + // write the first I-Frame. // Append the first raw frame into container, and set it as I-Frame. @@ +107,5 @@ > + EXPECT_TRUE(encodedBuf.Length() > 0); > + encodedBuf.Clear(); > + > + // write the first I-Frame. > + { Why need this {}? @@ +125,5 @@ > + createEncodedFrameContainer(&encodedVideoData, > + EncodedFrame::VP8_I_FRAME, > + 1000000); > + writer.WriteEncodedTrack(encodedVideoData, 0); > + // Should have data because the first cluster is closed. Explain why the first cluster is closed. The definition of close is not clear @@ +164,5 @@ > + // Set vorbis metadata. > + WebMVorbisTrackEncoder vorbisEncoder; > + EXPECT_TRUE(vorbisEncoder.TestVorbisCreation(1, 44100)); > + nsRefPtr<TrackMetadataBase> vorbisMeta = vorbisEncoder.GetMetadata(); > + writer.SetMetadata(vorbisMeta); vorbisEncoder.SetContainerMetadata(1, 44100); @@ +170,5 @@ > + WebMVP8TrackEncoder vp8Encoder; > + EXPECT_TRUE(vp8Encoder.TestVP8Creation(640, 480, 640, 480, 90000)); > + nsRefPtr<TrackMetadataBase> vp8Meta = vp8Encoder.GetMetadata(); > + writer.SetMetadata(vp8Meta); > + vp8Encoder.SetContainerMetadata(640, 480, 640, 480, 90000);

I will try to add test case for Bug 970774 and Bug 999364 on the next version

1. add Bug 999364 testcase - by raw memory operation to find the cluster length. 2. add Bug 970774 testcase - use nestegg to verify the metadata

Comment on attachment 8427567 [details] [diff] [review] bug-951040.gtestWebM.v03.patch Review of attachment 8427567 [details] [diff] [review]: ----------------------------------------------------------------- Too many static functions. In my view, I will try to avoid static function as much possible in cpp. ::: content/media/gtest/TestWebMWriter.cpp @@ +107,5 @@ > + EXPECT_TRUE(encodedBuf.Length() > 0); > +} > + > +static > +void AppendDummyFrameIntoContainer(EncodedFrameContainer *aOutput, Any reason to use a static function here? Can it be a routine in a class? My my view, it will less readable if there are a lot of static functions. @@ +131,5 @@ > + AppendDummyFrameIntoContainer(&encodedVideoData, > + EncodedFrame::VP8_I_FRAME, > + aTimeStamp); > + WriteEncodedTrack(encodedVideoData, 0); > +} Why not just add a AddDummyFrame() in TestWebMWriter? Then we don't need this function and AppendDummyFrameIntoContainer(). The calling sequence could be straightforward like: writer.AddDummyFrame(VP_I_FRAME) writer.AddDummyFrame(VP_B_FRAME) @@ +141,5 @@ > + AppendDummyFrameIntoContainer(&encodedVideoData, > + EncodedFrame::VP8_P_FRAME, > + aTimeStamp); > + WriteEncodedTrack(encodedVideoData, 0); > +} Same as above. @@ +301,5 @@ > + } > + > + // http://matroska.org/technical/specs/index.html > + // Find the first cluster starts from pattern "1f 43 b6 75". > + uint8_t clusterId[4] ={0x1f,0x43,0xb6,0x75}; nit @@ +305,5 @@ > + uint8_t clusterId[4] ={0x1f,0x43,0xb6,0x75}; > + uint8_t* firstCluster = nullptr; > + uint8_t* secondCluster = nullptr; > + for (uint32_t i = 0; i < flatEncodedBuf.Length() - 4; i++) { > + if (memcmp(flatEncodedBuf.Elements()+i, clusterId, 4) == 0) { Since clusterId is not a dynamic array, how about sizeof(clusterId) instead of 4? @@ +316,5 @@ > + uint64_t length = 0; > + // Skip the first byte, it must be "0x01" > + // length = (*(first+4+1)<< 28) + (*(first+4+2)<< 24) + (*(first+4+3)<< 20) + > + // (*(first+4+4)<< 16) + (*(first+4+5)<< 12) + (*(first+4+6)<< 8) + > + // (*(first+4+7)<< 4)+(*(first+4+8)); Is this byte-order format change? If yes, mozilla got a convenient way to do it. https://blog.mozilla.org/nfroyd/2013/04/11/introducing-mozillaendian-h/. @@ +327,5 @@ > + EXPECT_TRUE(flatEncodedBuf.Length() >= > + (firstCluster-flatEncodedBuf.Elements()) + length + 12 + 4); > + secondCluster = firstCluster + length + 12; > + // Check the second cluster "1f 43 b6 75" > + EXPECT_TRUE(memcmp(secondCluster, clusterId, 4) == 0); Same above ::: content/media/gtest/moz.build @@ +17,3 @@ > ] > > + Remove empty line.

Comment on attachment 8427567 [details] [diff] [review] bug-951040.gtestWebM.v03.patch Review of attachment 8427567 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/gtest/TestWebMWriter.cpp @@ +390,5 @@ > + NS_ASSERTION(aUserData, "aUserData must point to a valid WebMioData"); > + WebMioData* ioData = reinterpret_cast<WebMioData*>(aUserData); > + return ioData->offset; > +} > + Can we move bug970774_aspect_ratio into another bug? keep this one to do fundamental testing.

Comment on attachment 8427567 [details] [diff] [review] bug-951040.gtestWebM.v03.patch Review of attachment 8427567 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/gtest/TestWebMWriter.cpp @@ +131,5 @@ > + AppendDummyFrameIntoContainer(&encodedVideoData, > + EncodedFrame::VP8_I_FRAME, > + aTimeStamp); > + WriteEncodedTrack(encodedVideoData, 0); > +} Define TestWebMWriter::AppendIFrameIntoWriter in class definition or stick it beneath class definition. @@ +151,5 @@ > + GetContainerData(&encodedBuf, 0); > + if (encodedBuf.Length()) { > + return true; > + } > + return false; return (encodedBuf.Length() > 0) ? true :false; @@ +260,5 @@ > + // Have data because the previous cluster is closed. > + EXPECT_TRUE(writer.HaveValidCluster()); > +} > + > +TEST(WebMWriter, bug999364_cluster_length) The same, suggest move bug999364_cluster_length into another patch/bug.

Address comments. Move other 2 testcases to other bug.

Comment on attachment 8428593 [details] [diff] [review] bug-951040.gtestWebM.v03.patch Review of attachment 8428593 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for writing tests! r=me with the timestamp comment addressed. ::: content/media/gtest/TestWebMWriter.cpp @@ +71,5 @@ > + // Create dummy frame data. > + frameData.SetLength(FIXED_FRAMESIZE); > + videoData->SetFrameType(aFrameType); > + videoData->SetTimeStamp(aTimeStamp); > + videoData->SetDuration(FIXED_DURATION); Duration is fixed, but aTimestamp is passed in. The passed in value for aTimestamp is always an accumulator the caller is maintaining separately. Since you don't need to know the actual timestamp for testing purposes, please move the accumulator to TestWebMWriter::mTimestamp, and either pass the duration to AppendDummyFrame() instead or pass only the frame type and use a fixed duration as you do here. That should make the test code easier to read. @@ +110,5 @@ > + EXPECT_TRUE(encodedBuf.Length() == 0); > + > + // Set vp8 metadata > + int32_t width = 640; > + int32_t height = 480; These values repeat in every test. Should be constants too? or vary with each test. @@ +169,5 @@ > + // Should have data because the second cluster is closed. > + EXPECT_TRUE(writer.HaveValidCluster()); > +} > + > +TEST(WebMWriter, FLUSH_NEEDED) Seems like this could be combined with the Cluster test, but I guess it might be helpful to test the first cluster with different call sequences.

r=rillian try server: https://tbpl.mozilla.org/?tree=Try&rev=48bcd049a9bc

fix nits. push try server again: https://tbpl.mozilla.org/?tree=Try&rev=e820425c6b9e