Mpeg2 TS stream parser for media source.

media/mpeg2/mpeg2ts_stream_parser.cc

+// Copyright (c) 2013 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/mpeg2/mpeg2ts_stream_parser.h"
+
+#include "base/bind.h"
+#include "base/memory/scoped_ptr.h"
+#include "media/base/audio_decoder_config.h"
+#include "media/base/stream_parser_buffer.h"
+#include "media/base/video_decoder_config.h"
+#include "media/mpeg2/es_parser.h"
+#include "media/mpeg2/es_parser_adts.h"
+#include "media/mpeg2/es_parser_h264.h"
+#include "media/mpeg2/mpeg2ts_common.h"
+#include "media/mpeg2/mpeg2ts_pat.h"
+#include "media/mpeg2/mpeg2ts_pes.h"
+#include "media/mpeg2/mpeg2ts_pmt.h"
+#include "media/mpeg2/mpeg2ts_section_parser.h"
+#include "media/mpeg2/ts_packet.h"
+
+namespace {
+
+enum StreamType {

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit: Please add spec reference for these magic numbers. and just move this into
the mpeg2ts namespace below instead of putting it in an anonymous namespace
here.

[damienv1, 2013/09/04 01:37:13]

Done.

+  kStreamTypeMpeg1Audio = 0x3,
+  kStreamTypeAAC = 0xf,
+  kStreamTypeAVC = 0x1b,
+};
+
+}
+
+namespace media {
+namespace mpeg2ts {
+
+class PidState {
+ public:
+  int pid;
+  int continuity_counter;
+  scoped_ptr<Mpeg2TsSectionParser> section_parser;
+};
+
+Mpeg2TsStreamParser::Mpeg2TsStreamParser()
+  : pids_deleter_(&pids_),
+    selected_pmt_pid_(-1),
+    selected_audio_pid_(-1),
+    selected_video_pid_(-1),
+    is_initialized_(false),
+    segment_started_(false) {
+  LOG(INFO) << "Mpeg2TsStreamParser::Mpeg2TsStreamParser";
+}
+
+Mpeg2TsStreamParser::~Mpeg2TsStreamParser() {
+}
+
+void Mpeg2TsStreamParser::Init(
+    const InitCB& init_cb,
+    const NewConfigCB& config_cb,
+    const NewBuffersCB& new_buffers_cb,
+    const NewTextBuffersCB& text_cb,
+    const NeedKeyCB& need_key_cb,
+    const AddTextTrackCB& add_text_track_cb,
+    const NewMediaSegmentCB& new_segment_cb,
+    const base::Closure& end_of_segment_cb,
+    const LogCB& log_cb) {
+  DCHECK(init_cb_.is_null());

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit:DCHECK(!is_initialized_)?

[damienv1, 2013/09/04 01:37:13]

Done.

+  DCHECK(!init_cb.is_null());
+  DCHECK(!config_cb.is_null());
+  DCHECK(!new_buffers_cb.is_null());
+  DCHECK(!need_key_cb.is_null());
+  DCHECK(!end_of_segment_cb.is_null());
+
+  init_cb_ = init_cb;
+  config_cb_ = config_cb;
+  new_buffers_cb_ = new_buffers_cb;
+  need_key_cb_ = need_key_cb;
+  new_segment_cb_ = new_segment_cb;
+  end_of_segment_cb_ = end_of_segment_cb;
+  log_cb_ = log_cb;
+}
+
+void Mpeg2TsStreamParser::Flush() {
+  LOG(INFO) << "Mpeg2TsStreamParser::Flush()";

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit: Use DVLOG everywhere instead of LOG. DVLOG(1) is typically used in the
other parsers for this type of message.

[damienv1, 2013/09/04 01:37:13]

Done.

+
+  // Flush the buffers and reset the pids.
+  for (std::map<int, PidState*>::iterator it = pids_.begin();
+       it != pids_.end(); ++it) {
+    LOG(INFO) << "Flushing PID: " << it->first;
+    PidState* pid_state = it->second;
+    pid_state->section_parser->Flush();
+    delete pid_state;

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

If you are deleting the pid_state, why do you need to call Flush()?

[damienv1, 2013/09/04 01:37:13]

For video, there might be some pending buffers:
a reassembled video PES is emitted only when the next one is coming, so the last
reassembled PES is a pending buffer. Same at the h264 ES level where a frame is
emitted when the next AUD is coming. So Flush must be invoked to force emitting
these buffers, otherwise, the last frame might be missing.

+  }
+  pids_.clear();
+  EmitRemainingBuffers();
+
+  // End of the segment.
+  end_of_segment_cb_.Run();

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

You shouldn't need to make this call. Are you running into problems if you don't
call it?

[damienv1, 2013/09/04 01:37:13]

I was thinking it was needed. Checked without, it works.
Done.

+  segment_started_ = false;
+
+  // Remove any bytes left in the TS buffer.
+  // (i.e. any partial TS packet => less than 188 bytes).
+  ts_buffer_.clear();
+
+  // Remove the programs and the corresponding PIDs.
+  programs_.clear();
+
+  // Reset the selected PIDs.
+  selected_pmt_pid_ = -1;
+  selected_audio_pid_ = -1;
+  selected_video_pid_ = -1;
+
+  // Reset the audio and video configs.
+  audio_config_.reset();
+  video_config_.reset();
+
+  // TODO(damienv): What to do with the remaining audio/video buffer queues ?
+  // Note: there should not be any.
+  LOG_IF(WARNING, !audio_buffer_queue_.empty())
+      << "Flush: audio buffer queue not empty";

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit: Make these DCHECKS and place them right after the EmitRemainingBuffers()
call above.

[damienv1, 2013/09/04 01:37:13]

Done.

+  LOG_IF(WARNING, !video_buffer_queue_.empty())
+      << "Flush: video buffer queue not empty";
+  audio_buffer_queue_.clear();
+  video_buffer_queue_.clear();
+}
+
+bool Mpeg2TsStreamParser::Parse(const uint8* buf, int size) {
+  LOG(INFO) << "Mpeg2TsStreamParser::Parse size=" << size;
+
+  // Add the data to the parser state.
+  int old_size = ts_buffer_.size();
+  ts_buffer_.resize(old_size + size);

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

Convert ts_buffer_ to a media::ByteQueue so you don't have to do this.

[damienv1, 2013/09/04 01:37:13]

Done.

+  memcpy(&ts_buffer_[old_size], buf, size);
+
+  int remaining_size = ts_buffer_.size();
+  int pos = 0;
+  while (remaining_size >= TsPacket::kPacketSize) {
+    // Synchronization.
+    int skipped_bytes = TsPacket::Sync(&ts_buffer_[pos], remaining_size);
+    if (skipped_bytes > 0) {
+      LOG(WARNING) << "Packet not aligned on a TS syncword:"
+                   << " skipped_bytes=" << skipped_bytes;
+      pos += skipped_bytes;
+      remaining_size -= skipped_bytes;
+      continue;
+    }
+
+    // Parse the TS header.
+    scoped_ptr<TsPacket> ts_packet(
+        TsPacket::Parse(&ts_buffer_[pos], remaining_size));
+    if (!ts_packet.get()) {

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit: You shouldn't need the get().

[damienv1, 2013/09/04 01:37:13]

Done.

+      LOG(WARNING) << "Error: invalid TS packet";
+      pos += 1;
+      remaining_size -= 1;

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit: You should probably put an upper bound on invalid TS packets observed and
return false if too many are encountered in a row.

[damienv1, 2013/09/04 01:37:13]

Not sure if this is the right behavior yet.
I would rather try to make the StreamParser as dumb as possible.

+      continue;
+    }
+
+    VLOG(LOG_LEVEL_TS)

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit: Use DVLOG instead of VLOG here and everywhere else.

[damienv1, 2013/09/04 01:37:13]

Done.

+        << "Processing PID=" << ts_packet->pid()
+        << " start_unit=" << ts_packet->payload_unit_start_indicator();
+
+    // Parse the section.
+    std::map<int, PidState*>::iterator it =
+        pids_.find(ts_packet->pid());

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit: This should fit on the line above.

[damienv1, 2013/09/04 01:37:13]

Done.

+    if (it == pids_.end() && ts_packet->pid() == 0) {
+      // Create the PAT state here if needed.
+      LOG(INFO) << "Create a new PAT parser";
+      PidState* pat_state = new PidState;

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit: Use scoped_ptr<PidState> here and .release() below.

[damienv1, 2013/09/04 01:37:13]

Done.

+      pat_state->section_parser.reset(
+          new Mpeg2TsPatParser(
+              base::Bind(&Mpeg2TsStreamParser::RegisterPmt,
+                         base::Unretained(this))));
+      it = pids_.insert(
+          std::pair<int, PidState*>(ts_packet->pid(), pat_state)).first;
+    }
+
+    LOG_IF(WARNING, it == pids_.end()) << "Ignoring TS packet for pid: "

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit: Just place an unconditional log statement in an else clause below.

[damienv1, 2013/09/04 01:37:13]

Done.

+                                       << ts_packet->pid();
+    if (it != pids_.end()) {
+      it->second->section_parser->Parse(
+          ts_packet->payload_unit_start_indicator(),
+          &ts_buffer_[pos+ts_packet->GetPayloadOffset()],
+          ts_packet->GetPayloadSize());
+    }
+
+    // Go to the next packet.
+    pos += TsPacket::kPacketSize;
+    remaining_size -= TsPacket::kPacketSize;
+  }
+
+  // Check whether we are on a TS packet boundary.

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit: This comment seems stale. It doesn't appear that you are doing any checks
here.

[damienv1, 2013/09/04 01:37:13]

Done.

+  if (remaining_size > 0) {
+    memmove(&ts_buffer_[0], &ts_buffer_[pos], remaining_size);
+    ts_buffer_.resize(remaining_size);
+  } else {
+    ts_buffer_.resize(0);
+  }
+
+  EmitRemainingBuffers();
+
+  return true;
+}
+
+void Mpeg2TsStreamParser::RegisterPmt(int program_number, int pmt_pid) {
+  LOG(INFO) << "RegisterPmt:"
+            << " program_number=" << program_number
+            << " pmt_pid=" << pmt_pid;
+  std::map<int, PidState*>::iterator it = pids_.find(pmt_pid);
+  if (it != pids_.end()) {

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit: remove {} here and on other 1 line statements below.

[damienv1, 2013/09/04 01:37:13]

Done.

+    return;
+  }
+
+  if (selected_pmt_pid_ < 0) {
+    selected_pmt_pid_ = pmt_pid;
+  }
+  LOG_IF(WARNING, selected_pmt_pid_ != pmt_pid)
+      << "More than one program is defined";

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

If more than one program is present then an error needs to be signalled. The MSE
spec states that only 1 program is allowed.

[damienv1, 2013/09/04 01:37:13]

When there are multiple programs, the Mpeg2 TS stream parser considers only one
of them. I was considering this behavior as a reasonable behavior instead of
signaling an error (that ends the playback).

[acolwell GONE FROM CHROMIUM, 2013/09/05 18:29:09]

Multiple programs are not allowed by the spec. Signal an error.

+
+  // Create the PMT state here if needed.
+  LOG(INFO) << "Create a new PMT parser";
+  PidState* pmt_state = new PidState;

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit: Use scoped_ptr<PidState> here and release() below.

[damienv1, 2013/09/04 01:37:13]

Done.

+  pmt_state->section_parser.reset(
+      new Mpeg2TsPmtParser(
+          base::Bind(&Mpeg2TsStreamParser::RegisterPes,
+                     base::Unretained(this), pmt_pid)));
+  pids_.insert(std::pair<int, PidState*>(pmt_pid, pmt_state));
+}
+
+void Mpeg2TsStreamParser::RegisterPes(int pmt_pid,
+                                      int pes_pid,
+                                      int stream_type) {
+  // TODO(damienv): check there is no mismatch if the entry already exists.
+  LOG(INFO) << "RegisterPes:"
+            << " pes_pid=" << pes_pid
+            << " stream_type=" << std::hex << stream_type << std::dec;
+  std::map<int, PidState*>::iterator it = pids_.find(pes_pid);
+  if (it != pids_.end()) {
+    return;
+  }
+
+  // TODO(damienv): add other formats if needed.
+  bool is_audio = (stream_type == kStreamTypeAAC);
+  bool is_video = (stream_type == kStreamTypeAVC);
+
+  // Update the active tracks.
+  // Select the audio/video tracks with the lowest PID.
+  if (pmt_pid == selected_pmt_pid_) {

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit: reverse condition and early return. We only really need parsers for the
pids & programs we care about right?

[damienv1, 2013/09/04 01:37:13]

Done. I changed the behavior of the automatic PID filtering.

+    if (is_audio &&
+        (selected_audio_pid_ < 0 || pes_pid < selected_audio_pid_)) {

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

If the selected_xxx_pid_ changes downward, shouldn't we destroy the state for
the pid that is no longer selected?

[damienv1, 2013/09/04 01:37:13]

I changed the behavior of the PidState.
The PidState can now be enabled or disabled. So, a PidState is always created
for a new PES pid but only the lowest PES pid will be enabled.
I think this is a cleaner design.

+      selected_audio_pid_ = pes_pid;
+    }
+    if (is_video &&
+        (selected_video_pid_ < 0 || pes_pid < selected_video_pid_)) {
+      selected_video_pid_ = pes_pid;
+    }
+  }
+
+  // Create a stream parser corresponding to the stream type.
+  scoped_ptr<EsParser> es_parser;
+  if (stream_type == kStreamTypeAVC) {
+    es_parser.reset(
+        new EsParserH264(
+            base::Bind(&Mpeg2TsStreamParser::OnVideoConfigChanged,
+                       base::Unretained(this),
+                       pes_pid),
+            base::Bind(&Mpeg2TsStreamParser::OnEmitVideoBuffer,
+                       base::Unretained(this),
+                       pes_pid)));
+  } else if (stream_type == kStreamTypeAAC) {
+    es_parser.reset(
+        new EsParserAdts(
+            base::Bind(&Mpeg2TsStreamParser::OnAudioConfigChanged,
+                       base::Unretained(this),
+                       pes_pid),
+            base::Bind(&Mpeg2TsStreamParser::OnEmitAudioBuffer,
+                       base::Unretained(this),
+                       pes_pid)));
+  }

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit: else return. We don't need state for a strem_type we don't care about
right?

[damienv1, 2013/09/04 01:37:13]

Done.

+
+  // Create the PES state here.
+  LOG(INFO) << "Create a new PES state";
+  PidState* pes_state = new PidState;

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

ditto. Perhaps an AddPidParser(pid, parser) helper function is in order.

[damienv1, 2013/09/04 01:37:13]

Code has been slightly refactored, and this helper does not seem to be needed.

+  pes_state->section_parser.reset(
+      new Mpeg2TsPesParser(es_parser.release()));
+  pids_.insert(std::pair<int, PidState*>(pes_pid, pes_state));
+}
+
+void Mpeg2TsStreamParser::OnVideoConfigChanged(
+    int pes_pid,
+    const VideoDecoderConfig& video_decoder_config) {
+  DCHECK_GT(selected_video_pid_, 0);
+  LOG(INFO) << "OnVideoConfigChanged for pid=" << pes_pid;
+  if (pes_pid != selected_video_pid_) {

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit: It seems like this should be a DCHECK_NE(pes_pid, selected_video_pid_). We
shouldn't be doing work for pids we don't care about.

[damienv1, 2013/09/04 01:37:13]

Done.

+    return;
+  }
+
+  video_config_.reset(new VideoDecoderConfig());
+  *video_config_ = video_decoder_config;
+
+  OnAudioVideoConfigChanged();
+}
+
+void Mpeg2TsStreamParser::OnAudioConfigChanged(
+    int pes_pid,
+    const AudioDecoderConfig& audio_decoder_config) {
+  LOG(INFO) << "OnAudioConfigChanged";
+  if (pes_pid != selected_audio_pid_) {
+    return;
+  }
+
+  audio_config_.reset(new AudioDecoderConfig());

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit: You shouldn't need a pointer here. You can just assign the variable to
AudioDecoderConfig() if you want it to be invalid.

[damienv1, 2013/09/04 01:37:13]

Done.

+  *audio_config_ = audio_decoder_config;
+
+  OnAudioVideoConfigChanged();
+}
+
+void Mpeg2TsStreamParser::OnAudioVideoConfigChanged() {
+  if (selected_audio_pid_ > 0 && !audio_config_) {

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

It seems like this should be DCHECK(!audio_config_.IsValidConfig() ||
selected_audio_pid_) instead.

[damienv1, 2013/09/04 01:37:13]

I slightly changed the design so this comment should not be relevant anymore.

+    // Need to get the audio config as well before going any further.
+    return;
+  }
+  if (selected_video_pid_ > 0 && !video_config_) {

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

ditto for video.

[damienv1, 2013/09/04 01:37:13]

Ditto

+    // Need to get the video config as well before going any further.
+    return;
+  }
+
+  // Emit pending buffers only if already initialized.
+  // Buffers need to be emitted since these buffers do not necesseraly

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit: s/ necesseraly/necessarily/

[damienv1, 2013/09/04 01:37:13]

Not relevant anymore.

+  // have the same config.
+  if (is_initialized_) {
+    EmitRemainingBuffers();
+  }
+
+  if (selected_audio_pid_ > 0 && selected_video_pid_ > 0) {

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

Removing the pointer usage would remove the need for most of this code.

[damienv1, 2013/09/04 01:37:13]

Done.

+    // Streams with both audio and video.
+    config_cb_.Run(*audio_config_, *video_config_);
+  } else if (selected_video_pid_ > 0) {
+    // Video stream only.
+    AudioDecoderConfig audio_config;
+    config_cb_.Run(audio_config, *video_config_);
+  } else if (selected_audio_pid_ > 0) {
+    // Audio stream only.
+    VideoDecoderConfig video_config;
+    config_cb_.Run(*audio_config_, video_config);
+  } else {
+    // At least one audio or video config should be set.
+    NOTREACHED();

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

This should be a DCHECK(audio_config_.IsValidConfig() ||
video_config_.IsValidConfig()) at the top of the method.

[damienv1, 2013/09/04 01:37:13]

Removed.

+  }
+
+  if (!is_initialized_) {

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit: reverse condition and return early.

[damienv1, 2013/09/04 01:37:13]

I usually use "early return" for the exceptional flow (e.g. errors or sth that
happens only once). Here, handling the un-initialized case is definitely the
exceptional flow (as opposed to the main flow), so I prefer to keep the
condition the current condition and not invert it.

[acolwell GONE FROM CHROMIUM, 2013/09/05 18:29:09]

In this case, it isn't about exceptional flow. It is about "does more work need
to be done". Returning early is a stronger signal that nothing more needs to be
done.

+    // For Mpeg2 TS we don't know the stream duration.
+    LOG(INFO) << "Mpeg2TS stream parser initialization done";
+    init_cb_.Run(true, base::TimeDelta());

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit: kInfiniteDuration() should be used here instead.

[damienv1, 2013/09/04 01:37:13]

Done.

+    is_initialized_ = true;
+  }
+}
+
+void Mpeg2TsStreamParser::OnEmitAudioBuffer(
+    int pes_pid,
+    scoped_refptr<StreamParserBuffer> stream_parser_buffer) {
+  // Since ChunkDemuxer currently handle only one audio and one video,

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit: Do not reference ChunkDemuxer here. "Since StreamParsers can only emit one
audio and one video,"

[damienv1, 2013/09/04 01:37:13]

Done.

+  // the PID filter must be done inside the TS parser.
+  // Eventually, the PID filter will be done on the JS application side
+  // (using the HTML5 video element track selection).
+  if (pes_pid != selected_audio_pid_) {

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit: It seems like this should be a DCHECK. We shouldn't be parsing PIDs we
don't care about.

[damienv1, 2013/09/04 01:37:13]

Done.

+    return;
+  }
+
+  VLOG(LOG_LEVEL_ES)
+      << "OnEmitAudioBuffer: "
+      << " size="
+      << stream_parser_buffer->data_size()
+      << " dts="
+      << stream_parser_buffer->GetDecodeTimestamp().InMilliseconds()
+      << " pts="
+      << stream_parser_buffer->timestamp().InMilliseconds();
+  stream_parser_buffer->set_timestamp(
+      stream_parser_buffer->timestamp() - time_offset_);
+  stream_parser_buffer->SetDecodeTimestamp(
+      stream_parser_buffer->GetDecodeTimestamp() - time_offset_);
+  audio_buffer_queue_.push_back(stream_parser_buffer);
+}
+
+void Mpeg2TsStreamParser::OnEmitVideoBuffer(
+    int pes_pid,
+    scoped_refptr<StreamParserBuffer> stream_parser_buffer) {
+  // Since ChunkDemuxer currently handle only one audio and one video,
+  // the PID filter must be done inside the TS parser.
+  // Eventually, the PID filter will be done on the JS application side
+  // (using the HTML5 video element track selection).
+  if (pes_pid != selected_video_pid_) {

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

ditto

[damienv1, 2013/09/04 01:37:13]

Done.

+    return;
+  }
+
+  VLOG(LOG_LEVEL_ES)
+      << "OnEmitVideoBuffer"
+      << " size="
+      << stream_parser_buffer->data_size()
+      << " dts="
+      << stream_parser_buffer->GetDecodeTimestamp().InMilliseconds()
+      << " pts="
+      << stream_parser_buffer->timestamp().InMilliseconds()
+      << " IsKeyframe="
+      << stream_parser_buffer->IsKeyframe();
+  stream_parser_buffer->set_timestamp(
+      stream_parser_buffer->timestamp() - time_offset_);
+  stream_parser_buffer->SetDecodeTimestamp(
+      stream_parser_buffer->GetDecodeTimestamp() - time_offset_);
+  video_buffer_queue_.push_back(stream_parser_buffer);
+}
+
+void Mpeg2TsStreamParser::EmitRemainingBuffers() {
+  LOG(INFO) << "Mpeg2TsStreamParser::EmitRemainingBuffers";
+  // Segment cannot start with non key frames
+  // so replace non key frames with the 1st IDR.
+  // TODO(damienv): Non key frames could be replaced with a stuffing NAL
+  // as well, the most important is to avoid any gap between frames
+  // as MSE does not allow sparse video/audio buffers.
+  if (!segment_started_) {
+    StreamParser::BufferQueue::iterator it = video_buffer_queue_.begin();

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

I don't think this is the right solution. The parser should just drop the
non-keyframes at the beginning for now and we'll deal with the gaps introduced
in another CL that applies to all parsers.

[damienv1, 2013/09/04 01:37:13]

I removed this section but I don't really see how this can be handled as part of
MSE as there is an implicit assumption in MSE that makes sparse buffers not
possible.

+    for ( ; it != video_buffer_queue_.end(); ++it) {
+      if ((*it)->IsKeyframe()) {
+        break;
+      }
+    }
+    if (it != video_buffer_queue_.end()) {
+      StreamParser::BufferQueue::iterator it2 = video_buffer_queue_.begin();
+      while (!(*it2)->IsKeyframe()) {
+        base::TimeDelta dts = (*it2)->GetDecodeTimestamp();
+        base::TimeDelta pts = (*it2)->timestamp();
+        scoped_refptr<StreamParserBuffer> stream_parser_buffer =
+            StreamParserBuffer::CopyFrom(
+                (*it)->data(), (*it)->data_size(), true);
+        stream_parser_buffer->set_timestamp(pts);
+        stream_parser_buffer->SetDecodeTimestamp(dts);
+        LOG(WARNING) << "Replacing frame with an IDR @ pts="
+                     << pts.InMilliseconds();
+        *it2 = stream_parser_buffer;
+        ++it2;
+      }
+    } else {
+      LOG(WARNING) << "Only non key frames in the buffer queue";
+      // video_buffer_queue_.clear();
+    }
+  }
+
+  // Possibly start a segment if not done yet.
+  StartSegmentIfNeeded();

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit: Just move this into the block below and inline.

[damienv1, 2013/09/04 01:37:13]

Done.

+
+  // Finally, add the video and audio buffers.
+  if (!video_buffer_queue_.empty() ||
+      !audio_buffer_queue_.empty()) {

[acolwell GONE FROM CHROMIUM, 2013/08/29 20:44:24]

nit: reverse condition and return early.

[damienv1, 2013/09/04 01:37:13]

Done.

+    new_buffers_cb_.Run(audio_buffer_queue_, video_buffer_queue_);
+    audio_buffer_queue_.clear();
+    video_buffer_queue_.clear();
+  }
+}
+
+void Mpeg2TsStreamParser::StartSegmentIfNeeded() {
+  if (segment_started_) {
+    return;
+  }
+  if (video_buffer_queue_.empty() && audio_buffer_queue_.empty()) {
+    LOG(WARNING) << "Start a new segment but no buffer is available";
+    return;
+  }
+  LOG(INFO) << "Starting a new segment";
+  segment_started_ = true;
+  new_segment_cb_.Run();
+}
+
+}  // namespace mpeg2ts
+}  // namespace media