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

media/formats/mp2t/es_parser_test_helper.cc

Index: media/formats/mp2t/es_parser_test_helper.cc
diff --git a/media/formats/mp2t/es_parser_test_helper.cc b/media/formats/mp2t/es_parser_test_helper.cc
new file mode 100644
index 0000000000000000000000000000000000000000..96ebae7e3c634a04fda662d448e6122aa5bed4f8
--- /dev/null
+++ b/media/formats/mp2t/es_parser_test_helper.cc
@@ -0,0 +1,56 @@
+// 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 "media/formats/mp2t/es_parser_test_helper.h"
+
+#include "media/base/buffers.h"
+#include "media/formats/mp2t/es_parser.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace media {
+namespace mp2t {
+
+Packet::Packet()
+    : offset(0u),
+      size(0u) {

[wolenetz, 2014/07/16 00:21:03]

I'm not sure, but you might want to default-initialize packet PTS to
kNoTimestamp().

[damienv1, 2014/07/16 02:34:33]

Done.

+}
+
+void ComputePacketSize(std::vector<Packet>* packets, size_t stream_size) {

[wolenetz, 2014/07/16 00:21:03]

nit: Following convention in media/base/test_helpers.h, append "ForTest" to
these function names (and s/Packet/TestPacket/) that are test-only but in the
media::mp2t namespace?

[damienv1, 2014/07/16 02:34:33]

In fact, I checked at this file and not all functions/class/struct are
prefixed/suffixed with Test or ForTest.
I don't know what is the rule here either (Seems like adding Test for everything
might make the code a bit less readable).

+  ASSERT_TRUE(packets);
+  if (packets->size() == 0u)
+    return;
+
+  Packet* cur = &packets->at(0);

[wolenetz, 2014/07/16 00:21:03]

nit: I'm not convinced you need to use at(k) or at(k+1) instead of just [k] or
[k+1], eg:
Packet* cur = &packets->front();
(or)
Packet* cur = &(*packets)[0];
...
Packet* next = &(*packets)[k+1];

In both cases, & has lower precedence than ->, (), [].

[damienv1, 2014/07/16 02:34:33]

Done.

+  for (size_t k = 0; k < packets->size() - 1; k++) {
+    Packet* next = &packets->at(k + 1);
+    DCHECK_GE(next->offset, cur->offset);
+    cur->size = next->offset - cur->offset;
+    cur = next;
+  }
+  cur->size = stream_size - cur->offset;

[wolenetz, 2014/07/16 00:21:03]

nit: First, DCHECK_GE(stream_size, cur->offset); ?

[damienv1, 2014/07/16 02:34:33]

Done.

+}
+
+bool ProcessPesPackets(
+    EsParser* es_parser,
+    const std::vector<uint8>& stream,
+    const std::vector<Packet>& pes_packets,
+    bool force_timing) {
+  for (size_t k = 0; k < pes_packets.size(); k++) {

[wolenetz, 2014/07/16 00:21:04]

nit: First, ASSERT_TRUE(es_parser); ?

+    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))

[wolenetz, 2014/07/16 00:21:04]

nit: just pass kNoTimestamp() here instead of dts (and drop dts from this
block), or is sth other than kNoTimestamp() sometimes expected for dts?

[damienv1, 2014/07/16 02:34:33]

I was finding it more explicit to pass a variable name so that I know just by
looking at the code that it refers to a DTS.
(and test code does not need to be as optimized as production code).
Please let me know if you really prefer your suggestion.

[wolenetz, 2014/07/16 20:35:30]

I have no strong feeling here. Maybe s/dts/no_timestamp_dts/ to make it clear to
maintainers that dts is still expected to be kNoTimestamp() when passed as param
here?

+      return false;
+  }
+  es_parser->Flush();
+  return true;
+}
+
+}  // namespace mp2t
+}  // namespace media