Mpeg2 TS stream parser for media source.

media/mpeg2/es_parser_h264.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/es_parser_h264.h"
+
+#include "base/basictypes.h"
+#include "base/logging.h"
+#include "media/base/bit_reader.h"
+#include "media/base/buffers.h"
+#include "media/base/stream_parser_buffer.h"
+#include "media/base/video_decoder_config.h"
+#include "media/base/video_frame.h"
+#include "media/mpeg2/mpeg2ts_common.h"
+#include "ui/gfx/rect.h"
+#include "ui/gfx/size.h"
+
+namespace {
+
+const int kExtendedSar = 255;
+
+const int kTableSarWidth[14] = {

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

nit: static const please.

[damienv1, 2013/09/09 23:29:45]

Done.

+  1, 1, 12, 10, 16, 40, 24, 20, 32, 80, 18, 15, 64, 160
+};
+
+const int kTableSarHeight[14] = {
+  1, 1, 11, 11, 11, 33, 11, 11, 11, 33, 11, 11, 33, 99
+};
+
+class ByteReaderChainedBuffer {

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

docs

[damienv1, 2013/09/09 23:29:45]

Code removed.

+ public:
+  ByteReaderChainedBuffer(const uint8* buf0, int size0,
+                          const uint8* buf1, int size1)
+      : buf0_(buf0),
+        size0_(size0),
+        buf1_(buf1),
+        size1_(size1) { }
+
+  uint8 Get(int offset) const {
+    DCHECK_GE(offset, 0);
+    DCHECK_LT(offset, size0_ + size1_);

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

nit: I think using GetSize() would make this more clear.

[damienv1, 2013/09/09 23:29:45]

Code removed.

+    if (offset < size0_)
+      return buf0_[offset];
+    return buf1_[offset - size0_];
+  }
+
+  int GetSize() const {
+    return (size0_ + size1_);

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

nit: remove ().

[damienv1, 2013/09/09 23:29:45]

Code removed.

+  }
+
+ private:
+  const uint8* const buf0_;
+  const int size0_;
+  const uint8* const buf1_;
+  const int size1_;
+};
+
+}  // namespace
+
+namespace media {
+namespace mpeg2ts {
+
+EsParserH264::EsParserH264(
+    NewVideoConfigCB new_video_config_cb,
+    EmitBufferCB emit_buffer_cb)
+  : nal_es_pos_(0),
+    new_video_config_cb_(new_video_config_cb),
+    emit_buffer_cb_(emit_buffer_cb),
+    is_video_config_known_(false),
+    profile_idc_(0),
+    level_idc_(0),
+    pic_width_in_mbs_minus1_(0),
+    pic_height_in_map_units_minus1_(0) {
+}
+
+EsParserH264::~EsParserH264() {
+}
+
+void EsParserH264::Parse(const uint8* buf, int size,
+                         base::TimeDelta pts,
+                         base::TimeDelta dts) {
+  // Note: Parse is invoked each time a PES packet has been reassembled.
+  // Unfortunately, a PES packet does not necessarily map
+  // to an h264 access unit, although the HLS recommandation is to use one PES
+  // for each access unit (but this is just a recommandation and some streams
+  // do not comply with this recommandation).
+
+  int raw_es_size = 0;
+  const uint8* raw_es = NULL;
+  es_byte_queue_.Peek(&raw_es, &raw_es_size);
+
+  // Link position |raw_es_.size()| in the ES stream with a timing descriptor.

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

nit: s/raw_es_.size()/raw_es_size/?

[damienv1, 2013/09/09 23:29:45]

Done.

+  // HLS recommandation: "In AVC video, you should have both a DTS and a

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

nit: s/recommandation/recommendation/

[damienv1, 2013/09/09 23:29:45]

Done.

+  // PTS in each PES header".
+  // TODO(damienv): What if the stream is not compliant and both the PTS and the
+  // DTS are not valid ?

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

We should error out.

[damienv1, 2013/09/09 23:29:45]

Done.

+  TimingDesc timing_desc;
+  timing_desc.pts = pts;
+  timing_desc.dts = (dts != kNoTimestamp()) ? dts : pts;
+  timing_desc_list_.push_back(
+      std::pair<int, TimingDesc>(raw_es_size, timing_desc));
+
+  // Add NALs from the incoming buffer.
+  FindNals(buf, size);
+
+  // Find access units based on AUD.
+  std::list<NalDescList::iterator> access_unit_list;
+  FindAccessUnits(&access_unit_list);
+  if (access_unit_list.empty()) {
+    es_byte_queue_.Push(buf, size);
+    es_byte_queue_.Peek(&raw_es, &raw_es_size);
+    DiscardEs(raw_es_size - 4);
+    return;
+  }
+
+  // Make sure that all the frames to be emitted are in the ES buffer.
+  int last_position = (access_unit_list.back())->position;
+  int copy_size = last_position - raw_es_size;
+  if (copy_size > 0) {
+    int copy_size = last_position - raw_es_size;
+    es_byte_queue_.Push(buf, copy_size);
+    es_byte_queue_.Peek(&raw_es, &raw_es_size);
+    buf += copy_size;
+    size -= copy_size;
+  }
+
+  // Emit all frames.
+  std::list<NalDescList::iterator>::iterator it0 = access_unit_list.begin();
+  std::list<NalDescList::iterator>::iterator it1 = it0;
+  ++it1;
+  LOG_IF(WARNING, (*it0)->position != 0)

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

nit: use DVLOG

[damienv1, 2013/09/09 23:29:45]

Use DLOG instead.

+      << "Needs to discard some ES data before getting the 1st access unit: "
+      << (*it0)->position;
+  for (; it1 != access_unit_list.end(); ++it0, ++it1) {
+    int nxt_frame_position = (*it1)->position;
+    EmitFrame(*it0, *it1, nxt_frame_position);
+  }
+
+  // Discard emitted frames.
+  DiscardEs(last_position);
+
+  // Finally copy the incomplete access unit to the ES buffer.
+  es_byte_queue_.Push(buf, size);

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

Why don't you just unconditionally push at the top before the first Peek()
instead of doing it in several places in this method?

[damienv1, 2013/09/09 23:29:45]

See my comment below.
This idea was to avoid multiple copies of the same buffer.
Optimization removed and copy is now done in only 1 place (this makes the code
slightly more readable).

+}
+
+void EsParserH264::Flush() {
+  // Find access units based on AUD.
+  std::list<NalDescList::iterator> access_unit_list;
+  FindAccessUnits(&access_unit_list);
+
+  // At this point, there can be at most one access unit in the buffer.
+  DCHECK_GE(access_unit_list.size(), 1u);
+  if (!access_unit_list.empty()) {
+    // Force emitting the last access unit (even it might be incomplete).

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

nit: s/it/if it/

[damienv1, 2013/09/09 23:29:45]

Done.

+    int nxt_frame_position = 0;
+    const uint8* raw_es = NULL;

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

nit: no need to initialize these values here and elsewhere since the Peek()
immediatly overwrites them.

[damienv1, 2013/09/09 23:29:45]

Done.

+    es_byte_queue_.Peek(&raw_es, &nxt_frame_position);
+    NalDescList::iterator cur_frame = *(access_unit_list.begin());

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

nit: A comment might be nice here since it was not clear initially that you are
actually passing iterators for the same list to EmitFrame().

+    NalDescList::iterator nxt_frame = nal_desc_list_.end();
+    EmitFrame(cur_frame, nxt_frame, nxt_frame_position);
+  }
+}
+
+void EsParserH264::FindNals(const uint8* buf, int size) {
+  int raw_es_size = 0;
+  const uint8* raw_es = NULL;
+  es_byte_queue_.Peek(&raw_es, &raw_es_size);
+
+  ByteReaderChainedBuffer byte_reader(

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

Why is this preferable to just pushing the original buffer into es_byte_queue_
and solely operating on that data? If we are just trying to avoid a memcpy, that
feels like premature optimization at this point.

[damienv1, 2013/09/09 23:29:45]

Avoiding an initial copy was indeed my initial intent. I also don't know whether
it's really worth the burden (although the impact is not that high on code
readability).

Overall, I agree, I will remove this optimization. And if I really need to do it
again, I would probably have another type of byte queue to handle chained
buffers (and the ownership of these buffers could even be transfered from the
PES to the ES parser so as to avoid yet another memcpy). Eventually, the ES
parser should not need to do any memcpy (but that comes at the expense of an
increased complexity of data structures).

+      raw_es, raw_es_size,
+      buf, size);
+
+  DCHECK_GE(nal_es_pos_, 0);
+  DCHECK_LT(nal_es_pos_, byte_reader.GetSize());
+
+  // Resume NAL segmentation where it was left.
+  for ( ; nal_es_pos_ < byte_reader.GetSize() - 4; nal_es_pos_++) {
+    // Make sure the syncword is either 00 00 00 01 or 00 00 01
+    if (byte_reader.Get(nal_es_pos_ + 0) != 0 ||
+        byte_reader.Get(nal_es_pos_ + 1) != 0) {
+      continue;
+    }
+    int syncword_length = 0;
+    if (byte_reader.Get(nal_es_pos_ + 2) == 0 &&
+        byte_reader.Get(nal_es_pos_ + 3) == 1) {
+      syncword_length = 4;
+    } else if (byte_reader.Get(nal_es_pos_ + 2) == 1) {
+      syncword_length = 3;
+    } else {
+      continue;
+    }
+
+    // Retrieve the NAL type.
+    int nal_header = byte_reader.Get(nal_es_pos_ + syncword_length);
+    int forbidden_zero_bit = (nal_header >> 7) & 0x1;
+    NalDesc nal_desc;
+    nal_desc.position = nal_es_pos_;
+    nal_desc.nal_unit_type = static_cast<NalUnitType>(nal_header & 0x1f);
+    if (forbidden_zero_bit != 0)
+      nal_desc.nal_unit_type = kNalUnitTypeInvalid;
+    DVLOG(LOG_LEVEL_ES) << "nal: offset=" << nal_desc.position
+                        << " type=" << nal_desc.nal_unit_type;
+    nal_desc_list_.push_back(nal_desc);
+    nal_es_pos_ += syncword_length;
+  }
+}
+
+void EsParserH264::FindAccessUnits(
+    std::list<NalDescList::iterator>* access_unit_list) {
+  // Get the H264 access units based on AUD.
+  // Mpeg2TS spec: "2.14 Carriage of Rec. ITU-T H.264 | ISO/IEC 14496-10 video"
+  // "Each AVC access unit shall contain an access unit delimiter NAL Unit;"
+  for (NalDescList::iterator it = nal_desc_list_.begin();
+       it != nal_desc_list_.end(); ++it) {
+    if (it->nal_unit_type == kNalUnitTypeAUD) {
+      DVLOG(LOG_LEVEL_ES) << "aud found @ pos=" << it->position;
+      access_unit_list->push_back(it);
+    }
+  }
+}
+
+void EsParserH264::EmitFrame(
+    NalDescList::iterator cur_frame,

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

nit: const&

[damienv1, 2013/09/09 23:29:45]

Done.

+    NalDescList::iterator nxt_frame,
+    int nxt_frame_position) {
+  int raw_es_size = 0;
+  const uint8* raw_es = NULL;
+  es_byte_queue_.Peek(&raw_es, &raw_es_size);
+
+  // Current frame position = position of the 1st NAL of the frame.
+  int cur_frame_position = cur_frame->position;
+  int access_unit_size = nxt_frame_position - cur_frame_position;
+
+  // Get the access unit timing info.
+  TimingDesc current_timing_desc;
+  while (!timing_desc_list_.empty() &&

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

Any reason we can't just store the TimingDesc w/ the NALDesc so we don't have to
keep these 2 lists in sync? It seems like that could make this code easier to
follow.

An alternative might be to have TimingDesc have a NalDescList that stores the
information for NAL units associated w/ this timing data. I think that would be
easier to follow and less error prone for future code updates.

[damienv1, 2013/09/09 23:29:45]

A timing descriptor refers to a specific byte position. I don't feel these 2
lists have anything in common.
Eventually, I'd like to remove the list of NALs and switch to a more bitstream
oriented parser (i.e. each byte is parsed only once and we don't go backward as
it is the case now).

+         timing_desc_list_.front().first <= cur_frame_position) {
+    current_timing_desc = timing_desc_list_.front().second;
+    timing_desc_list_.pop_front();
+  }
+
+  // Check whether this is a key frame + light NAL parsing to get some
+  // relevant information (e.g. SPS/PPS).
+  // Note: it would have been nice to get the keyframe decision based
+  // on the Mpeg2TS random_access_indicator but encoders sometimes just don't
+  // bother setting this flag in the MPEG2 TS stream.
+  bool is_key_frame = true;
+  for (NalDescList::iterator it = cur_frame; it != nxt_frame; ++it) {
+    if (it->nal_unit_type == kNalUnitTypeNonIdrSlice)
+      is_key_frame = false;
+    NalDescList::iterator next_nal_it = it;
+    ++next_nal_it;
+    int cur_nal_position = it->position;
+    int nxt_nal_position = (next_nal_it == nxt_frame)
+        ? nxt_frame_position : next_nal_it->position;
+    int nal_size = nxt_nal_position - cur_nal_position;
+    DCHECK_LE(cur_nal_position + nal_size, raw_es_size);
+    NalParser(&raw_es[cur_nal_position], nal_size);

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

This should signal an error when it encounters invalid NALs

[damienv1, 2013/09/09 23:29:45]

Done.

+  }
+
+  // Emit the current frame.
+  DVLOG(LOG_LEVEL_ES) << "is_key_frame = " << is_key_frame;
+  scoped_refptr<StreamParserBuffer> stream_parser_buffer =
+      StreamParserBuffer::CopyFrom(
+          &raw_es[cur_frame_position],
+          access_unit_size,
+          is_key_frame);
+  stream_parser_buffer->SetDecodeTimestamp(current_timing_desc.dts);
+  stream_parser_buffer->set_timestamp(current_timing_desc.pts);
+  emit_buffer_cb_.Run(stream_parser_buffer);
+}
+
+void EsParserH264::DiscardEs(int nbytes) {
+  if (nbytes <= 0)
+    return;
+
+  // Update the NAL list accordingly.
+  while (!nal_desc_list_.empty() &&

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

Would it be possible to make this and the timing_desc_ list pruning happen in
EmitFrame()? It seems like it would be nicer to have this information pruned as
we are processing the NAL data and emitting the frames associated with it.

[damienv1, 2013/09/09 23:29:45]

NAL pruning is done here mainly for "safety" reasons. The code keeps some
iterators on the NAL list and I feel like removing data from the list while
still using some iterators on that same list is more error prone.

+         nal_desc_list_.front().position < nbytes) {
+    nal_desc_list_.pop_front();
+  }
+  for (NalDescList::iterator it = nal_desc_list_.begin();
+       it != nal_desc_list_.end(); ++it) {
+    DCHECK(it->position >= nbytes);
+    it->position -= nbytes;
+  }
+  nal_es_pos_ -= nbytes;
+  if (nal_es_pos_ < 0)
+    nal_es_pos_ = 0;
+
+  // Update the timing information accordingly.
+  std::list<std::pair<int, TimingDesc> >::iterator timing_it
+      = timing_desc_list_.begin();
+  for (; timing_it != timing_desc_list_.end(); ++timing_it) {
+    timing_it->first -= nbytes;
+  }
+
+  // Discard |nbytes| of ES.
+  es_byte_queue_.Pop(nbytes);
+}
+
+void EsParserH264::NalParser(const uint8* buf, int size) {
+  // Discard the annexB syncword.
+  if (size < 3) {
+    LOG(WARNING) << "NalParser: incomplete NAL";
+    return;

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

Shouldn't this signal an error since it appears that bad data is in the
bytestream?

[damienv1, 2013/09/09 23:29:45]

Done.

+  }
+  DCHECK_EQ(buf[0], 0);

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

These should signal an error since it represents bad data.

[damienv1, 2013/09/09 23:29:45]

Done.

+  DCHECK_EQ(buf[1], 0);
+  if (buf[2] == 1) {
+    buf += 3;
+    size -= 3;
+  } else {
+    buf += 4;
+    size -= 4;
+  }
+
+  // Get the NAL header.
+  if (size < 1) {
+    LOG(WARNING) << "NalParser: incomplete NAL";
+    return;

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

ditto

[damienv1, 2013/09/09 23:29:45]

Done.

+  }
+  int nal_header = buf[0];
+  buf += 1;
+  size -= 1;
+
+  int forbidden_zero_bit = (nal_header >> 7) & 0x1;
+  if (forbidden_zero_bit != 0)
+    return;

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

should trigger an error.

[damienv1, 2013/09/09 23:29:45]

Done.

+  int nal_ref_idc = (nal_header >> 5) & 0x3;
+  int nal_unit_type = nal_header & 0x1f;
+
+  // TODO(damienv):
+  // The nal start code emulation prevention should be un-done,
+  // before parsing the NAL content.
+
+  // Process the NAL content.
+  if (nal_unit_type == kNalUnitTypeSPS) {
+    DVLOG(LOG_LEVEL_ES) << "NAL: SPS";
+    if (nal_ref_idc == 0) {
+      // Should not be 0 for a SPS.
+      return;

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

should trigger an error.

[damienv1, 2013/09/09 23:29:45]

Done.

+    }
+    ProcessSPS(buf, size);

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

false return values from these methods should trigger an error.

[damienv1, 2013/09/09 23:29:45]

Done.

+  } else if (nal_unit_type == kNalUnitTypeIdrSlice) {
+    DVLOG(LOG_LEVEL_ES) << "NAL: IDR slice";
+    ProcessSliceLayer(buf, size);
+  } else if (nal_unit_type == kNalUnitTypeNonIdrSlice) {
+    DVLOG(LOG_LEVEL_ES) << "NAL: Non IDR slice";
+    ProcessSliceLayer(buf, size);
+  } else if (nal_unit_type == kNalUnitTypePPS) {
+    DVLOG(LOG_LEVEL_ES) << "NAL: PPS";
+  } else if (nal_unit_type == kNalUnitTypeAUD) {
+    DVLOG(LOG_LEVEL_ES) << "NAL: AUD";
+  } else {
+    DVLOG(LOG_LEVEL_ES) << "NAL: " << nal_unit_type;
+  }
+}
+
+bool EsParserH264::ProcessSPS(const uint8* buf, int size) {
+  if (size <= 0)
+    return false;
+  BitReader bit_reader(buf, size);
+
+  int profile_idc;
+  RCHECK(bit_reader.ReadBits(8, &profile_idc));
+  int constraint_setX_flag;

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

nit: I think the code would be easier to read if all the declarations were above
this group of RCHECKs instead of interleaved between each call.

[damienv1, 2013/09/09 23:29:45]

Done.

+  RCHECK(bit_reader.ReadBits(8, &constraint_setX_flag));
+  int level_idc;
+  RCHECK(bit_reader.ReadBits(8, &level_idc));
+  uint32 seq_parameter_set_id;
+  RCHECK(ReadBitsExpGolomb(&bit_reader, &seq_parameter_set_id));
+  uint32 log2_max_frame_num_minus4;
+  RCHECK(ReadBitsExpGolomb(&bit_reader, &log2_max_frame_num_minus4));
+  uint32 pic_order_cnt_type;
+  RCHECK(ReadBitsExpGolomb(&bit_reader, &pic_order_cnt_type));
+
+  if (pic_order_cnt_type > 2) {
+    // Bitstream error: pic_order_cnt_type shall be in the range of 0 to 2.
+    return false;
+  }
+  if (pic_order_cnt_type == 0) {
+    uint32 log2_max_pic_order_cnt_lsb_minus4;
+    RCHECK(ReadBitsExpGolomb(&bit_reader, &log2_max_pic_order_cnt_lsb_minus4));
+  } else if (pic_order_cnt_type == 1) {
+    NOTIMPLEMENTED();
+    LOG(FATAL) << "pic_order_cnt_type = 1 not supported yet";

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

return false here since it isn't supported yet. Use DVLOG() or MEDIA_LOG() here
instead.

[damienv1, 2013/09/09 23:29:45]

Done.

+  }
+
+  uint32 num_ref_frames;
+  RCHECK(ReadBitsExpGolomb(&bit_reader, &num_ref_frames));
+  int gaps_in_frame_num_value_allowed_flag;

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

nit: I think grouping the variable declarations at the top would make this more
readable.

[damienv1, 2013/09/09 23:29:45]

Done.

+  RCHECK(bit_reader.ReadBits(1, &gaps_in_frame_num_value_allowed_flag));
+  uint32 pic_width_in_mbs_minus1;
+  RCHECK(ReadBitsExpGolomb(&bit_reader, &pic_width_in_mbs_minus1));
+  uint32 pic_height_in_map_units_minus1;
+  RCHECK(ReadBitsExpGolomb(&bit_reader, &pic_height_in_map_units_minus1));
+
+  int frame_mbs_only_flag;
+  RCHECK(bit_reader.ReadBits(1, &frame_mbs_only_flag));
+  if (!frame_mbs_only_flag) {
+    int mb_adaptive_frame_field_flag;
+    RCHECK(bit_reader.ReadBits(1, &mb_adaptive_frame_field_flag));
+  }
+
+  int direct_8x8_inference_flag;
+  RCHECK(bit_reader.ReadBits(1, &direct_8x8_inference_flag));
+
+  bool frame_cropping_flag;
+  uint32 frame_crop_left_offset = 0;
+  uint32 frame_crop_right_offset = 0;
+  uint32 frame_crop_top_offset = 0;
+  uint32 frame_crop_bottom_offset = 0;
+  RCHECK(bit_reader.ReadBits(1, &frame_cropping_flag));
+  if (frame_cropping_flag) {
+    RCHECK(ReadBitsExpGolomb(&bit_reader, &frame_crop_left_offset));
+    RCHECK(ReadBitsExpGolomb(&bit_reader, &frame_crop_right_offset));
+    RCHECK(ReadBitsExpGolomb(&bit_reader, &frame_crop_top_offset));
+    RCHECK(ReadBitsExpGolomb(&bit_reader, &frame_crop_bottom_offset));
+  }
+
+  bool vui_parameters_present_flag;
+  RCHECK(bit_reader.ReadBits(1, &vui_parameters_present_flag));
+  int sar_width = 1;
+  int sar_height = 1;
+  if (vui_parameters_present_flag) {
+    // Read only the aspect ratio information from the VUI section.
+    // TODO(damienv): check whether other VUI info are useful.
+    bool aspect_ratio_info_present_flag = false;
+    RCHECK(bit_reader.ReadBits(1, &aspect_ratio_info_present_flag));
+    if (aspect_ratio_info_present_flag) {
+      int aspect_ratio_idc;
+      RCHECK(bit_reader.ReadBits(8, &aspect_ratio_idc));
+      if (aspect_ratio_idc == kExtendedSar) {
+        RCHECK(bit_reader.ReadBits(16, &sar_width));
+        RCHECK(bit_reader.ReadBits(16, &sar_height));
+      } else if (aspect_ratio_idc < 14) {
+        sar_width = kTableSarWidth[aspect_ratio_idc];
+        sar_height = kTableSarHeight[aspect_ratio_idc];
+      }
+    }
+  }
+
+  LOG_IF(WARNING, sar_width != sar_height)
+      << "Non square pixel not supported yet:"
+      << " sar_width=" << sar_width
+      << " sar_height=" << sar_height;

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

nit: Trigger an error for things that are not supported yet. Use MEDIA_LOG() for
notification.

[damienv1, 2013/09/09 23:29:45]

Done.

+
+  if (is_video_config_known_ &&
+      profile_idc == profile_idc_ &&
+      level_idc == level_idc_ &&
+      pic_width_in_mbs_minus1 == pic_width_in_mbs_minus1_ &&
+      pic_height_in_map_units_minus1 == pic_height_in_map_units_minus1_) {
+    // This is the same SPS as the previous one.
+    return true;
+  }
+  is_video_config_known_ = true;
+  profile_idc_ = profile_idc;
+  level_idc_ = level_idc;
+  pic_width_in_mbs_minus1_ = pic_width_in_mbs_minus1;
+  pic_height_in_map_units_minus1_ = pic_height_in_map_units_minus1;
+
+  // TODO(damienv):
+  // Assuming the SPS is used right away by the PPS
+  // and the slice headers is a strong assumption.
+  // In theory, we should process the SPS and PPS
+  // and only when one of the slice header is switching
+  // the PPS id, the video decoder config should be changed.
+  DVLOG(1) << "Profile IDC: " << profile_idc;
+  DVLOG(1) << "Level IDC: " << level_idc;
+  DVLOG(1) << "Pic width: " << (pic_width_in_mbs_minus1 + 1) * 16;
+  DVLOG(1) << "Pic height: " << (pic_height_in_map_units_minus1 + 1) * 16;
+  DVLOG(1) << "log2_max_frame_num_minus4: " << log2_max_frame_num_minus4;
+
+  // TODO(damienv): a MAP unit can be either 16 or 32 pixels.
+  // although it's 16 pixels for progressive non MBAFF frames.
+  gfx::Size coded_size((pic_width_in_mbs_minus1 + 1) * 16,
+                       (pic_height_in_map_units_minus1 + 1) * 16);
+  gfx::Rect visible_rect(
+      frame_crop_left_offset,
+      frame_crop_top_offset,
+      (coded_size.width() - frame_crop_right_offset) - frame_crop_left_offset,
+      (coded_size.height() - frame_crop_bottom_offset) - frame_crop_top_offset);
+
+  // TODO(damienv): calculate the natural size based
+  // on the possible aspect ratio coded in the VUI parameters.
+  gfx::Size natural_size(visible_rect.width(),
+                         visible_rect.height());
+
+  VideoDecoderConfig video_decoder_config(
+      kCodecH264,
+      VIDEO_CODEC_PROFILE_UNKNOWN,    // TODO(damienv)
+      VideoFrame::YV12,
+      coded_size,
+      visible_rect,
+      natural_size,
+      NULL, 0,
+      false);
+  new_video_config_cb_.Run(video_decoder_config);
+
+  return true;
+}
+
+bool EsParserH264::ProcessSliceLayer(const uint8* buf, int size) {
+  if (size <= 0)
+    return false;
+  BitReader bit_reader(buf, size);
+
+  // Read only the slice header.
+  // TODO(damienv): frame_num
+  uint32 first_mb_in_slice;
+  RCHECK(ReadBitsExpGolomb(&bit_reader, &first_mb_in_slice));
+  uint32 slice_type;

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

ditto

[damienv1, 2013/09/09 23:29:45]

Code removed.

+  RCHECK(ReadBitsExpGolomb(&bit_reader, &slice_type));
+  uint32 pic_parameter_set_id;
+  RCHECK(ReadBitsExpGolomb(&bit_reader, &pic_parameter_set_id));
+
+  DVLOG(LOG_LEVEL_ES) << "first_mb_in_slice: " << first_mb_in_slice;
+  DVLOG(LOG_LEVEL_ES) << "slice_type: " << slice_type;
+  return true;

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

Why are we doing this parsing? Just to verify that the NAL is larg enough to
contain these fields?

[damienv1, 2013/09/09 23:29:45]

Initially, I didn't know whether I would get some streams that don't use AUD. In
this case, slice layer must be parsed to know which |frame_num| a NAL belongs
to. That would have been used to do access unit segmentation.

But in the end, both Mpeg2 TS spec and HLS recommandation specifies that AUD
must be used. And that's always the case in practice (at least what I have seen
so far).

Long story just to say that I am going to remove this code.

+}
+
+bool EsParserH264::ReadBitsExpGolomb(
+    BitReader* bit_reader, uint32* exp_golomb_value) {
+  // TODO(damienv): this should be a member function of BitReader.

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

nit: This should probably be a member of a class that derives from BitReader,
but I think this seems specific to H.264 so it isn't clear to me that it should
be in BitReader itself. Keeping this here until more classes need it seems like
a reasonable approach to me.

[damienv1, 2013/09/09 23:29:45]

Made it a subclass of BitReader.

+
+  // Get the number of leading zeros.
+  int zero_count = 0;
+  for (zero_count = 0; ; zero_count++) {

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

nit: the zero_count = 0 is not needed here.

[damienv1, 2013/09/09 23:29:45]

Done.

+    int one_bit;
+    if (!bit_reader->ReadBits(1, &one_bit)) {

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

nit: remove {} here and below.

[damienv1, 2013/09/09 23:29:45]

Done.

+      return false;
+    }
+    if (one_bit != 0) {
+      break;
+    }
+  }
+

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

Add the following here to avoid undefined behavior below.

if (zero_count > 31)
  return false;

[damienv1, 2013/09/09 23:29:45]

Good remark !

+  // Read the actual value.
+  uint32 base_value = (1 << zero_count) - 1;
+  uint32 value = 0;
+  for (int bit_count = 0; bit_count < zero_count; bit_count++) {
+    int one_bit;
+    if (!bit_reader->ReadBits(1, &one_bit)) {
+      return false;
+    }
+    if (one_bit != 0) {
+      value += (1 << (zero_count-1 - bit_count));

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

nit: space on either side of -

[damienv1, 2013/09/09 23:29:45]

Done.

+    }
+  }
+
+  *exp_golomb_value = base_value + value;
+  return true;
+}
+
+}  // namespace mpeg2ts
+}  // namespace media
+