Mpeg2 TS - Fail when no valid timestamp in the ADTS parser.

media/formats/mp2t/es_parser_adts_unittest.cc

Index: media/formats/mp2t/es_parser_adts_unittest.cc
diff --git a/media/formats/mp2t/es_parser_adts_unittest.cc b/media/formats/mp2t/es_parser_adts_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..0fc65ff8f251c826c9bccc68dc5564f72fa0f427
--- /dev/null
+++ b/media/formats/mp2t/es_parser_adts_unittest.cc
@@ -0,0 +1,161 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <algorithm>
+#include <vector>
+
+#include "base/bind.h"
+#include "base/command_line.h"
+#include "base/files/memory_mapped_file.h"
+#include "base/logging.h"
+#include "base/path_service.h"
+#include "base/time/time.h"
+#include "media/base/buffers.h"
+#include "media/base/stream_parser_buffer.h"
+#include "media/base/test_data_util.h"
+#include "media/formats/mp2t/es_parser_adts.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace media {
+class AudioDecoderConfig;
+
+namespace mp2t {
+
+namespace {
+
+struct Packet {

[wolenetz, 2014/07/15 21:59:57]

Some of this (struct Packet, void ComputePacketSize, and various utility methods
in the test class) looks very much in common with es_parser_h264_unittest.cc.
ILTM like it would a good idea to split out a common test header/class, possible
as a common superclass of EsParserH264Test and EsParserAdtsTest.

[damienv1, 2014/07/15 22:22:40]

Will start an es_parser_test_helper.*

+  // Offset in the stream.
+  size_t offset;
+
+  // Size of the packet.
+  size_t size;
+
+  // Timestamp of the packet.
+  base::TimeDelta pts;
+};
+
+// Compute the size of each packet assuming packets are given in stream order
+// and the last packet covers the end of the stream.
+void ComputePacketSize(std::vector<Packet>& packets, size_t stream_size) {

[wolenetz, 2014/07/15 21:59:57]

lint nit: Is this a non-const reference? If so, make const or use a pointer.

[damienv1, 2014/07/15 22:22:40]

I made it a reference since otherwise packets[k] does not have the same meaning.
In that case, I'll have to do sth like: packets->at(k) which becomes less
readable.

[wolenetz, 2014/07/15 22:39:21]

non-const ref violates
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Refere...

Would using a const pointer, and sth like: (*packets)[k] work? (And add some
ASSERT_TRUE(packets) to protect against null |packets| deref...)

+  for (size_t k = 0; k < packets.size() - 1; k++) {
+    DCHECK_GE(packets[k + 1].offset, packets[k].offset);
+    packets[k].size = packets[k + 1].offset - packets[k].offset;
+  }
+  packets[packets.size() - 1].size =
+      stream_size - packets[packets.size() - 1].offset;
+}
+
+// Generate fixed size PES packets for t
+std::vector<Packet> GenerateFixedSizePesPacket(
+    const std::vector<uint8>& stream, size_t pes_size) {
+  std::vector<Packet> pes_packets;
+
+  Packet cur_pes_packet;
+  cur_pes_packet.offset = 0;
+  cur_pes_packet.pts = kNoTimestamp();
+  while (cur_pes_packet.offset < stream.size()) {
+    pes_packets.push_back(cur_pes_packet);
+    cur_pes_packet.offset += pes_size;
+  }
+  ComputePacketSize(pes_packets, stream.size());
+
+  return pes_packets;
+}
+
+}  // namespace
+
+class EsParserAdtsTest : public testing::Test {
+ public:
+  EsParserAdtsTest();
+  virtual ~EsParserAdtsTest() {}
+
+ protected:
+  void LoadStream(const char* filename);
+  bool ProcessPesPackets(const std::vector<Packet>& pes_packets,
+                         bool force_timing);
+
+  // ADTS stream.
+  std::vector<uint8> stream_;
+
+  // Number of different audio configs in the ADTS stream.
+  size_t config_count_;
+
+  // Number of buffers generated while parsing the ADTS stream.
+  size_t buffer_count_;
+
+ private:
+  void NewAudioConfig(const AudioDecoderConfig& config);
+  void EmitBuffer(scoped_refptr<StreamParserBuffer> buffer);
+
+  DISALLOW_COPY_AND_ASSIGN(EsParserAdtsTest);
+};
+
+EsParserAdtsTest::EsParserAdtsTest()
+    : config_count_(0),
+      buffer_count_(0) {
+}
+
+void EsParserAdtsTest::LoadStream(const char* filename) {
+  base::FilePath file_path = GetTestDataFilePath(filename);
+
+  base::MemoryMappedFile stream;
+  ASSERT_TRUE(stream.Initialize(file_path))
+      << "Couldn't open stream file: " << file_path.MaybeAsASCII();
+
+  stream_.resize(stream.length());
+  memcpy(stream_.data(), stream.data(), stream_.size());
+}
+
+bool EsParserAdtsTest::ProcessPesPackets(
+    const std::vector<Packet>& pes_packets,
+    bool force_timing) {
+  EsParserAdts es_parser(
+      base::Bind(&EsParserAdtsTest::NewAudioConfig, base::Unretained(this)),
+      base::Bind(&EsParserAdtsTest::EmitBuffer, base::Unretained(this)),
+      false);
+
+  for (size_t k = 0; k < pes_packets.size(); k++) {
+    size_t cur_pes_offset = pes_packets[k].offset;
+    size_t cur_pes_size = pes_packets[k].size;
+
+    base::TimeDelta pts = kNoTimestamp();
+    base::TimeDelta dts = kNoTimestamp();
+    if (pes_packets[k].pts >= base::TimeDelta() || force_timing)
+      pts = pes_packets[k].pts;
+
+    if (!es_parser.Parse(&stream_[cur_pes_offset], cur_pes_size, pts, dts))
+      return false;
+  }
+  es_parser.Flush();
+  return true;
+}
+
+void EsParserAdtsTest::NewAudioConfig(const AudioDecoderConfig& config) {
+  config_count_++;
+}
+
+void EsParserAdtsTest::EmitBuffer(scoped_refptr<StreamParserBuffer> buffer) {
+  buffer_count_++;
+}
+
+TEST_F(EsParserAdtsTest, NoInitialPts) {
+  LoadStream("bear.adts");
+  std::vector<Packet> pes_packets = GenerateFixedSizePesPacket(stream_, 512);
+  EXPECT_FALSE(ProcessPesPackets(pes_packets, false));

[wolenetz, 2014/07/15 21:59:57]

To verify the first condition added in this change ("don't emit a buffer if no
valid pts yet"), EXPECT_EQ(1u, config_count_); and EXPECT_EQ(0u, buffer_count_);
after confirming failure of ProcessPesPackets() here?

[damienv1, 2014/07/15 22:22:40]

I'll check the buffers.
On the other side, whether the implementation emits the corresponding audio
config is an implementation detail (we could have an implementation which does
not emit the audio config when the frame is not valid from a timestamp
perspective).

[wolenetz, 2014/07/15 22:39:21]

Acknowledged.

+}
+

[wolenetz, 2014/07/15 21:59:56]

Is it possible to distinctly verify the second condition added in this change
("don't GetTimestamp() a kNoTimestamp()-as-base_timestamp() audio helper when
updating audio config")? IMO, this would be a lower pri test versus your
"NoInitialPts" test.

+TEST_F(EsParserAdtsTest, SinglePts) {
+  LoadStream("bear.adts");
+
+  std::vector<Packet> pes_packets = GenerateFixedSizePesPacket(stream_, 512);
+  pes_packets.front().pts = base::TimeDelta::FromSeconds(10);
+
+  EXPECT_TRUE(ProcessPesPackets(pes_packets, false));
+  EXPECT_EQ(config_count_, 1u);

[wolenetz, 2014/07/15 21:59:57]

nit: here and elsewhere in this file, EXPECT_EQ(<expectation>, <actual>) per
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...

[damienv1, 2014/07/15 22:22:40]

Done.

+  EXPECT_EQ(buffer_count_, 45u);
+}
+
+}  // namespace mp2t
+}  // namespace media
+