Should test the following member functions: virtual nsresult Init(int aWidth, int aHeight, TrackRate aTrackRate) ; NotifyQueuedTrackChanges + GetEncodedTrack: feed a fake image with correct and error frame resolution. reference patch https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=936981&attachment=8356408

WIP

WIP

Attach last patch

Comment on attachment 8388435 [details] [diff] [review] WIP VP8TrackEncoder Test case Review of attachment 8388435 [details] [diff] [review]: ----------------------------------------------------------------- Hi Benjamin/ John, feedback need, thanks

Comment on attachment 8388435 [details] [diff] [review] WIP VP8TrackEncoder Test case Review of attachment 8388435 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/media/encoder/moz.build @@ +15,1 @@ > 'TrackEncoder.h', Where is the file 'MutedFrameGenerator.h'? I didn't see it in the patch. And should we put it under some condition flag? like |if CONFIG['XXX']| ::: content/media/gtest/TestVideoTrackEncoder.cpp @@ +41,5 @@ > + const uint32_t yPlanSize = aSize.width * aSize.height; > + const uint32_t halfWidth = (aSize.width + 1) / 2; > + const uint32_t halfHeight = (aSize.height + 1) / 2; > + const uint32_t uvPlanSize = halfWidth * halfHeight; > + yPlaneSize, uvPlaneSize.

keep a note: HW decoder on NEXUS5 generates YV12 YUV format.

Hi Ralph, Please help to review this patch. Hi gps, build config change, need you feedback, thanks

Comment on attachment 8391049 [details] [diff] [review] VP8TrackEncoder Test case Review of attachment 8391049 [details] [diff] [review]: ----------------------------------------------------------------- You don't need build peer review you modify source lists in moz.build files.

Comment on attachment 8391049 [details] [diff] [review] VP8TrackEncoder Test case Review of attachment 8391049 [details] [diff] [review]: ----------------------------------------------------------------- Sorry to take so long responding; I was on vacation. r=me with the mSourceBuffer size rounding issue addressed. ::: content/media/gtest/TestVideoTrackEncoder.cpp @@ +24,5 @@ > + { > + mImageSize = aSize; > + > + int yPlaneLen = aSize.width * aSize.height; > + int cbcrPlaneLen = yPlaneLen / 2; I believe you need to round up the CbCr dimensions before calculating the data length to have enough mSourceBuffer elements to handle odd sizes in the Generate() call. @@ +155,5 @@ > + data.mCbCrStride = mImageSize.width; > + data.mCbCrSize.width = halfWidth; > + data.mCbCrSize.height = halfHeight; > + > + image->SetData(data); Just to confirm, SetData makes a copy of mSourceBuffer's contents, so it can safely outlive the generator? @@ +165,5 @@ > + nsTArray<uint8_t> mSourceBuffer; > +}; > + > +struct InitParam { > + bool mShouldSuccess; // This parameter should cause success or fail result bool mValid; // Whether Init() should succeed with these parameters. Other possible names are mExpect or mShouldSucceed. mShouldSuccess is confusing because 'should' takes a participle (or verb) and with a noun the meaning is unclear. @@ +196,5 @@ > +// Init test > +TEST(VideoTrackEncoder, Initialization) > +{ > + InitParam params[] = { > + // Falied cases. // Failure cases. @@ +201,5 @@ > + { false, 640, 480, 0 }, // Trackrate should be larger than 1. > + { false, 640, 480, -1 }, // Trackrate should be larger than 1. > + { false, 0, 0, 90000 }, // Height/ width should be larger than 1. > + { false, 0, 1, 90000 }, // Height/ width should be larger than 1. > + { false, 1, 0, 90000}, // Height/ width should be larger than 1. We reject images greater than 4000x3000 for playback. Maybe we should check that we can't create such videos as well? See MAX_VIDEO_WIDTH, MAX_VIDEO_HEIGHT, and IsValidVideoRegion() in VideoUtils. @@ +210,5 @@ > + { true, 960, 540, 90000}, // Standard qHD > + { true, 1280, 720, 90000} // Standard HD > + }; > + > + for (size_t i = 0; i < sizeof(params)/ sizeof(InitParam); i++) Please use mfbt::ArrayLength(params) for the loop bound. @@ +230,5 @@ > + nsRefPtr<VP8Metadata> vp8Meta(static_cast<VP8Metadata*>(meta.get())); > + > + // METADATA should be depend on how to initiate encoder. > + EXPECT_TRUE(vp8Meta->mWidth == 640); > + EXPECT_TRUE(vp8Meta->mHeight == 480); Would be nice to verify this for all the successful initializations in the previous test. @@ +278,5 @@ > + encoder.NotifyQueuedTrackChanges(nullptr, 0, 0, 0, MediaStreamListener::TRACK_EVENT_ENDED, segment); > + > + // Pull Encoded Data back from encoder. Since we had send out > + // EOS to encoder, encoder.GetEncodedTrack shoould return > + // NS_OK immidiately. ... Since we have sent EOS // to the encoder, encoder.GetEncodedTrack should return // NS_OK immediately.

(In reply to Ralph Giles (:rillian) from comment #14) > Sorry to take so long responding; I was on vacation. r=me with the > mSourceBuffer size rounding issue addressed. No problem, thanks for your review. > ::: content/media/gtest/TestVideoTrackEncoder.cpp > @@ +24,5 @@ > > + { > > + mImageSize = aSize; > > + > > + int yPlaneLen = aSize.width * aSize.height; > > + int cbcrPlaneLen = yPlaneLen / 2; > > I believe you need to round up the CbCr dimensions before calculating the > data length to have enough mSourceBuffer elements to handle odd sizes in the > Generate() call. Will do > @@ +155,5 @@ > > + data.mCbCrStride = mImageSize.width; > > + data.mCbCrSize.width = halfWidth; > > + data.mCbCrSize.height = halfHeight; > > + > > + image->SetData(data); > > Just to confirm, SetData makes a copy of mSourceBuffer's contents, so it can > safely outlive the generator? Yes. > @@ +165,5 @@ > > + nsTArray<uint8_t> mSourceBuffer; > > +}; > > + > > +struct InitParam { > > + bool mShouldSuccess; // This parameter should cause success or fail result > > bool mValid; // Whether Init() should succeed with these parameters. > > Other possible names are mExpect or mShouldSucceed. mShouldSuccess is > confusing because 'should' takes a participle (or verb) and with a noun the > meaning is unclear. will do > @@ +196,5 @@ > > +// Init test > > +TEST(VideoTrackEncoder, Initialization) > > +{ > > + InitParam params[] = { > > + // Falied cases. > > // Failure cases. > > @@ +201,5 @@ > > + { false, 640, 480, 0 }, // Trackrate should be larger than 1. > > + { false, 640, 480, -1 }, // Trackrate should be larger than 1. > > + { false, 0, 0, 90000 }, // Height/ width should be larger than 1. > > + { false, 0, 1, 90000 }, // Height/ width should be larger than 1. > > + { false, 1, 0, 90000}, // Height/ width should be larger than 1. > > We reject images greater than 4000x3000 for playback. Maybe we should check > that we can't create such videos as well? > > See MAX_VIDEO_WIDTH, MAX_VIDEO_HEIGHT, and IsValidVideoRegion() in > VideoUtils. We did not set this upper bound in track encoder. I will file an issue for this limitation. > @@ +210,5 @@ > > + { true, 960, 540, 90000}, // Standard qHD > > + { true, 1280, 720, 90000} // Standard HD > > + }; > > + > > + for (size_t i = 0; i < sizeof(params)/ sizeof(InitParam); i++) > > Please use mfbt::ArrayLength(params) for the loop bound. Thanks :), will do > @@ +230,5 @@ > > + nsRefPtr<VP8Metadata> vp8Meta(static_cast<VP8Metadata*>(meta.get())); > > + > > + // METADATA should be depend on how to initiate encoder. > > + EXPECT_TRUE(vp8Meta->mWidth == 640); > > + EXPECT_TRUE(vp8Meta->mHeight == 480); > > Would be nice to verify this for all the successful initializations in the > previous test. Will do > @@ +278,5 @@ > > + encoder.NotifyQueuedTrackChanges(nullptr, 0, 0, 0, MediaStreamListener::TRACK_EVENT_ENDED, segment); > > + > > + // Pull Encoded Data back from encoder. Since we had send out > > + // EOS to encoder, encoder.GetEncodedTrack shoould return > > + // NS_OK immidiately. > > ... Since we have sent EOS > // to the encoder, encoder.GetEncodedTrack should return > // NS_OK immediately. Thanks...tense is...

According to giles's review, fix typo and take suggestion. Carry r+

TryServer result https://tbpl.mozilla.org/?tree=Try&rev=7b3b469c45d3

Remove trailer space and enable test case base on platform. Carry r+

Sign... attach wrong patch. Again

Try server result https://tbpl.mozilla.org/?tree=Try&rev=ff0d60a17e7a

Fix namespace compile error on some platform Try server result https://tbpl.mozilla.org/?tree=Try&rev=fc2b1827e48d

Fix debug build break and carry r+