Add metadata, encode frame, EOS testing.

Comment on attachment 8403837 [details] [diff] [review] bug-951043.v02.patch Review of attachment 8403837 [details] [diff] [review]: ----------------------------------------------------------------- Looks good!! ::: content/media/gtest/TestVorbisTrackEncoder.cpp @@ +69,5 @@ > +} > + > +// Test init function > +TEST(VorbisTrackEncoder, Init) > +{ // Channel number range test @@ +79,5 @@ > + // Should accept channels within valid range. > + for (int i = 1; i <= 8; i++) { > + EXPECT_TRUE(TestVorbisInit(i, 16000)); > + } > + // Sample rate range test @@ +95,5 @@ > + encoder.TestVorbisCreation(1, 44100); > + > + nsRefPtr<TrackMetadataBase> meta = encoder.GetMetadata(); > + nsRefPtr<VorbisMetadata> vorbisMetadata(static_cast<VorbisMetadata*>(meta.get())); > + // According to initialization parameters, verify the correctness of VorbisMetaData @@ +108,5 @@ > + // Initiate vorbis encoder > + TestVorbisTrackEncoder encoder; > + encoder.TestVorbisCreation(1, 44100); > + > + AudioSegment segment; Move #112 to #115 @@ +124,5 @@ > + > + // Track change notification. > + encoder.NotifyQueuedTrackChanges(nullptr, 0, 0, 0, 0, segment); > + > + // Pull Encoded data back from encoder. // Verify encoded sample

Comment on attachment 8408911 [details] [diff] [review] bug-951043.v02.patch Review of attachment 8408911 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, but some issues with the lacing parser. Please address comments and re-submit. ::: content/media/gtest/TestVorbisTrackEncoder.cpp @@ +4,5 @@ > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +#include "gtest/gtest.h" > +#include "VorbisTrackEncoder.h" > +#include "WebMWriter.h" Don't need WebMWriter.h. @@ +29,5 @@ > + return encoder.TestVorbisCreation(aChannels, aSamplingRate); > +} > + > +static int > +ReadLacing(const uint8_t* aInput, uint8_t *aReadBytes) int& aReadBytes. Or uint32_t if you prefer. Matroska xiph-lacing fields can be up to 2^55 bytes. @@ +34,5 @@ > +{ > + const uint8_t *ptr = aInput; > + *aReadBytes = 0; > + > + int i = 0; Please use a more descriptive name here. 'packetSize' or 'length', etc. instead of 'i'. It's not an iterator. @@ +35,5 @@ > + const uint8_t *ptr = aInput; > + *aReadBytes = 0; > + > + int i = 0; > + while (*ptr == 255) { Pass in the size of aInput (or an end pointer) and also terminate the loop if we get to the end of valid data. If aReadBytes is only 8 bits, this loop can easily overflow it. It might make sense to bound it at some reasonable number, but that number is more that 256. We don't want to read off the end of the array either way. @@ +41,5 @@ > + ptr++; > + (*aReadBytes)++; > + } > + (*aReadBytes)++; > + i += *ptr; Having both ptr and aReadBytes here is redundant. Using aInput[aReadBytes] would be more clear. @@ +46,5 @@ > + > + return i; > +} > + > +static bool parseVorbisMetadata(nsTArray<uint8_t> &aData) nsTArray<uint8_t>& aData @@ +61,5 @@ > + header_length = ReadLacing(aData.Elements()+offset, &readbytes); > + offset += readbytes; > + header_comm_length = ReadLacing(aData.Elements()+offset, &readbytes); > + offset += readbytes; > + if (aData.Length() < (offset + header_length + header_comm_length)) { The third 'setup' header is included in the remaining bytes of the element data. I'm not sure what the minimum size of that header is, but it looks like it should be at least 32 bytes. I'd add that to the check as well. @@ +64,5 @@ > + offset += readbytes; > + if (aData.Length() < (offset + header_length + header_comm_length)) { > + return false; > + } > + return true; Having parsed out the headers, you can verify their internal structure by configuring a decoder with them. Call vorbis_synthesis_headerin() three times, each time passing in a successive packet. See https://xiph.org/vorbis/doc/libvorbis/vorbis_synthesis_headerin.html and related links for details. @@ +85,5 @@ > + // Sample rate range test. > + // Expect false with negative sampling rate of input signal. > + EXPECT_FALSE(TestVorbisInit(1, -1)); > + > + EXPECT_FALSE(TestVorbisInit(1, 200000 + 1)); Please verify standard sample rates are accepted. e.g. 8k, 12k, 16k, 22050, 24k, 32, 44100, 48k, 96k. @@ +116,5 @@ > + // Generate 1 second samples. > + // Reference PeerConnectionMedia.h::Fake_AudioGenerator > + nsRefPtr<mozilla::SharedBuffer> samples = > + mozilla::SharedBuffer::Create(44100 * sizeof(int16_t)); > + int16_t* data = static_cast<int16_t*>(samples->Data()); Will this work on desktop? Does this need to be an AudioDataValue so it works with float samples? @@ +117,5 @@ > + // Reference PeerConnectionMedia.h::Fake_AudioGenerator > + nsRefPtr<mozilla::SharedBuffer> samples = > + mozilla::SharedBuffer::Create(44100 * sizeof(int16_t)); > + int16_t* data = static_cast<int16_t*>(samples->Data()); > + for (int i=0; i<44100; i++) { Use a variable for the buffer size instead of repeating a constant. @@ +123,5 @@ > + } > + nsAutoTArray<const int16_t*,1> channelData; > + channelData.AppendElement(data); > + AudioSegment segment; > + segment.AppendFrames(samples.forget(), channelData, 44100); And here. @@ +156,5 @@ > + MediaStreamListener::TRACK_EVENT_ENDED, > + segment); > + > + // Pull Encoded Data back from encoder. Since we had send out > + // EOS to encoder, encoder.GetEncodedTrack shoould return should

Address comments. r=rillian try server :https://tbpl.mozilla.org/?tree=Try&rev=7f66af5595bf

Fix try server build break. +#include "WebMWriter.h" for |VorbisMetadata| try server: https://tbpl.mozilla.org/?tree=Try&rev=2587f7c321ff

Fix build break again. +#include "MediaStreamGraph.h" for MediaStreamListener::TRACK_EVENT_ENDED try server: https://tbpl.mozilla.org/?tree=Try&rev=d9bf0f0ddf80

sorry had to backout for test failures like https://tbpl.mozilla.org/php/getParsedLog.php?id=39468712&tree=Mozilla-Inbound

Fix the gtest fail: 1. Explicit initialize the structure |ogg_packet|. 2. Raise the flag |b_o_s|. https://tbpl.mozilla.org/?tree=Try&rev=0db7a658022a try server: https://tbpl.mozilla.org/?tree=Try&rev=dacc7b29a39a