Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23566013:
Mpeg2 TS stream parser for media source. (Closed)
|
Created:
7 years, 3 months ago by damienv1 Modified:
7 years, 3 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionMpeg2 TS stream parser for media source.
BUG=254214
TEST=Mp2tStreamParserTest.*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=224404
Patch Set 1 #
Total comments: 92
Patch Set 2 : #
Total comments: 37
Patch Set 3 : #
Total comments: 202
Patch Set 4 : Add basic unit tests + Cleanup #
Total comments: 20
Patch Set 5 : Address comments from patch set #3 #
Total comments: 38
Patch Set 6 : Fix audio/video buffer emission + Cleanup #Patch Set 7 : Better naming: mp2t namespace & class names #
Total comments: 11
Patch Set 8 : Address comment about DCHECK vs RCHECK #
Total comments: 26
Patch Set 9 : Cleanup - address comments from patch set #8 #
Total comments: 145
Patch Set 10 : #
Total comments: 12
Patch Set 11 : Improve buffer emission + Cleanup #
Total comments: 37
Patch Set 12 : Error handling + Time aligned audio/video buffer emission #
Total comments: 8
Patch Set 13 : Cleanup #Patch Set 14 : Minor fixes for Release build. #Patch Set 15 : Clang warning fix #
Messages
Total messages: 29 (0 generated)
Here are an initial set of comments. Several apply to multiple files across the CL, so please take a pass at fixing them everywhere and let me know when a new version is posted. https://codereview.chromium.org/23566013/diff/1/media/filters/stream_parser_f... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/23566013/diff/1/media/filters/stream_parser_f... media/filters/stream_parser_factory.cc:21: #include "media/mpeg2/mpeg2ts_stream_parser.h" nit: This should be inside the USE_PROPRIETARY_CODECS define since it relies on the proprietary codecs being compiled in. https://codereview.chromium.org/23566013/diff/1/media/filters/stream_parser_f... media/filters/stream_parser_factory.cc:207: #if defined(ENABLE_MPEG2TS_STREAM_PARSER) nit: This should be in the USE_PROPRIETARY_CODECS section too. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_section_p... File media/mpeg2/mpeg2ts_section_parser.h (right): https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_section_p... media/mpeg2/mpeg2ts_section_parser.h:13: enum SpecialPid { nit: Add spec section reference for these constants. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_section_p... media/mpeg2/mpeg2ts_section_parser.h:23: virtual bool Parse(bool payload_unit_start_indicator, nit: Document return value. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_section_p... media/mpeg2/mpeg2ts_section_parser.h:26: // Flush. nit: This comment isn't very helpful. Please provide more detailed description of what implementations are expected to do when they get this call. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... File media/mpeg2/mpeg2ts_stream_parser.cc (right): https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:24: enum StreamType { 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. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:65: DCHECK(init_cb_.is_null()); nit:DCHECK(!is_initialized_)? https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:82: LOG(INFO) << "Mpeg2TsStreamParser::Flush()"; nit: Use DVLOG everywhere instead of LOG. DVLOG(1) is typically used in the other parsers for this type of message. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:90: delete pid_state; If you are deleting the pid_state, why do you need to call Flush()? https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:96: end_of_segment_cb_.Run(); You shouldn't need to make this call. Are you running into problems if you don't call it? https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:118: << "Flush: audio buffer queue not empty"; nit: Make these DCHECKS and place them right after the EmitRemainingBuffers() call above. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:130: ts_buffer_.resize(old_size + size); Convert ts_buffer_ to a media::ByteQueue so you don't have to do this. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:149: if (!ts_packet.get()) { nit: You shouldn't need the get(). https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:152: remaining_size -= 1; nit: You should probably put an upper bound on invalid TS packets observed and return false if too many are encountered in a row. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:156: VLOG(LOG_LEVEL_TS) nit: Use DVLOG instead of VLOG here and everywhere else. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:162: pids_.find(ts_packet->pid()); nit: This should fit on the line above. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:166: PidState* pat_state = new PidState; nit: Use scoped_ptr<PidState> here and .release() below. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:175: LOG_IF(WARNING, it == pids_.end()) << "Ignoring TS packet for pid: " nit: Just place an unconditional log statement in an else clause below. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:189: // Check whether we are on a TS packet boundary. nit: This comment seems stale. It doesn't appear that you are doing any checks here. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:207: if (it != pids_.end()) { nit: remove {} here and on other 1 line statements below. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:215: << "More than one program is defined"; If more than one program is present then an error needs to be signalled. The MSE spec states that only 1 program is allowed. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:219: PidState* pmt_state = new PidState; nit: Use scoped_ptr<PidState> here and release() below. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:245: if (pmt_pid == selected_pmt_pid_) { nit: reverse condition and early return. We only really need parsers for the pids & programs we care about right? https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:247: (selected_audio_pid_ < 0 || pes_pid < selected_audio_pid_)) { If the selected_xxx_pid_ changes downward, shouldn't we destroy the state for the pid that is no longer selected? https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:276: } nit: else return. We don't need state for a strem_type we don't care about right? https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:280: PidState* pes_state = new PidState; ditto. Perhaps an AddPidParser(pid, parser) helper function is in order. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:291: if (pes_pid != selected_video_pid_) { 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. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:309: audio_config_.reset(new AudioDecoderConfig()); nit: You shouldn't need a pointer here. You can just assign the variable to AudioDecoderConfig() if you want it to be invalid. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:316: if (selected_audio_pid_ > 0 && !audio_config_) { It seems like this should be DCHECK(!audio_config_.IsValidConfig() || selected_audio_pid_) instead. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:320: if (selected_video_pid_ > 0 && !video_config_) { ditto for video. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:326: // Buffers need to be emitted since these buffers do not necesseraly nit: s/ necesseraly/necessarily/ https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:332: if (selected_audio_pid_ > 0 && selected_video_pid_ > 0) { Removing the pointer usage would remove the need for most of this code. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:345: NOTREACHED(); This should be a DCHECK(audio_config_.IsValidConfig() || video_config_.IsValidConfig()) at the top of the method. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:348: if (!is_initialized_) { nit: reverse condition and return early. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:351: init_cb_.Run(true, base::TimeDelta()); nit: kInfiniteDuration() should be used here instead. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:359: // Since ChunkDemuxer currently handle only one audio and one video, nit: Do not reference ChunkDemuxer here. "Since StreamParsers can only emit one audio and one video," https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:363: if (pes_pid != selected_audio_pid_) { nit: It seems like this should be a DCHECK. We shouldn't be parsing PIDs we don't care about. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:389: if (pes_pid != selected_video_pid_) { ditto https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:418: StreamParser::BufferQueue::iterator it = video_buffer_queue_.begin(); 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. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:446: StartSegmentIfNeeded(); nit: Just move this into the block below and inline. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:450: !audio_buffer_queue_.empty()) { nit: reverse condition and return early. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... File media/mpeg2/mpeg2ts_stream_parser.h (right): https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.h:47: void RegisterPmt(int program_number, int pmt_pid); nit: Docs for all these methods. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.h:80: STLValueDeleter<PidMap> pids_deleter_; nit: This seems unnecessary. Just put a call to STLDeleteValues(&pids_) in the destructor. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.h:96: // Whether init_cb_ has been invoked. nit: References to variables should have || around them. (eg |init_cb_|) https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.h:98: // from the ChunkDemuxer perspective. nit: You shouldn't refer to ChunkDemuxer here. s/the ChunkDemuxer/this object's owner/? https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts.cc File media/mpeg2/es_parser_adts.cc (right): https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts... media/mpeg2/es_parser_adts.cc:18: namespace { nit: Move these into the mpeg2 namespace and use static const instead. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts... media/mpeg2/es_parser_adts.cc:58: int ExtractAdtsFrameSize(const uint8* adts_header) { nit: move these into the mpeg2 namespace and make them static. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts... media/mpeg2/es_parser_adts.cc:79: // Look for an ADTS syncword. nit: Please document the parameters and return value. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts... media/mpeg2/es_parser_adts.cc:84: if (max_offset < 0) { nit: remove {} for single line bodies here and everywhere else in this patch. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts... media/mpeg2/es_parser_adts.cc:159: memcpy(&raw_es_[old_size], buf, size); nit: use media::ByteQueue to avoid doing this stuff manually. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts... media/mpeg2/es_parser_adts.cc:194: LOG(WARNING) << "ADTS: pts not monotonic"; This seems like it should be a DCHECK or at least trigger a parser error. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts... media/mpeg2/es_parser_adts.cc:207: stream_parser_buffer->set_timestamp(current_pts); nit: Set the duration of the buffer too since it is known. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts... media/mpeg2/es_parser_adts.cc:213: (1000000 * kNumberSamplesPerAACFrame) / sampling_frequency_); Use media::AudioTimestampHelper for this type of computation. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts... media/mpeg2/es_parser_adts.cc:237: return; This should probably cause a parse error and print a log message since it isn't supported. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts... media/mpeg2/es_parser_adts.cc:244: #if 0 nit: Remove if this isn't going to be turned on in the initial implementation. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts.h File media/mpeg2/es_parser_adts.h (right): https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts... media/mpeg2/es_parser_adts.h:38: bool is_pts_valid, base::TimeDelta pts, nit: use kNoTimestamp() to signal invalid timestamps instead of using an extra bool. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_h264.cc File media/mpeg2/es_parser_h264.cc (right): https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_h264... media/mpeg2/es_parser_h264.cc:19: const int kExtendedSar = 255; nit: Use static const and move into the mpeg2ts namespace. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_h264... media/mpeg2/es_parser_h264.cc:29: class ByteReaderChainedBuffer { nit: Move into the mpeg2ts namespace. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_h264... media/mpeg2/es_parser_h264.cc:111: raw_es_.resize(old_size + size); use media::ByteQueue here and in all other places in this CL that you do this type of thing. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_h264.h File media/mpeg2/es_parser_h264.h (right): https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_h264... media/mpeg2/es_parser_h264.h:61: // Nal type. nit: This comment doesn't add any value. Please remove. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_h264... media/mpeg2/es_parser_h264.h:73: void EmitFrame(NalDescList::iterator cur_frame, nit: docs https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_h264... media/mpeg2/es_parser_h264.h:79: bool ProcessSPS(const uint8* buf, int size); nit: docs on return values https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/mpeg2ts_stream... File media/mpeg2/mpeg2ts_stream_parser.cc (right): https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/mpeg2ts_stream... media/mpeg2/mpeg2ts_stream_parser.cc:160: if (it == pids_.end() && ts_packet->pid() == 0) { nit: Use Mpeg2TsSectionParser::kPidPat here. You might consider making SpecialPid values top level constants in this namepace to avoid such a long name. :)
Here are the main changes: - Clean up the PID filter (the PidState is now a real class). - Queue both the audio/video buffers as well as the audio/video configurations to make buffer emission much simpler and to deal effectively with special cases that might happen before initialization is completed. - Use of ByteQueue. https://codereview.chromium.org/23566013/diff/1/media/filters/stream_parser_f... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/23566013/diff/1/media/filters/stream_parser_f... media/filters/stream_parser_factory.cc:21: #include "media/mpeg2/mpeg2ts_stream_parser.h" On 2013/08/29 20:44:24, acolwell wrote: > nit: This should be inside the USE_PROPRIETARY_CODECS define since it relies on > the proprietary codecs being compiled in. Done. https://codereview.chromium.org/23566013/diff/1/media/filters/stream_parser_f... media/filters/stream_parser_factory.cc:207: #if defined(ENABLE_MPEG2TS_STREAM_PARSER) On 2013/08/29 20:44:24, acolwell wrote: > nit: This should be in the USE_PROPRIETARY_CODECS section too. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_section_p... File media/mpeg2/mpeg2ts_section_parser.h (right): https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_section_p... media/mpeg2/mpeg2ts_section_parser.h:13: enum SpecialPid { On 2013/08/29 20:44:24, acolwell wrote: > nit: Add spec section reference for these constants. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_section_p... media/mpeg2/mpeg2ts_section_parser.h:23: virtual bool Parse(bool payload_unit_start_indicator, On 2013/08/29 20:44:24, acolwell wrote: > nit: Document return value. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_section_p... media/mpeg2/mpeg2ts_section_parser.h:26: // Flush. On 2013/08/29 20:44:24, acolwell wrote: > nit: This comment isn't very helpful. Please provide more detailed description > of what implementations are expected to do when they get this call. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... File media/mpeg2/mpeg2ts_stream_parser.cc (right): https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:24: enum StreamType { On 2013/08/29 20:44:24, acolwell wrote: > 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. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:65: DCHECK(init_cb_.is_null()); On 2013/08/29 20:44:24, acolwell wrote: > nit:DCHECK(!is_initialized_)? Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:82: LOG(INFO) << "Mpeg2TsStreamParser::Flush()"; On 2013/08/29 20:44:24, acolwell wrote: > nit: Use DVLOG everywhere instead of LOG. DVLOG(1) is typically used in the > other parsers for this type of message. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:90: delete pid_state; On 2013/08/29 20:44:24, acolwell wrote: > If you are deleting the pid_state, why do you need to call Flush()? 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. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:96: end_of_segment_cb_.Run(); On 2013/08/29 20:44:24, acolwell wrote: > You shouldn't need to make this call. Are you running into problems if you don't > call it? I was thinking it was needed. Checked without, it works. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:118: << "Flush: audio buffer queue not empty"; On 2013/08/29 20:44:24, acolwell wrote: > nit: Make these DCHECKS and place them right after the EmitRemainingBuffers() > call above. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:130: ts_buffer_.resize(old_size + size); On 2013/08/29 20:44:24, acolwell wrote: > Convert ts_buffer_ to a media::ByteQueue so you don't have to do this. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:149: if (!ts_packet.get()) { On 2013/08/29 20:44:24, acolwell wrote: > nit: You shouldn't need the get(). Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:152: remaining_size -= 1; On 2013/08/29 20:44:24, acolwell wrote: > nit: You should probably put an upper bound on invalid TS packets observed and > return false if too many are encountered in a row. Not sure if this is the right behavior yet. I would rather try to make the StreamParser as dumb as possible. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:156: VLOG(LOG_LEVEL_TS) On 2013/08/29 20:44:24, acolwell wrote: > nit: Use DVLOG instead of VLOG here and everywhere else. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:162: pids_.find(ts_packet->pid()); On 2013/08/29 20:44:24, acolwell wrote: > nit: This should fit on the line above. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:166: PidState* pat_state = new PidState; On 2013/08/29 20:44:24, acolwell wrote: > nit: Use scoped_ptr<PidState> here and .release() below. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:175: LOG_IF(WARNING, it == pids_.end()) << "Ignoring TS packet for pid: " On 2013/08/29 20:44:24, acolwell wrote: > nit: Just place an unconditional log statement in an else clause below. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:189: // Check whether we are on a TS packet boundary. On 2013/08/29 20:44:24, acolwell wrote: > nit: This comment seems stale. It doesn't appear that you are doing any checks > here. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:207: if (it != pids_.end()) { On 2013/08/29 20:44:24, acolwell wrote: > nit: remove {} here and on other 1 line statements below. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:215: << "More than one program is defined"; On 2013/08/29 20:44:24, acolwell wrote: > If more than one program is present then an error needs to be signalled. The MSE > spec states that only 1 program is allowed. 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). https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:219: PidState* pmt_state = new PidState; On 2013/08/29 20:44:24, acolwell wrote: > nit: Use scoped_ptr<PidState> here and release() below. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:245: if (pmt_pid == selected_pmt_pid_) { On 2013/08/29 20:44:24, acolwell wrote: > nit: reverse condition and early return. We only really need parsers for the > pids & programs we care about right? Done. I changed the behavior of the automatic PID filtering. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:247: (selected_audio_pid_ < 0 || pes_pid < selected_audio_pid_)) { On 2013/08/29 20:44:24, acolwell wrote: > If the selected_xxx_pid_ changes downward, shouldn't we destroy the state for > the pid that is no longer selected? 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. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:276: } On 2013/08/29 20:44:24, acolwell wrote: > nit: else return. We don't need state for a strem_type we don't care about > right? Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:280: PidState* pes_state = new PidState; On 2013/08/29 20:44:24, acolwell wrote: > ditto. Perhaps an AddPidParser(pid, parser) helper function is in order. Code has been slightly refactored, and this helper does not seem to be needed. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:291: if (pes_pid != selected_video_pid_) { On 2013/08/29 20:44:24, acolwell wrote: > 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. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:309: audio_config_.reset(new AudioDecoderConfig()); On 2013/08/29 20:44:24, acolwell wrote: > nit: You shouldn't need a pointer here. You can just assign the variable to > AudioDecoderConfig() if you want it to be invalid. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:316: if (selected_audio_pid_ > 0 && !audio_config_) { On 2013/08/29 20:44:24, acolwell wrote: > It seems like this should be DCHECK(!audio_config_.IsValidConfig() || > selected_audio_pid_) instead. I slightly changed the design so this comment should not be relevant anymore. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:320: if (selected_video_pid_ > 0 && !video_config_) { On 2013/08/29 20:44:24, acolwell wrote: > ditto for video. Ditto https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:326: // Buffers need to be emitted since these buffers do not necesseraly On 2013/08/29 20:44:24, acolwell wrote: > nit: s/ necesseraly/necessarily/ Not relevant anymore. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:332: if (selected_audio_pid_ > 0 && selected_video_pid_ > 0) { On 2013/08/29 20:44:24, acolwell wrote: > Removing the pointer usage would remove the need for most of this code. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:345: NOTREACHED(); On 2013/08/29 20:44:24, acolwell wrote: > This should be a DCHECK(audio_config_.IsValidConfig() || > video_config_.IsValidConfig()) at the top of the method. Removed. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:348: if (!is_initialized_) { On 2013/08/29 20:44:24, acolwell wrote: > nit: reverse condition and return early. 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. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:351: init_cb_.Run(true, base::TimeDelta()); On 2013/08/29 20:44:24, acolwell wrote: > nit: kInfiniteDuration() should be used here instead. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:359: // Since ChunkDemuxer currently handle only one audio and one video, On 2013/08/29 20:44:24, acolwell wrote: > nit: Do not reference ChunkDemuxer here. "Since StreamParsers can only emit one > audio and one video," Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:363: if (pes_pid != selected_audio_pid_) { On 2013/08/29 20:44:24, acolwell wrote: > nit: It seems like this should be a DCHECK. We shouldn't be parsing PIDs we > don't care about. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:389: if (pes_pid != selected_video_pid_) { On 2013/08/29 20:44:24, acolwell wrote: > ditto Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:418: StreamParser::BufferQueue::iterator it = video_buffer_queue_.begin(); On 2013/08/29 20:44:24, acolwell wrote: > 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. 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. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:446: StartSegmentIfNeeded(); On 2013/08/29 20:44:24, acolwell wrote: > nit: Just move this into the block below and inline. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:450: !audio_buffer_queue_.empty()) { On 2013/08/29 20:44:24, acolwell wrote: > nit: reverse condition and return early. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... File media/mpeg2/mpeg2ts_stream_parser.h (right): https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.h:47: void RegisterPmt(int program_number, int pmt_pid); On 2013/08/29 20:44:24, acolwell wrote: > nit: Docs for all these methods. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.h:80: STLValueDeleter<PidMap> pids_deleter_; On 2013/08/29 20:44:24, acolwell wrote: > nit: This seems unnecessary. Just put a call to STLDeleteValues(&pids_) in the > destructor. Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.h:96: // Whether init_cb_ has been invoked. On 2013/08/29 20:44:24, acolwell wrote: > nit: References to variables should have || around them. (eg |init_cb_|) Done. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.h:98: // from the ChunkDemuxer perspective. On 2013/08/29 20:44:24, acolwell wrote: > nit: You shouldn't refer to ChunkDemuxer here. s/the ChunkDemuxer/this object's > owner/? Done. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts.cc File media/mpeg2/es_parser_adts.cc (right): https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts... media/mpeg2/es_parser_adts.cc:18: namespace { On 2013/08/29 20:44:24, acolwell wrote: > nit: Move these into the mpeg2 namespace and use static const instead. According to http://www.chromium.org/developers/coding-style, unnamed namespace should be used for local items in a cc file. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts... media/mpeg2/es_parser_adts.cc:58: int ExtractAdtsFrameSize(const uint8* adts_header) { On 2013/08/29 20:44:24, acolwell wrote: > nit: move these into the mpeg2 namespace and make them static. ditto. If you have an updated coding guideline and/or you think the code should be consistent with what is done in other modules in media, please confirm your remark. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts... media/mpeg2/es_parser_adts.cc:79: // Look for an ADTS syncword. On 2013/08/29 20:44:24, acolwell wrote: > nit: Please document the parameters and return value. Done. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts... media/mpeg2/es_parser_adts.cc:84: if (max_offset < 0) { On 2013/08/29 20:44:24, acolwell wrote: > nit: remove {} for single line bodies here and everywhere else in this patch. Done. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts... media/mpeg2/es_parser_adts.cc:159: memcpy(&raw_es_[old_size], buf, size); On 2013/08/29 20:44:24, acolwell wrote: > nit: use media::ByteQueue to avoid doing this stuff manually. Done. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts... media/mpeg2/es_parser_adts.cc:194: LOG(WARNING) << "ADTS: pts not monotonic"; On 2013/08/29 20:44:24, acolwell wrote: > This seems like it should be a DCHECK or at least trigger a parser error. At the ES level, nothing is preventing an audio frame to have ordered timestamps (although it seems weird and unlikely to get such a stream). Having ordered timestamps is already enforced in the SourceBuffer and this is where all the verifications are done. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts... media/mpeg2/es_parser_adts.cc:207: stream_parser_buffer->set_timestamp(current_pts); On 2013/08/29 20:44:24, acolwell wrote: > nit: Set the duration of the buffer too since it is known. Done. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts... media/mpeg2/es_parser_adts.cc:237: return; On 2013/08/29 20:44:24, acolwell wrote: > This should probably cause a parse error and print a log message since it isn't > supported. I slightly changed the behavior. Now, ADTS syncword synchronization will filter unsupported frequency indices. In UpdateAudioConfiguration, I now use a DCHECK to verify I get only supported frequency indices. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts... media/mpeg2/es_parser_adts.cc:244: #if 0 On 2013/08/29 20:44:24, acolwell wrote: > nit: Remove if this isn't going to be turned on in the initial implementation. Done. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts.h File media/mpeg2/es_parser_adts.h (right): https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts... media/mpeg2/es_parser_adts.h:38: bool is_pts_valid, base::TimeDelta pts, On 2013/08/29 20:44:24, acolwell wrote: > nit: use kNoTimestamp() to signal invalid timestamps instead of using an extra > bool. Done. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_h264.cc File media/mpeg2/es_parser_h264.cc (right): https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_h264... media/mpeg2/es_parser_h264.cc:19: const int kExtendedSar = 255; On 2013/08/29 20:44:24, acolwell wrote: > nit: Use static const and move into the mpeg2ts namespace. http://www.chromium.org/developers/coding-style, unnamed namespace should be used for local items in a cc file. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_h264... media/mpeg2/es_parser_h264.cc:29: class ByteReaderChainedBuffer { On 2013/08/29 20:44:24, acolwell wrote: > nit: Move into the mpeg2ts namespace. ditto. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_h264... media/mpeg2/es_parser_h264.cc:111: raw_es_.resize(old_size + size); On 2013/08/29 20:44:24, acolwell wrote: > use media::ByteQueue here and in all other places in this CL that you do this > type of thing. Done. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_h264.h File media/mpeg2/es_parser_h264.h (right): https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_h264... media/mpeg2/es_parser_h264.h:61: // Nal type. On 2013/08/29 20:44:24, acolwell wrote: > nit: This comment doesn't add any value. Please remove. Done. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_h264... media/mpeg2/es_parser_h264.h:73: void EmitFrame(NalDescList::iterator cur_frame, On 2013/08/29 20:44:24, acolwell wrote: > nit: docs Done. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_h264... media/mpeg2/es_parser_h264.h:79: bool ProcessSPS(const uint8* buf, int size); On 2013/08/29 20:44:24, acolwell wrote: > nit: docs on return values Done. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/mpeg2ts_stream... File media/mpeg2/mpeg2ts_stream_parser.cc (right): https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/mpeg2ts_stream... media/mpeg2/mpeg2ts_stream_parser.cc:160: if (it == pids_.end() && ts_packet->pid() == 0) { On 2013/08/29 20:44:24, acolwell wrote: > nit: Use Mpeg2TsSectionParser::kPidPat here. You might consider making > SpecialPid values top level constants in this namepace to avoid such a long > name. :) Done.
Forgot a couple of cleanups. https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/es_parser_adt... File media/mpeg2/es_parser_adts.cc (right): https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:182: } Brackets un-needed https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:256: channel_configuration_ = channel_configuration; Instead of {sampling_frequency, channel_configuration}, should use a |current_audio_decoder_config_| and use IsValid & Matches for updates. https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/mpeg2ts_crc.cc File media/mpeg2/mpeg2ts_crc.cc (right): https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/mpeg2ts_crc.c... media/mpeg2/mpeg2ts_crc.cc:14: void Mpeg2TsCrc::Update(uint32 data, int nbits) { DCHECK_LE(nbits, 32); https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/mpeg2ts_pat.cc File media/mpeg2/mpeg2ts_pat.cc (right): https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/mpeg2ts_pat.c... media/mpeg2/mpeg2ts_pat.cc:97: } Remove brackets. https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/mpeg2ts_pat.c... media/mpeg2/mpeg2ts_pat.cc:103: } Remove brackets. https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/mpeg2ts_pat.h File media/mpeg2/mpeg2ts_pat.h (right): https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/mpeg2ts_pat.h... media/mpeg2/mpeg2ts_pat.h:33: // or is incremented by 1. Remove TODO as it's now done. https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/mpeg2ts_pmt.cc File media/mpeg2/mpeg2ts_pmt.cc (right): https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/mpeg2ts_pmt.c... media/mpeg2/mpeg2ts_pmt.cc:148: it != pid_map.end(); ++it) { Might fit in one line. https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/mpeg2ts_pmt.c... media/mpeg2/mpeg2ts_pmt.cc:150: } Remove brackets.
Here are my next batch of comments. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... File media/mpeg2/mpeg2ts_stream_parser.cc (right): https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:215: << "More than one program is defined"; On 2013/09/04 01:37:13, damienv1 wrote: > On 2013/08/29 20:44:24, acolwell wrote: > > If more than one program is present then an error needs to be signalled. The > MSE > > spec states that only 1 program is allowed. > > 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). Multiple programs are not allowed by the spec. Signal an error. https://codereview.chromium.org/23566013/diff/1/media/mpeg2/mpeg2ts_stream_pa... media/mpeg2/mpeg2ts_stream_parser.cc:348: if (!is_initialized_) { On 2013/09/04 01:37:13, damienv1 wrote: > On 2013/08/29 20:44:24, acolwell wrote: > > nit: reverse condition and return early. > > 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. 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. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts.cc File media/mpeg2/es_parser_adts.cc (right): https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts... media/mpeg2/es_parser_adts.cc:58: int ExtractAdtsFrameSize(const uint8* adts_header) { On 2013/09/04 01:37:14, damienv1 wrote: > On 2013/08/29 20:44:24, acolwell wrote: > > nit: move these into the mpeg2 namespace and make them static. > > ditto. > If you have an updated coding guideline and/or you think the code should be > consistent with what is done in other modules in media, please confirm your > remark. The majority of the media code uses static instead of anonymous namespaces. This request is to make the code consistent with the norms in the media code. https://codereview.chromium.org/23566013/diff/3001/media/mpeg2/es_parser_adts... media/mpeg2/es_parser_adts.cc:194: LOG(WARNING) << "ADTS: pts not monotonic"; On 2013/09/04 01:37:14, damienv1 wrote: > On 2013/08/29 20:44:24, acolwell wrote: > > This seems like it should be a DCHECK or at least trigger a parser error. > > At the ES level, nothing is preventing an audio frame to have ordered timestamps > (although it seems weird and unlikely to get such a stream). > > Having ordered timestamps is already enforced in the SourceBuffer and this is > where all the verifications are done. My concern is that having code here makes me think that this is an exceptional situation that needs action. If we don't actually need action here and we are relying on the SourceBuffer code, then it seems like this should just be removed. https://codereview.chromium.org/23566013/diff/25001/media/filters/stream_pars... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/23566013/diff/25001/media/filters/stream_pars... media/filters/stream_parser_factory.cc:19: #include "media/mpeg2/mpeg2ts_stream_parser.h" nit: Fix include order. https://codereview.chromium.org/23566013/diff/25001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/23566013/diff/25001/media/media.gyp#newcode863 media/media.gyp:863: 'mpeg2/es_parser.h', nit: How about making the directory mp2t to be consistent w/ the extention & mime subtype like webm & mp4 are? Another option is mp2 so that it is consistent with mp4 and the soon to be landed mp3. Either is fine with me, but the namespace is supposed to match the directory name. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser.h File media/mpeg2/es_parser.h (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser.h#n... media/mpeg2/es_parser.h:21: // of Mpeg2 TS be useful as an additional argument ? nit: Does this TODO need to stay? It isn't a call to action. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... File media/mpeg2/es_parser_adts.cc (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:21: const int kAdtsHeaderMinSize = 7; nit: Please use static here and below instead of an anon namespace to be consistent with the majority of the media/ code. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:56: const int kNumberSamplesPerHeAACFrame = 2048; nit: This and the one below do not appear to be getting used. Please remove or update the code to use them. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:60: int frame_size = nit: The local vars aren't needed here and below. Just return the values. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:106: continue; If these aren't supported, shouldn't we trigger an error? Otherwise we'll just keep consuming content that can't be played w/o the user knowing why. At least signalling an error indicates that the content is not playable. In general we should be strict about what we accept and clearly error out when we encounter situations we don't support. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:180: if (frame_size > remaining_size) { nit: remove unnecessary {} https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:199: base::TimeDelta frame_duration = nit: Use AudioTimestampHelper for these computations. Since this code always rounds down you'll run into cases where timestamp() + duration() does not equal the timestamp() on the next buffer. AudioTimestampHelper allows you to avoid this problem and construct buffer timestamps and durations that have the proper rounding. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:204: if (!first_frame_ && current_pts < last_frame_pts_) { nit: remove first_frame_ & last_frame_pts_ if you are really intending to rely on SourceBuffer to handle non-increasing PTS. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:223: estimated_pts_ = current_pts + frame_duration; Replace this with AudioTimestampHelper for more accurate timestamp generation. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:258: LOG(INFO) << "Sampling frequency: " << samples_per_second; nit: Use DVLOG https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:273: if (nbytes <= 0) nit: Add DCHECK_GE(nbytes, 0) since a negative value is an error in the code. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adts.h File media/mpeg2/es_parser_adts.h (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.h:9: #include <vector> Add #include <utility> for std::pair https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... File media/mpeg2/es_parser_h264.cc (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:22: const int kTableSarWidth[14] = { nit: static const please. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:30: class ByteReaderChainedBuffer { docs https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:41: DCHECK_LT(offset, size0_ + size1_); nit: I think using GetSize() would make this more clear. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:48: return (size0_ + size1_); nit: remove (). https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:92: // Link position |raw_es_.size()| in the ES stream with a timing descriptor. nit: s/raw_es_.size()/raw_es_size/? https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:93: // HLS recommandation: "In AVC video, you should have both a DTS and a nit: s/recommandation/recommendation/ https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:96: // DTS are not valid ? We should error out. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:131: LOG_IF(WARNING, (*it0)->position != 0) nit: use DVLOG https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:143: es_byte_queue_.Push(buf, size); Why don't you just unconditionally push at the top before the first Peek() instead of doing it in several places in this method? https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:154: // Force emitting the last access unit (even it might be incomplete). nit: s/it/if it/ https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:156: const uint8* raw_es = NULL; nit: no need to initialize these values here and elsewhere since the Peek() immediatly overwrites them. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:158: NalDescList::iterator cur_frame = *(access_unit_list.begin()); 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(). https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:169: ByteReaderChainedBuffer byte_reader( 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. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:223: NalDescList::iterator cur_frame, nit: const& https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:236: while (!timing_desc_list_.empty() && 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. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:258: NalParser(&raw_es[cur_nal_position], nal_size); This should signal an error when it encounters invalid NALs https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:278: while (!nal_desc_list_.empty() && 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. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:306: return; Shouldn't this signal an error since it appears that bad data is in the bytestream? https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:308: DCHECK_EQ(buf[0], 0); These should signal an error since it represents bad data. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:321: return; ditto https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:329: return; should trigger an error. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:342: return; should trigger an error. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:344: ProcessSPS(buf, size); false return values from these methods should trigger an error. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:367: int constraint_setX_flag; 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. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:387: LOG(FATAL) << "pic_order_cnt_type = 1 not supported yet"; return false here since it isn't supported yet. Use DVLOG() or MEDIA_LOG() here instead. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:392: int gaps_in_frame_num_value_allowed_flag; nit: I think grouping the variable declarations at the top would make this more readable. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:447: << " sar_height=" << sar_height; nit: Trigger an error for things that are not supported yet. Use MEDIA_LOG() for notification. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:513: uint32 slice_type; ditto https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:520: return true; Why are we doing this parsing? Just to verify that the NAL is larg enough to contain these fields? https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:525: // TODO(damienv): this should be a member function of BitReader. 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. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:529: for (zero_count = 0; ; zero_count++) { nit: the zero_count = 0 is not needed here. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:531: if (!bit_reader->ReadBits(1, &one_bit)) { nit: remove {} here and below. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:538: Add the following here to avoid undefined behavior below. if (zero_count > 31) return false; https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:548: value += (1 << (zero_count-1 - bit_count)); nit: space on either side of - https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h264.h File media/mpeg2/es_parser_h264.h (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.h:37: typedef base::Callback<void(scoped_refptr<StreamParserBuffer>)> EmitBufferCB; nit: Perhaps move this to EsParser.h since it is defined in multiple .h files? https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.h:70: nit: remove extra line. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.h:72: void FindNals(const uint8* buf, int size); docs for this and the method below. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.h:78: // and |nxt_frame| (not included). nit: The "between" and "not included" seems to make this comment a little ambiguous. Perhaps add (included) next to cur_frame so it is clear that cur_frame is actually the start and not the frame after it? https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_common.h File media/mpeg2/mpeg2ts_common.h (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_commo... media/mpeg2/mpeg2ts_common.h:15: LOG(WARNING) << "Failure while parsing Mpeg2TS: " << #x; \ nit: use DVLOG https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_crc.cc File media/mpeg2/mpeg2ts_crc.cc (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_crc.c... media/mpeg2/mpeg2ts_crc.cc:15: const uint32_t kCrcPoly = 0x4c11db7; DCHECK_GT(nbits, 0); DCHECK_LE(nbits, 32); https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_crc.h File media/mpeg2/mpeg2ts_crc.h (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_crc.h... media/mpeg2/mpeg2ts_crc.h:14: // Update the CRC using the LSB of |data|. nit: s/LSB/LSb/ since you are referring to the least significant bits and not bytes? https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pat.cc File media/mpeg2/mpeg2ts_pat.cc (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pat.c... media/mpeg2/mpeg2ts_pat.cc:26: LOG(WARNING) << "remaining_size=" << remaining_size; nit: DVLOG https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pat.c... media/mpeg2/mpeg2ts_pat.cc:31: bool section_syntax_indicator; nit: put all declarations together to make this easier to read. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pat.c... media/mpeg2/mpeg2ts_pat.cc:32: DCHECK(bit_reader->ReadBits(1, §ion_syntax_indicator)); These should be RCHECK otherwise these won't run in release builds. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pat.c... media/mpeg2/mpeg2ts_pat.cc:64: LOG(WARNING) << "remaining_size=" << remaining_size; DVLOG here and all cases below. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pat.c... media/mpeg2/mpeg2ts_pat.cc:83: DCHECK(bit_reader->ReadBits(16, &program_number_array[k])); RCHECK here and below https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pat.c... media/mpeg2/mpeg2ts_pat.cc:85: DCHECK(bit_reader->ReadBits(3, &reserved)); RCHECK reserved value https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pat.c... media/mpeg2/mpeg2ts_pat.cc:98: if (version_number == version_number_) { nit: remove {} https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pat.c... media/mpeg2/mpeg2ts_pat.cc:115: // HLS: "Transport Stream segments MUST contain a single MPEG-2 Program." If this code is detecting multiple programs here, it should return false to trigger an error. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.cc File media/mpeg2/mpeg2ts_pes.cc (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:14: namespace { nit: Use static to match the style of the rest of the media code. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:18: int64 UnrollTimestamp(int64 previous_unrolled_time, docs https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:37: if (diff0 < 0) { nit: use std::abs() above instead of doing this. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:56: if (diff2 < min_diff) { nit: remove {} https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:59: LOG_IF(INFO, unrolled_time != time1) Use DVLOG https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:75: bool is_valid = nit: variable isn't necessary. Just use return here. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:83: int64 timestamp = ditto https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:119: int raw_pes_size = 0; nit: initialization is not neccesary here since it is immediately overwritten by the Peek(). https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:152: int raw_pes_size = 0; nit: initialization not needed here and below. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:186: int raw_pes_size = 0; ditto https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:198: DCHECK(bit_reader.ReadBits(24, &packet_start_code_prefix)); Use RCHECK here and in every similar usage below. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:203: remaining_size -= 6; Instead of modifying remaining_size in several places here, I believe it would be less error prone to just use reader.bits_available() to compute this value down at the bottom. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:259: if (remaining_size < 5) { nit: remove {} here and in several one-line ifs below. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:312: LOG(WARNING) << "Invalid PES header length"; DVLOG https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:343: es_parser_->Parse(&raw_pes[es_offset], es_size, errors returned by the parser should be bubbled up to the caller. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.h File media/mpeg2/mpeg2ts_pes.h (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.h... media/mpeg2/mpeg2ts_pes.h:24: explicit Mpeg2TsPesParser(EsParser* es_parser); nit: use scoped_ptr<EsParser> here https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.h... media/mpeg2/mpeg2ts_pes.h:33: bool Emit(bool emit_for_unknown_size); docs https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pmt.cc File media/mpeg2/mpeg2ts_pmt.cc (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pmt.c... media/mpeg2/mpeg2ts_pmt.cc:25: DCHECK_GE(bit_reader->bits_available(), 8 * 8); RCHECK here and everywhere below. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pmt.c... media/mpeg2/mpeg2ts_pmt.cc:26: int table_id; group declarations above to improve readability. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pmt.c... media/mpeg2/mpeg2ts_pmt.cc:67: LOG(WARNING) << "remaining_size=" << remaining_size; DVLOG here and below https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pmt.c... media/mpeg2/mpeg2ts_pmt.cc:107: int reserved; group declarations https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pmt.c... media/mpeg2/mpeg2ts_pmt.cc:119: pid_map.insert(std::pair<int,int>(pid_es, stream_type)); nit: space after , https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_psi.cc File media/mpeg2/mpeg2ts_psi.cc (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_psi.c... media/mpeg2/mpeg2ts_psi.cc:42: int raw_psi_size = 0; nit: remove initialization since it is immediately overridden. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_psi.c... media/mpeg2/mpeg2ts_psi.cc:45: LOG_IF(WARNING, raw_psi_size > 0) DVLOG here and below https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_psi.c... media/mpeg2/mpeg2ts_psi.cc:65: if (size == 0) { nit: remove{} here and all 1 line ifs below https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_psi.c... media/mpeg2/mpeg2ts_psi.cc:71: int raw_psi_size = 0; ditto https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_psi.c... media/mpeg2/mpeg2ts_psi.cc:100: crc.Update(raw_psi[k], 8); This appears to be the only call site for this method. Since the CRC is only ever updated a byte at a time, change the signature of Update() to just take a uint8 to reflect that this is the only way this method is being used right now. If the required usage changes in the future, then we can always revert the signature. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... File media/mpeg2/mpeg2ts_stream_parser.cc (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.cc:49: Mpeg2TsSectionParser* section_parser); nit: Use scoped_ptr<Mpeg2TsSectionParser> here so the comment is unnecessary. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.cc:57: void Flush(); nit: EmitPendingBuffersAndReset() seems like a more appropriate name. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.cc:64: bool IsEnabled(); nit: add const https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.cc:66: PidType pid_type() { return pid_type_; } nit: add const here and in any other similar accessors that I may have missed. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.cc:105: bool parse_result = section_parser_->Parse( nit: just return here since nothing else uses parse_result https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.cc:135: //section_parser_->ResetState(); Remove if this really isn't needed. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.cc:205: audio_buffer_queue_.clear(); nit: If we are DCHECKing that these queues are empty, then we shouldn't need to clear() them here. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.cc:233: const uint8* ts_buffer = NULL; nit: init not needed https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.cc:243: LOG(WARNING) << "Packet not aligned on a TS syncword:" DVLOG here and everywhere else https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.cc:494: audio_buffer_with_config.config = audio_config_; You shouldn't need to do this. Just emit the pending buffers when the new config arrives. Then you don't have to make a copy of the config for every buffer or keep track of which configs are associated with what buffers. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... File media/mpeg2/mpeg2ts_stream_parser.h (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.h:8: #include <list> Add #include <map> https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/es_parser.h File media/mpeg2/es_parser.h (right): https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/es_parser.h#n... media/mpeg2/es_parser.h:23: virtual void Parse(const uint8* buf, int size, Should return a value so that parse errors can be reported.
- Address comments from patch #3 & #4. - Use audio timestamp helpers. - Code simplified in the H264 ES parser. - Some cleanup. - Better error handling. https://codereview.chromium.org/23566013/diff/25001/media/filters/stream_pars... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/23566013/diff/25001/media/filters/stream_pars... media/filters/stream_parser_factory.cc:19: #include "media/mpeg2/mpeg2ts_stream_parser.h" On 2013/09/05 18:29:10, acolwell wrote: > nit: Fix include order. mp4 is not in include order as well. I followed the same scheme: put the conditionally included files after the others. https://codereview.chromium.org/23566013/diff/25001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/23566013/diff/25001/media/media.gyp#newcode863 media/media.gyp:863: 'mpeg2/es_parser.h', On 2013/09/05 18:29:10, acolwell wrote: > nit: How about making the directory mp2t to be consistent w/ the extention & > mime subtype like webm & mp4 are? Another option is mp2 so that it is consistent > with mp4 and the soon to be landed mp3. Either is fine with me, but the > namespace is supposed to match the directory name. I would rather use the mime type: mp2t. That seems a more consistent way to name directories in the future. I will update the directory and the namespace. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser.h File media/mpeg2/es_parser.h (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser.h#n... media/mpeg2/es_parser.h:21: // of Mpeg2 TS be useful as an additional argument ? On 2013/09/05 18:29:10, acolwell wrote: > nit: Does this TODO need to stay? It isn't a call to action. Removed. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... File media/mpeg2/es_parser_adts.cc (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:21: const int kAdtsHeaderMinSize = 7; On 2013/09/05 18:29:10, acolwell wrote: > nit: Please use static here and below instead of an anon namespace to be > consistent with the majority of the media/ code. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:56: const int kNumberSamplesPerHeAACFrame = 2048; On 2013/09/05 18:29:10, acolwell wrote: > nit: This and the one below do not appear to be getting used. Please remove or > update the code to use them. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:60: int frame_size = On 2013/09/05 18:29:10, acolwell wrote: > nit: The local vars aren't needed here and below. Just return the values. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:106: continue; On 2013/09/05 18:29:10, acolwell wrote: > If these aren't supported, shouldn't we trigger an error? Otherwise we'll just > keep consuming content that can't be played w/o the user knowing why. At least > signalling an error indicates that the content is not playable. > > In general we should be strict about what we accept and clearly error out when > we encounter situations we don't support. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:180: if (frame_size > remaining_size) { On 2013/09/05 18:29:10, acolwell wrote: > nit: remove unnecessary {} Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:199: base::TimeDelta frame_duration = On 2013/09/05 18:29:10, acolwell wrote: > nit: Use AudioTimestampHelper for these computations. Since this code always > rounds down you'll run into cases where timestamp() + duration() does not equal > the timestamp() on the next buffer. AudioTimestampHelper allows you to avoid > this problem and construct buffer timestamps and durations that have the proper > rounding. That's correct. However, I am not sure the AudioTimestampHelper is correct either: to avoid any rounding issue, it should not use "double" at all, all the operations should be integer operations. I agree this is an implementation issue. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:204: if (!first_frame_ && current_pts < last_frame_pts_) { On 2013/09/05 18:29:10, acolwell wrote: > nit: remove first_frame_ & last_frame_pts_ if you are really intending to rely > on SourceBuffer to handle non-increasing PTS. Removed (I used it mainly for my own debugging). https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:223: estimated_pts_ = current_pts + frame_duration; On 2013/09/05 18:29:10, acolwell wrote: > Replace this with AudioTimestampHelper for more accurate timestamp generation. ok. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:258: LOG(INFO) << "Sampling frequency: " << samples_per_second; On 2013/09/05 18:29:10, acolwell wrote: > nit: Use DVLOG Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:273: if (nbytes <= 0) On 2013/09/05 18:29:10, acolwell wrote: > nit: Add DCHECK_GE(nbytes, 0) since a negative value is an error in the code. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adts.h File media/mpeg2/es_parser_adts.h (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.h:9: #include <vector> On 2013/09/05 18:29:10, acolwell wrote: > Add #include <utility> for std::pair Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... File media/mpeg2/es_parser_h264.cc (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:22: const int kTableSarWidth[14] = { On 2013/09/05 18:29:10, acolwell wrote: > nit: static const please. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:30: class ByteReaderChainedBuffer { On 2013/09/05 18:29:10, acolwell wrote: > docs Code removed. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:41: DCHECK_LT(offset, size0_ + size1_); On 2013/09/05 18:29:10, acolwell wrote: > nit: I think using GetSize() would make this more clear. Code removed. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:48: return (size0_ + size1_); On 2013/09/05 18:29:10, acolwell wrote: > nit: remove (). Code removed. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:92: // Link position |raw_es_.size()| in the ES stream with a timing descriptor. On 2013/09/05 18:29:10, acolwell wrote: > nit: s/raw_es_.size()/raw_es_size/? Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:93: // HLS recommandation: "In AVC video, you should have both a DTS and a On 2013/09/05 18:29:10, acolwell wrote: > nit: s/recommandation/recommendation/ Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:96: // DTS are not valid ? On 2013/09/05 18:29:10, acolwell wrote: > We should error out. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:131: LOG_IF(WARNING, (*it0)->position != 0) On 2013/09/05 18:29:10, acolwell wrote: > nit: use DVLOG Use DLOG instead. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:143: es_byte_queue_.Push(buf, size); On 2013/09/05 18:29:10, acolwell wrote: > Why don't you just unconditionally push at the top before the first Peek() > instead of doing it in several places in this method? 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). https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:154: // Force emitting the last access unit (even it might be incomplete). On 2013/09/05 18:29:10, acolwell wrote: > nit: s/it/if it/ Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:156: const uint8* raw_es = NULL; On 2013/09/05 18:29:10, acolwell wrote: > nit: no need to initialize these values here and elsewhere since the Peek() > immediatly overwrites them. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:169: ByteReaderChainedBuffer byte_reader( On 2013/09/05 18:29:10, acolwell wrote: > 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. 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). https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:223: NalDescList::iterator cur_frame, On 2013/09/05 18:29:10, acolwell wrote: > nit: const& Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:236: while (!timing_desc_list_.empty() && On 2013/09/05 18:29:10, acolwell wrote: > 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. 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). https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:258: NalParser(&raw_es[cur_nal_position], nal_size); On 2013/09/05 18:29:10, acolwell wrote: > This should signal an error when it encounters invalid NALs Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:278: while (!nal_desc_list_.empty() && On 2013/09/05 18:29:10, acolwell wrote: > 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. 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. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:306: return; On 2013/09/05 18:29:10, acolwell wrote: > Shouldn't this signal an error since it appears that bad data is in the > bytestream? Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:308: DCHECK_EQ(buf[0], 0); On 2013/09/05 18:29:10, acolwell wrote: > These should signal an error since it represents bad data. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:321: return; On 2013/09/05 18:29:10, acolwell wrote: > ditto Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:329: return; On 2013/09/05 18:29:10, acolwell wrote: > should trigger an error. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:342: return; On 2013/09/05 18:29:10, acolwell wrote: > should trigger an error. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:344: ProcessSPS(buf, size); On 2013/09/05 18:29:10, acolwell wrote: > false return values from these methods should trigger an error. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:367: int constraint_setX_flag; On 2013/09/05 18:29:10, acolwell wrote: > 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. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:387: LOG(FATAL) << "pic_order_cnt_type = 1 not supported yet"; On 2013/09/05 18:29:10, acolwell wrote: > return false here since it isn't supported yet. Use DVLOG() or MEDIA_LOG() here > instead. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:392: int gaps_in_frame_num_value_allowed_flag; On 2013/09/05 18:29:10, acolwell wrote: > nit: I think grouping the variable declarations at the top would make this more > readable. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:447: << " sar_height=" << sar_height; On 2013/09/05 18:29:10, acolwell wrote: > nit: Trigger an error for things that are not supported yet. Use MEDIA_LOG() for > notification. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:513: uint32 slice_type; On 2013/09/05 18:29:10, acolwell wrote: > ditto Code removed. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:520: return true; On 2013/09/05 18:29:10, acolwell wrote: > Why are we doing this parsing? Just to verify that the NAL is larg enough to > contain these fields? 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. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:525: // TODO(damienv): this should be a member function of BitReader. On 2013/09/05 18:29:10, acolwell wrote: > 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. Made it a subclass of BitReader. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:529: for (zero_count = 0; ; zero_count++) { On 2013/09/05 18:29:10, acolwell wrote: > nit: the zero_count = 0 is not needed here. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:531: if (!bit_reader->ReadBits(1, &one_bit)) { On 2013/09/05 18:29:10, acolwell wrote: > nit: remove {} here and below. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:538: On 2013/09/05 18:29:10, acolwell wrote: > Add the following here to avoid undefined behavior below. > > if (zero_count > 31) > return false; Good remark ! https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:548: value += (1 << (zero_count-1 - bit_count)); On 2013/09/05 18:29:10, acolwell wrote: > nit: space on either side of - Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h264.h File media/mpeg2/es_parser_h264.h (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.h:37: typedef base::Callback<void(scoped_refptr<StreamParserBuffer>)> EmitBufferCB; On 2013/09/05 18:29:10, acolwell wrote: > nit: Perhaps move this to EsParser.h since it is defined in multiple .h files? Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.h:70: On 2013/09/05 18:29:10, acolwell wrote: > nit: remove extra line. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.h:72: void FindNals(const uint8* buf, int size); On 2013/09/05 18:29:10, acolwell wrote: > docs for this and the method below. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.h:78: // and |nxt_frame| (not included). On 2013/09/05 18:29:10, acolwell wrote: > nit: The "between" and "not included" seems to make this comment a little > ambiguous. Perhaps add (included) next to cur_frame so it is clear that > cur_frame is actually the start and not the frame after it? Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_common.h File media/mpeg2/mpeg2ts_common.h (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_commo... media/mpeg2/mpeg2ts_common.h:15: LOG(WARNING) << "Failure while parsing Mpeg2TS: " << #x; \ On 2013/09/05 18:29:10, acolwell wrote: > nit: use DVLOG Use DLOG(WARNING) instead. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_crc.cc File media/mpeg2/mpeg2ts_crc.cc (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_crc.c... media/mpeg2/mpeg2ts_crc.cc:15: const uint32_t kCrcPoly = 0x4c11db7; On 2013/09/05 18:29:10, acolwell wrote: > DCHECK_GT(nbits, 0); > DCHECK_LE(nbits, 32); Function signature changed: now always receive 8 bits of data at a time. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_crc.h File media/mpeg2/mpeg2ts_crc.h (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_crc.h... media/mpeg2/mpeg2ts_crc.h:14: // Update the CRC using the LSB of |data|. On 2013/09/05 18:29:10, acolwell wrote: > nit: s/LSB/LSb/ since you are referring to the least significant bits and not > bytes? I am referring to bits. Since the signature of this functions has changed, I don't use LSb anymore ;) https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pat.cc File media/mpeg2/mpeg2ts_pat.cc (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pat.c... media/mpeg2/mpeg2ts_pat.cc:26: LOG(WARNING) << "remaining_size=" << remaining_size; On 2013/09/05 18:29:10, acolwell wrote: > nit: DVLOG Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pat.c... media/mpeg2/mpeg2ts_pat.cc:31: bool section_syntax_indicator; On 2013/09/05 18:29:10, acolwell wrote: > nit: put all declarations together to make this easier to read. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pat.c... media/mpeg2/mpeg2ts_pat.cc:32: DCHECK(bit_reader->ReadBits(1, §ion_syntax_indicator)); On 2013/09/05 18:29:10, acolwell wrote: > These should be RCHECK otherwise these won't run in release builds. Running this is safe in release builds: I just check before that I had at least 8*8 = 64 bits available. Pros: check is done only once instead of using RCHECK for every ReadBits call. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pat.c... media/mpeg2/mpeg2ts_pat.cc:64: LOG(WARNING) << "remaining_size=" << remaining_size; On 2013/09/05 18:29:10, acolwell wrote: > DVLOG here and all cases below. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pat.c... media/mpeg2/mpeg2ts_pat.cc:83: DCHECK(bit_reader->ReadBits(16, &program_number_array[k])); On 2013/09/05 18:29:10, acolwell wrote: > RCHECK here and below By design, we should not run out of bits in this section, that's why I use a DCHECK. pmt_pid_count is based on the section_length which itself has been verified to be equal to or less than the remaining number of bits. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pat.c... media/mpeg2/mpeg2ts_pat.cc:85: DCHECK(bit_reader->ReadBits(3, &reserved)); On 2013/09/05 18:29:10, acolwell wrote: > RCHECK reserved value Ditto. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pat.c... media/mpeg2/mpeg2ts_pat.cc:98: if (version_number == version_number_) { On 2013/09/05 18:29:10, acolwell wrote: > nit: remove {} Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pat.c... media/mpeg2/mpeg2ts_pat.cc:115: // HLS: "Transport Stream segments MUST contain a single MPEG-2 Program." On 2013/09/05 18:29:10, acolwell wrote: > If this code is detecting multiple programs here, it should return false to > trigger an error. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.cc File media/mpeg2/mpeg2ts_pes.cc (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:14: namespace { On 2013/09/05 18:29:10, acolwell wrote: > nit: Use static to match the style of the rest of the media code. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:18: int64 UnrollTimestamp(int64 previous_unrolled_time, On 2013/09/05 18:29:10, acolwell wrote: > docs Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:37: if (diff0 < 0) { On 2013/09/05 18:29:10, acolwell wrote: > nit: use std::abs() above instead of doing this. I prefer to keep the way it is. Before C++11, std::abs is defined for "int" and "long int" but there is no guarantee it is defined for int64 (which might be a long long int). Please let me know if you think otherwise. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:56: if (diff2 < min_diff) { On 2013/09/05 18:29:10, acolwell wrote: > nit: remove {} Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:59: LOG_IF(INFO, unrolled_time != time1) On 2013/09/05 18:29:10, acolwell wrote: > Use DVLOG I removed this piece of code. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:75: bool is_valid = On 2013/09/05 18:29:10, acolwell wrote: > nit: variable isn't necessary. Just use return here. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:83: int64 timestamp = On 2013/09/05 18:29:10, acolwell wrote: > ditto Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:119: int raw_pes_size = 0; On 2013/09/05 18:29:10, acolwell wrote: > nit: initialization is not neccesary here since it is immediately overwritten by > the Peek(). Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:152: int raw_pes_size = 0; On 2013/09/05 18:29:10, acolwell wrote: > nit: initialization not needed here and below. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:186: int raw_pes_size = 0; On 2013/09/05 18:29:10, acolwell wrote: > ditto Not relevant anymore (code removed). https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:198: DCHECK(bit_reader.ReadBits(24, &packet_start_code_prefix)); On 2013/09/05 18:29:10, acolwell wrote: > Use RCHECK here and in every similar usage below. RCHECK should be used if the remaining size is not checked before. In this case, I check the size before reading a whole section (to avoid doing a possible early exit at each line using RCHECK in production releases). https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:203: remaining_size -= 6; On 2013/09/05 18:29:10, acolwell wrote: > Instead of modifying remaining_size in several places here, I believe it would > be less error prone to just use reader.bits_available() to compute this value > down at the bottom. Agree. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:259: if (remaining_size < 5) { On 2013/09/05 18:29:10, acolwell wrote: > nit: remove {} here and in several one-line ifs below. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:312: LOG(WARNING) << "Invalid PES header length"; On 2013/09/05 18:29:10, acolwell wrote: > DVLOG Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:343: es_parser_->Parse(&raw_pes[es_offset], es_size, On 2013/09/05 18:29:10, acolwell wrote: > errors returned by the parser should be bubbled up to the caller. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.h File media/mpeg2/mpeg2ts_pes.h (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.h... media/mpeg2/mpeg2ts_pes.h:24: explicit Mpeg2TsPesParser(EsParser* es_parser); On 2013/09/05 18:29:10, acolwell wrote: > nit: use scoped_ptr<EsParser> here Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pes.h... media/mpeg2/mpeg2ts_pes.h:33: bool Emit(bool emit_for_unknown_size); On 2013/09/05 18:29:10, acolwell wrote: > docs Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pmt.cc File media/mpeg2/mpeg2ts_pmt.cc (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pmt.c... media/mpeg2/mpeg2ts_pmt.cc:25: DCHECK_GE(bit_reader->bits_available(), 8 * 8); On 2013/09/05 18:29:10, acolwell wrote: > RCHECK here and everywhere below. Please let me know if you prefer using RCHECK at the price of an overhead (compared to checking only once the available size and using DCHECK). https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pmt.c... media/mpeg2/mpeg2ts_pmt.cc:26: int table_id; On 2013/09/05 18:29:10, acolwell wrote: > group declarations above to improve readability. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pmt.c... media/mpeg2/mpeg2ts_pmt.cc:67: LOG(WARNING) << "remaining_size=" << remaining_size; On 2013/09/05 18:29:10, acolwell wrote: > DVLOG here and below Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pmt.c... media/mpeg2/mpeg2ts_pmt.cc:107: int reserved; On 2013/09/05 18:29:10, acolwell wrote: > group declarations Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pmt.c... media/mpeg2/mpeg2ts_pmt.cc:119: pid_map.insert(std::pair<int,int>(pid_es, stream_type)); On 2013/09/05 18:29:10, acolwell wrote: > nit: space after , Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_psi.cc File media/mpeg2/mpeg2ts_psi.cc (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_psi.c... media/mpeg2/mpeg2ts_psi.cc:42: int raw_psi_size = 0; On 2013/09/05 18:29:10, acolwell wrote: > nit: remove initialization since it is immediately overridden. Not relevant anymore (code removed). https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_psi.c... media/mpeg2/mpeg2ts_psi.cc:45: LOG_IF(WARNING, raw_psi_size > 0) On 2013/09/05 18:29:10, acolwell wrote: > DVLOG here and below Ditto. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_psi.c... media/mpeg2/mpeg2ts_psi.cc:65: if (size == 0) { On 2013/09/05 18:29:10, acolwell wrote: > nit: remove{} here and all 1 line ifs below Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_psi.c... media/mpeg2/mpeg2ts_psi.cc:71: int raw_psi_size = 0; On 2013/09/05 18:29:10, acolwell wrote: > ditto Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_psi.c... media/mpeg2/mpeg2ts_psi.cc:100: crc.Update(raw_psi[k], 8); On 2013/09/05 18:29:10, acolwell wrote: > This appears to be the only call site for this method. Since the CRC is only > ever updated a byte at a time, change the signature of Update() to just take a > uint8 to reflect that this is the only way this method is being used right now. > If the required usage changes in the future, then we can always revert the > signature. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... File media/mpeg2/mpeg2ts_stream_parser.cc (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.cc:49: Mpeg2TsSectionParser* section_parser); On 2013/09/05 18:29:10, acolwell wrote: > nit: Use scoped_ptr<Mpeg2TsSectionParser> here so the comment is unnecessary. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.cc:64: bool IsEnabled(); On 2013/09/05 18:29:10, acolwell wrote: > nit: add const Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.cc:66: PidType pid_type() { return pid_type_; } On 2013/09/05 18:29:10, acolwell wrote: > nit: add const here and in any other similar accessors that I may have missed. Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.cc:105: bool parse_result = section_parser_->Parse( On 2013/09/05 18:29:10, acolwell wrote: > nit: just return here since nothing else uses parse_result Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.cc:135: //section_parser_->ResetState(); On 2013/09/05 18:29:10, acolwell wrote: > Remove if this really isn't needed. Done: implemented. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.cc:205: audio_buffer_queue_.clear(); On 2013/09/05 18:29:10, acolwell wrote: > nit: If we are DCHECKing that these queues are empty, then we shouldn't need to > clear() them here. Should now be clearer. Whether buffers are emitted or cleared depend on |is_initialized|. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.cc:233: const uint8* ts_buffer = NULL; On 2013/09/05 18:29:10, acolwell wrote: > nit: init not needed Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.cc:243: LOG(WARNING) << "Packet not aligned on a TS syncword:" On 2013/09/05 18:29:10, acolwell wrote: > DVLOG here and everywhere else Done. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.cc:494: audio_buffer_with_config.config = audio_config_; On 2013/09/05 18:29:10, acolwell wrote: > You shouldn't need to do this. Just emit the pending buffers when the new config > arrives. Then you don't have to make a copy of the config for every buffer or > keep track of which configs are associated with what buffers. That was my initial implementation. However, I switched to keeping a configuration for each buffer because you can have some special cases before initialization. You can't emit any buffer before initialization is done. However, nothing is preventing several video configuration changes before initialization is completed. https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... File media/mpeg2/mpeg2ts_stream_parser.h (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.h:8: #include <list> On 2013/09/05 18:29:10, acolwell wrote: > Add #include <map> Done. https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/es_parser.h File media/mpeg2/es_parser.h (right): https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/es_parser.h#n... media/mpeg2/es_parser.h:23: virtual void Parse(const uint8* buf, int size, On 2013/09/05 18:29:10, acolwell wrote: > Should return a value so that parse errors can be reported. Done. https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/es_parser_adt... File media/mpeg2/es_parser_adts.cc (right): https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:182: } On 2013/09/05 01:58:29, damienv1 wrote: > Brackets un-needed Done. https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.cc:256: channel_configuration_ = channel_configuration; On 2013/09/05 01:58:29, damienv1 wrote: > Instead of {sampling_frequency, channel_configuration}, should use a > |current_audio_decoder_config_| and use IsValid & Matches for updates. Done. https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/mpeg2ts_crc.cc File media/mpeg2/mpeg2ts_crc.cc (right): https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/mpeg2ts_crc.c... media/mpeg2/mpeg2ts_crc.cc:14: void Mpeg2TsCrc::Update(uint32 data, int nbits) { On 2013/09/05 01:58:29, damienv1 wrote: > DCHECK_LE(nbits, 32); Done. https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/mpeg2ts_pat.cc File media/mpeg2/mpeg2ts_pat.cc (right): https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/mpeg2ts_pat.c... media/mpeg2/mpeg2ts_pat.cc:97: } On 2013/09/05 01:58:29, damienv1 wrote: > Remove brackets. Done. https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/mpeg2ts_pat.c... media/mpeg2/mpeg2ts_pat.cc:103: } On 2013/09/05 01:58:29, damienv1 wrote: > Remove brackets. Done. https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/mpeg2ts_pat.h File media/mpeg2/mpeg2ts_pat.h (right): https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/mpeg2ts_pat.h... media/mpeg2/mpeg2ts_pat.h:33: // or is incremented by 1. On 2013/09/05 01:58:29, damienv1 wrote: > Remove TODO as it's now done. Done. https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/mpeg2ts_pes.cc File media/mpeg2/mpeg2ts_pes.cc (right): https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:93: previous_dts_(0) { DCHECK(es_parser); https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/mpeg2ts_pes.c... media/mpeg2/mpeg2ts_pes.cc:319: if (es_parser_) Remove condition. https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/mpeg2ts_pmt.cc File media/mpeg2/mpeg2ts_pmt.cc (right): https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/mpeg2ts_pmt.c... media/mpeg2/mpeg2ts_pmt.cc:148: it != pid_map.end(); ++it) { On 2013/09/05 01:58:29, damienv1 wrote: > Might fit in one line. No. https://codereview.chromium.org/23566013/diff/14001/media/mpeg2/mpeg2ts_pmt.c... media/mpeg2/mpeg2ts_pmt.cc:150: } On 2013/09/05 01:58:29, damienv1 wrote: > Remove brackets. Done.
https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_pmt.cc File media/mpeg2/mpeg2ts_pmt.cc (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_pmt.c... media/mpeg2/mpeg2ts_pmt.cc:29: DCHECK_GE(bit_reader->bits_available(), 8 * 8); Remove DCHECK. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_psi.cc File media/mpeg2/mpeg2ts_psi.cc (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_psi.c... media/mpeg2/mpeg2ts_psi.cc:27: bool Mpeg2TsPsiParser::ParseInternal(bool payload_unit_start_indicator, Could remove ParseInternal function (since Parse is now just exactly the same as ParseInternal). https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_strea... File media/mpeg2/mpeg2ts_stream_parser.cc (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.cc:26: enum StreamType { Move to media::mpeg2ts namespace. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/ts_packet.cc File media/mpeg2/ts_packet.cc (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/ts_packet.cc#... media/mpeg2/ts_packet.cc:16: const uint8 kTsHeaderSyncword = 0x47; static
https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/es_parser_adts.h File media/mpeg2/es_parser_adts.h (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.h:10: #include <vector> <vector> not needed anymore. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/es_parser_h264.h File media/mpeg2/es_parser_h264.h (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.h:10: #include <vector> <vector> not needed anymore. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.h:108: // Last video decoder config. Should use a VideoDecoderConfig instead of all these parameters. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_pat.h File media/mpeg2/mpeg2ts_pat.h (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_pat.h... media/mpeg2/mpeg2ts_pat.h:8: #include <vector> Not needed anymore. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_pes.h File media/mpeg2/mpeg2ts_pes.h (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_pes.h... media/mpeg2/mpeg2ts_pes.h:8: #include <vector> Not needed anymore. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_pes.h... media/mpeg2/mpeg2ts_pes.h:11: #include "base/callback.h" callback.h not needed. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_pmt.h File media/mpeg2/mpeg2ts_pmt.h (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_pmt.h... media/mpeg2/mpeg2ts_pmt.h:8: #include <vector> Not needed anymore. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_psi.h File media/mpeg2/mpeg2ts_psi.h (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_psi.h... media/mpeg2/mpeg2ts_psi.h:8: #include <vector> Not needed anymore. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_psi.h... media/mpeg2/mpeg2ts_psi.h:10: #include "base/callback.h" callback.h not needed.
https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/es_parser_h26... File media/mpeg2/es_parser_h264.cc (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:57: RCHECK(ReadBits(1, &one_bit)); Would be more efficient to read |zero_count| bits at a time. The loop might not even be needed... https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:125: DiscardEs(raw_es_size - 4); Equivalent, but strictly should be |nal_es_pos_| and not raw_es_size - 4 https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:295: nal_desc_list_.front().position < nbytes) { Remove brackets. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:334: LOG(WARNING) << "NalParser: incomplete NAL"; DVLOG(1) https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:399: // Bitstream error: pic_order_cnt_type shall be in the range of 0 to 2. Move comment above and remove brackets. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_crc.cc File media/mpeg2/mpeg2ts_crc.cc (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_crc.c... media/mpeg2/mpeg2ts_crc.cc:7: #include "base/logging.h" Not needed anymore, to be removed.
https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/es_parser_adts.h File media/mpeg2/es_parser_adts.h (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/es_parser_adt... media/mpeg2/es_parser_adts.h:10: #include <vector> On 2013/09/10 04:57:01, damienv1 wrote: > <vector> not needed anymore. Done. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/es_parser_h26... File media/mpeg2/es_parser_h264.cc (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:57: RCHECK(ReadBits(1, &one_bit)); On 2013/09/10 15:28:25, damienv1 wrote: > Would be more efficient to read |zero_count| bits at a time. The loop might not > even be needed... Done. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:125: DiscardEs(raw_es_size - 4); On 2013/09/10 15:28:25, damienv1 wrote: > Equivalent, but strictly should be |nal_es_pos_| and not raw_es_size - 4 Done. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:295: nal_desc_list_.front().position < nbytes) { On 2013/09/10 15:28:25, damienv1 wrote: > Remove brackets. Done. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:334: LOG(WARNING) << "NalParser: incomplete NAL"; On 2013/09/10 15:28:25, damienv1 wrote: > DVLOG(1) Done. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.cc:399: // Bitstream error: pic_order_cnt_type shall be in the range of 0 to 2. On 2013/09/10 15:28:25, damienv1 wrote: > Move comment above and remove brackets. Done. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/es_parser_h264.h File media/mpeg2/es_parser_h264.h (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/es_parser_h26... media/mpeg2/es_parser_h264.h:10: #include <vector> On 2013/09/10 04:57:01, damienv1 wrote: > <vector> not needed anymore. Done. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_crc.cc File media/mpeg2/mpeg2ts_crc.cc (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_crc.c... media/mpeg2/mpeg2ts_crc.cc:7: #include "base/logging.h" On 2013/09/10 15:28:25, damienv1 wrote: > Not needed anymore, to be removed. Done. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_pat.h File media/mpeg2/mpeg2ts_pat.h (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_pat.h... media/mpeg2/mpeg2ts_pat.h:8: #include <vector> On 2013/09/10 04:57:01, damienv1 wrote: > Not needed anymore. Done. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_pes.h File media/mpeg2/mpeg2ts_pes.h (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_pes.h... media/mpeg2/mpeg2ts_pes.h:8: #include <vector> On 2013/09/10 04:57:01, damienv1 wrote: > Not needed anymore. Done. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_pes.h... media/mpeg2/mpeg2ts_pes.h:11: #include "base/callback.h" On 2013/09/10 04:57:01, damienv1 wrote: > callback.h not needed. Done. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_pmt.cc File media/mpeg2/mpeg2ts_pmt.cc (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_pmt.c... media/mpeg2/mpeg2ts_pmt.cc:29: DCHECK_GE(bit_reader->bits_available(), 8 * 8); On 2013/09/10 04:10:02, damienv1 wrote: > Remove DCHECK. Done. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_pmt.h File media/mpeg2/mpeg2ts_pmt.h (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_pmt.h... media/mpeg2/mpeg2ts_pmt.h:8: #include <vector> On 2013/09/10 04:57:01, damienv1 wrote: > Not needed anymore. Done. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_psi.cc File media/mpeg2/mpeg2ts_psi.cc (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_psi.c... media/mpeg2/mpeg2ts_psi.cc:27: bool Mpeg2TsPsiParser::ParseInternal(bool payload_unit_start_indicator, On 2013/09/10 04:10:02, damienv1 wrote: > Could remove ParseInternal function (since Parse is now just exactly the same as > ParseInternal). Done. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_psi.h File media/mpeg2/mpeg2ts_psi.h (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_psi.h... media/mpeg2/mpeg2ts_psi.h:8: #include <vector> On 2013/09/10 04:57:01, damienv1 wrote: > Not needed anymore. Done. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_psi.h... media/mpeg2/mpeg2ts_psi.h:10: #include "base/callback.h" On 2013/09/10 04:57:01, damienv1 wrote: > callback.h not needed. Done. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_strea... File media/mpeg2/mpeg2ts_stream_parser.cc (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.cc:26: enum StreamType { On 2013/09/10 04:10:02, damienv1 wrote: > Move to media::mpeg2ts namespace. Done. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/mpeg2ts_strea... media/mpeg2/mpeg2ts_stream_parser.cc:534: EmitAudioBuffers(); Emitting audio and video buffers in 2 different passes might be an issue: the segment start time is derived the first time |new_buffers_cb_| is invoked. The segment start time is thus wrong if the 1st video timestamp is earlier than the 1st audio timestamp. https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/ts_packet.cc File media/mpeg2/ts_packet.cc (right): https://codereview.chromium.org/23566013/diff/74001/media/mpeg2/ts_packet.cc#... media/mpeg2/ts_packet.cc:16: const uint8 kTsHeaderSyncword = 0x47; On 2013/09/10 04:10:02, damienv1 wrote: > static Done.
https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pat.cc File media/mpeg2/mpeg2ts_pat.cc (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pat.c... media/mpeg2/mpeg2ts_pat.cc:32: DCHECK(bit_reader->ReadBits(1, §ion_syntax_indicator)); On 2013/09/09 23:29:45, damienv1 wrote: > On 2013/09/05 18:29:10, acolwell wrote: > > These should be RCHECK otherwise these won't run in release builds. > > Running this is safe in release builds: I just check before that I had at least > 8*8 = 64 bits available. Pros: check is done only once instead of using RCHECK > for every ReadBits call. But none of the code in a DCHECK runs in a release build. All of the variables will have random values because ReadBits() won't actually be called. It is not safe to assume that the expression in a DCHECK will actually be evaluated in prelease builds. Please just use RCHECK in all the places I mentioned it should be used instead of DCHECK. We can optimize this code later if the checks become a significant perf problem.
https://codereview.chromium.org/23566013/diff/91001/media/filters/stream_pars... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/23566013/diff/91001/media/filters/stream_pars... media/filters/stream_parser_factory.cc:20: #include "media/mp2t/mp2t_stream_parser.h" Order (should be before mp4). https://codereview.chromium.org/23566013/diff/91001/media/mp2t/es_parser.h File media/mp2t/es_parser.h (right): https://codereview.chromium.org/23566013/diff/91001/media/mp2t/es_parser.h#ne... media/mp2t/es_parser.h:9: #include "base/callback.h" #include "base/memory/ref_counted.h" https://codereview.chromium.org/23566013/diff/91001/media/mp2t/es_parser_adts.h File media/mp2t/es_parser_adts.h (right): https://codereview.chromium.org/23566013/diff/91001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.h:11: #include "base/basictypes.h" Not needed here. https://codereview.chromium.org/23566013/diff/91001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.h:14: #include "base/memory/ref_counted.h" Not needed here. https://codereview.chromium.org/23566013/diff/91001/media/mp2t/es_parser_h264.cc File media/mp2t/es_parser_h264.cc (right): https://codereview.chromium.org/23566013/diff/91001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:56: } TODO: special case if |zero_count| = 0 ? https://codereview.chromium.org/23566013/diff/91001/media/mp2t/es_parser_h264.h File media/mp2t/es_parser_h264.h (right): https://codereview.chromium.org/23566013/diff/91001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.h:14: #include "base/memory/ref_counted.h" Not needed here.
https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pat.cc File media/mpeg2/mpeg2ts_pat.cc (right): https://codereview.chromium.org/23566013/diff/25001/media/mpeg2/mpeg2ts_pat.c... media/mpeg2/mpeg2ts_pat.cc:32: DCHECK(bit_reader->ReadBits(1, §ion_syntax_indicator)); On 2013/09/12 19:59:50, acolwell wrote: > On 2013/09/09 23:29:45, damienv1 wrote: > > On 2013/09/05 18:29:10, acolwell wrote: > > > These should be RCHECK otherwise these won't run in release builds. > > > > Running this is safe in release builds: I just check before that I had at > least > > 8*8 = 64 bits available. Pros: check is done only once instead of using RCHECK > > for every ReadBits call. > But none of the code in a DCHECK runs in a release build. All of the variables > will have random values because ReadBits() won't actually be called. It is not > safe to assume that the expression in a DCHECK will actually be evaluated in > prelease builds. > > Please just use RCHECK in all the places I mentioned it should be used instead > of DCHECK. We can optimize this code later if the checks become a significant > perf problem. OK, my mistake. If we had to use DCHECK, it would be: bool status = ReadBits(...); DCHECK(status); => Will replace by RCHECK in all the places. https://codereview.chromium.org/23566013/diff/91001/media/filters/stream_pars... File media/filters/stream_parser_factory.cc (right): https://codereview.chromium.org/23566013/diff/91001/media/filters/stream_pars... media/filters/stream_parser_factory.cc:20: #include "media/mp2t/mp2t_stream_parser.h" On 2013/09/12 20:43:02, damienv1 wrote: > Order (should be before mp4). Done. https://codereview.chromium.org/23566013/diff/91001/media/mp2t/es_parser.h File media/mp2t/es_parser.h (right): https://codereview.chromium.org/23566013/diff/91001/media/mp2t/es_parser.h#ne... media/mp2t/es_parser.h:9: #include "base/callback.h" On 2013/09/12 20:43:02, damienv1 wrote: > #include "base/memory/ref_counted.h" Done. https://codereview.chromium.org/23566013/diff/91001/media/mp2t/es_parser_adts.h File media/mp2t/es_parser_adts.h (right): https://codereview.chromium.org/23566013/diff/91001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.h:11: #include "base/basictypes.h" On 2013/09/12 20:43:02, damienv1 wrote: > Not needed here. Done. https://codereview.chromium.org/23566013/diff/91001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.h:14: #include "base/memory/ref_counted.h" On 2013/09/12 20:43:02, damienv1 wrote: > Not needed here. Done. https://codereview.chromium.org/23566013/diff/91001/media/mp2t/es_parser_h264.h File media/mp2t/es_parser_h264.h (right): https://codereview.chromium.org/23566013/diff/91001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.h:14: #include "base/memory/ref_counted.h" On 2013/09/12 20:43:02, damienv1 wrote: > Not needed here. Done.
https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_packet.cc File media/mp2t/ts_packet.cc (right): https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:49: LOG(WARNING) << "Buffer does not hold one full TS packet:" DVLOG https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:56: LOG(WARNING) << "Not on a TS syncword:" DVLOG https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:130: // in the adaptation field of a transport stream packet. Move comment before if and remove brackets. https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:223: } RCHECK(stuffing_byte == 0xff); https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_packet.h File media/mp2t/ts_packet.h (right): https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_packet.h#ne... media/mp2t/ts_packet.h:50: int GetPayloadOffset() const; GetPayloadOffset is never used -> to be removed. https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_section_pat.cc File media/mp2t/ts_section_pat.cc (right): https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_section_pat... media/mp2t/ts_section_pat.cc:80: LOG(WARNING) << "Not supported: received a PAT not applicable yet"; DVLOG https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_section_pes.cc File media/mp2t/ts_section_pes.cc (right): https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_section_pes... media/mp2t/ts_section_pes.cc:242: if (pts_dts_flags == 0x2) { RCHECK(pes_header_data_length >= 5); https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_section_pes... media/mp2t/ts_section_pes.cc:250: if (pts_dts_flags == 0x3) { RCHECK(pes_header_data_length >= 10); https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_section_pes... media/mp2t/ts_section_pes.cc:288: } Condition not needed if we have the previous RCHECKs. https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_section_pes... media/mp2t/ts_section_pes.cc:290: return false; Condition not needed since a RCHECK is done in the loop. https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_section_pes... media/mp2t/ts_section_pes.cc:316: return status; return es_parser_->Parse(...); https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_section_pmt.cc File media/mp2t/ts_section_pmt.cc (right): https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_section_pmt... media/mp2t/ts_section_pmt.cc:39: RCHECK(bit_reader->ReadBits(12, §ion_length)); RCHECK(section_length >= 5); https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_section_pmt... media/mp2t/ts_section_pmt.cc:67: } Not really needed anymore since a RCHECK is done each time ReadBits is called.
https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_packet.cc File media/mp2t/ts_packet.cc (right): https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:49: LOG(WARNING) << "Buffer does not hold one full TS packet:" On 2013/09/13 01:23:01, damienv1 wrote: > DVLOG Done. https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:56: LOG(WARNING) << "Not on a TS syncword:" On 2013/09/13 01:23:01, damienv1 wrote: > DVLOG Done. https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:130: // in the adaptation field of a transport stream packet. On 2013/09/13 01:23:01, damienv1 wrote: > Move comment before if and remove brackets. Done. https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:223: } On 2013/09/13 01:23:01, damienv1 wrote: > RCHECK(stuffing_byte == 0xff); Done. https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_packet.h File media/mp2t/ts_packet.h (right): https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_packet.h#ne... media/mp2t/ts_packet.h:50: int GetPayloadOffset() const; On 2013/09/13 01:23:01, damienv1 wrote: > GetPayloadOffset is never used -> to be removed. Done. https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_section_pat.cc File media/mp2t/ts_section_pat.cc (right): https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_section_pat... media/mp2t/ts_section_pat.cc:80: LOG(WARNING) << "Not supported: received a PAT not applicable yet"; On 2013/09/13 01:23:01, damienv1 wrote: > DVLOG Done. https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_section_pes.cc File media/mp2t/ts_section_pes.cc (right): https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_section_pes... media/mp2t/ts_section_pes.cc:242: if (pts_dts_flags == 0x2) { On 2013/09/13 01:23:01, damienv1 wrote: > RCHECK(pes_header_data_length >= 5); Done. https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_section_pes... media/mp2t/ts_section_pes.cc:250: if (pts_dts_flags == 0x3) { On 2013/09/13 01:23:01, damienv1 wrote: > RCHECK(pes_header_data_length >= 10); Done. https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_section_pes... media/mp2t/ts_section_pes.cc:288: } On 2013/09/13 01:23:01, damienv1 wrote: > Condition not needed if we have the previous RCHECKs. Done. https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_section_pes... media/mp2t/ts_section_pes.cc:290: return false; On 2013/09/13 01:23:01, damienv1 wrote: > Condition not needed since a RCHECK is done in the loop. Done. https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_section_pes... media/mp2t/ts_section_pes.cc:316: return status; On 2013/09/13 01:23:01, damienv1 wrote: > return es_parser_->Parse(...); Done. https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_section_pmt.cc File media/mp2t/ts_section_pmt.cc (right): https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_section_pmt... media/mp2t/ts_section_pmt.cc:39: RCHECK(bit_reader->ReadBits(12, §ion_length)); On 2013/09/13 01:23:01, damienv1 wrote: > RCHECK(section_length >= 5); Done. https://codereview.chromium.org/23566013/diff/97001/media/mp2t/ts_section_pmt... media/mp2t/ts_section_pmt.cc:67: } On 2013/09/13 01:23:01, damienv1 wrote: > Not really needed anymore since a RCHECK is done each time ReadBits is called. Done.
looking good. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts.cc File media/mp2t/es_parser_adts.cc (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:12: #include "media/base/audio_decoder_config.h" nit: Shouldn't need this now since it is in the header. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:81: int max_offset = raw_es_size - kAdtsHeaderMinSize; nit: DCHECK_GE(pos, 0); DCHECK_LT(pos, raw_es_size); https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:83: max_offset = 0; nit: I think it might be clearer if you just set *new_pos and early return here. Also I think there might be a bug in the existing code if pos > 0 && max_offset < 0. *new_pos should always be == to pos in this case right? https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:102: if ((cur_buf[frame_size] != 0xff) || nit: Replace this and the check above w/ isSyncWord() helper call just to make the code a little clearer? https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:133: const uint8* raw_es = NULL; nit: var init isn't needed here. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:167: DCHECK(audio_timestamp_helper_); nit: No need for DCHECK here since the code will crash in 4 or 7 lines later. Usually in these types of situations we just let the code crash instead of putting a DCHECK. We tend to use NULL DCHECKs only for method param verification. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:170: audio_timestamp_helper_->SetBaseTimestamp(pts_list_.front().second); Is this here to compensate for gaps in the audio? You shouldn't need to do this otherwise since the accumulation of samples should be more accurate and ensures that gaps/overlaps aren't created. I think you should only reset the base if the timestamp is more than a few samples different. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:207: pts_list_.clear(); This doesn't seem consistent w/ the description in es_parser.h. I'd expect this function to be a noop, but it looks like it is effecting the parser state. Am I misunderstanding something here? https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:230: I think you need the following since I don't believe we support inband-channel layout. // TODO: Add support for inband channel configurations. if (channel_configuration == 0) return false; https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:268: it->first -= nbytes; How big is this list likely to be? It might be more efficient to keep a bytes_consumed_ count and then just add the offset when you insert the elements & subract it off when you are popping them off. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts.h File media/mp2t/es_parser_adts.h (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.h:20: class AudioDecoderConfig; nit: Not needed since you are including the .h https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264.cc File media/mp2t/es_parser_h264.cc (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:20: static const int kTableSarWidth[14] = { nit: spec reference here and below. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:88: // to an h264 access unit, although the HLS recommandation is to use one PES nit: s/recommandation/recommendation here and everywhere below https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:108: std::pair<int, TimingDesc>(raw_es_size, timing_desc)); Should we check to see if dts & pts actually match an entry that is already in the list? It seems like extra entries w/ the same info wouldn't really be helpful. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:112: es_byte_queue_.Peek(&raw_es, &raw_es_size); nit: You can avoid the extra Peek above and in the adts code by just subtracting size from raw_es_size after this Peek. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:115: FindNals(); WDYT about change this to something like "nal_es_pos_ = FindNals(nal_es_pos_);" so that it is more explicit that nal_es_pos_ is getting modified. I'm only suggesting it because the DiscardEs(nal_eo_pos_) is a little surprising below since it isn't clear from this context that it was ever modified. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:126: std::list<NalDescList::const_iterator>::iterator it0 = nit: how about using cur_frame, nxt_frame here? https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:130: LOG_IF(WARNING, (*it0)->position != 0) nit: convert to DVLOG https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:137: Reset(); nit: You shouldn't need to Reset() here. If a format error occurs, you should be able to assume that you won't get called again. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:143: int last_position = access_unit_list.back()->position; nit: Just inline below since you don't use the value anywhere else and it is pretty cleay that this is the last position. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:156: if (!access_unit_list.empty()) { nit: Remove this check if the list is always supposed to be non-empty like the DCHECK above implies. Otherwise, remove the DCHECK & reverse the condition & early return. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:159: const uint8* raw_es = NULL; nit: init not needed https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:209: DVLOG(LOG_LEVEL_ES) << "nal: offset=" << nal_desc.position nit: Add an extra line before this line to visually separate this. When I originally was scanning it, I thought that only invalid NALs were being added to nal_desc_list_. :) https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:256: for (NalDescList::const_iterator it = cur_frame; it != nxt_frame; ++it) { It feels like this loop could be done as part of the FindNals() loop and then you wouldn't have to keep all the NAL boundary state. Am I missing something here? https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:266: bool nal_status = NalParser(&raw_es[cur_nal_position], nal_size); nit: s/NalParser/ParseNAL/ & inline below. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:296: it->position -= nbytes; This list could be potentially big right? Perhaps keeping a bytes_consumed_ counter like I mentioned for the ADTS code would remove the need for this loop. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:306: timing_it->first -= nbytes; ditto https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:315: !(buf[2] == 1 || (size >= 4 && buf[2] == 0 && buf[3] == 1))) { It would be nice if FindNals() and this method could share code for this aspect of NAL header parsing. Perhaps something along the lines of int ParseNalHeader(const uint8* buf, int size, uint8* nal_header) w/ the return value semantics used in other parsers <0 : error 0 : need more data >0 : number of bytes parsed. I think that would work for both cases. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:344: // before parsing the NAL content. content/common/gpu/media/h264_bit_reader.h can probably help with this, but it would need to be moved to media/base first. Best to do that in a separate CL. :) https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:347: if (nal_unit_type == kNalUnitTypeSPS) { nit: use switch(nal_unit_type) for this and the code below instead. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:394: if (pic_order_cnt_type > 2) nit: RCHECK(pic_order_cnt_type <= 2); https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:400: NOTIMPLEMENTED(); nit: Please add a TODO or comment about why this isn't implemented. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... File media/mp2t/mp2t_stream_parser.cc (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:48: bool PushTsPacket(TsPacket* ts_packet); nit: const TsPacket& since NULL shouldn't be allowed and that packet isn't getting modified. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:141: class Mp2tStreamParser::AudioBufferWithConfig { I am still not a fan of this. The decoder configs are not intended to be copied for every buffer. Copies and comparison should not be considered cheap. That is why there are Get/SetConfigId() methods on StreamParserBuffer. Please convert the code to use this mechanism instead. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:287: bool status = it->second->PushTsPacket(ts_packet.get()); nit: inline below. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:316: LOG_IF(WARNING, pmt_pid != pid) << "More than one program is defined"; nit: convert to DVLOG https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:334: int pes_pid, nit: fix indent https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:389: std::map<int, PidState*>::iterator lowest_audio_pid = pids_.end(); nit: use PidMap here and elsewhere below. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:399: ((lowest_video_pid == pids_.end() || pid < lowest_video_pid->first))) nit: It looks like you have an extra set of () here and above. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... File media/mp2t/mp2t_stream_parser.h (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.h:21: class AudioDecoderConfig; nit: Not needed since you are including the .h. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.h:23: class VideoDecoderConfig; Ditto https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.h:69: // changing. nit: s/changing/changed/? https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... File media/mp2t/mp2t_stream_parser_unittest.cc (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser_unittest.cc:68: configs_received_ = true; nit: You don't appear to check the value of this anywhere. Either remove the variable or verify that it is set somewhere. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser_unittest.cc:94: dts < video_min_dts_) nit: Add {} or move second condition to the line above. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser_unittest.cc:96: if (video_max_dts_ == kNoTimestamp() || ditto https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser_unittest.cc:99: } Shouldn't video_min_dts_ == video_buffers.front()->GetDecodeTimestamp() and video_max_dts_ == video_buffers.back()->GetDecodeTimestamp() ? It seems like you should only need the loop to verify monotonically increasing timestamps. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.cc File media/mp2t/ts_packet.cc (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:37: if (is_header) { nit: remove {} https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:42: LOG_IF(WARNING, k != 0) << "SYNC: nbytes_skipped=" << k; nit: convert to DVLOG https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:49: LOG(WARNING) << "Buffer does not hold one full TS packet:" nit: convert to DVLOG https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:70: ts_packet->payload_ = buf + (kPacketSize - ts_packet->payload_size_); nit: These seems out of place. How about just passing buf & size to ParseHeader() so you don't need to do this? https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:124: // return false; nit: We should not encourage this behavior. I think we should return false here. Unless the Apple encoder does this, I think we should reject these streams. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:155: adaptation_field_length -= 1; nit: You shouldn't need to do all these length updates. Just capture bits_available() at the top and then check it again at the bottom. I think that would be less error prone. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:158: RCHECK(adaptation_field_length >= 6); These types of checks shouldn't be needed since you are using RCHECKs below. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:196: RCHECK(bit_reader->ReadBits(8, &private_data_byte)); nit: You should be able to use RCHECK(bit_reader->SkipBits(8 * transport_private_data_length)) instead of a loop here. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:210: RCHECK(bit_reader->ReadBits(8, &dummy_byte)); ditto re: SkipBits() https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.h File media/mp2t/ts_packet.h (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.h#ne... media/mp2t/ts_packet.h:49: const uint8* GetPayload() const; nit: s/GetPayload()/payload/ and inline since this is a simple accessor as well. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.h#ne... media/mp2t/ts_packet.h:50: int GetPayloadSize() const; s/GetPayloadSize/payload_size and inline https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pat.cc File media/mp2t/ts_section_pat.cc (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pat... media/mp2t/ts_section_pat.cc:57: RCHECK(section_length <= 1021); s/1021/1016 to compensate for the -= 5 above? https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pat... media/mp2t/ts_section_pat.cc:61: RCHECK((section_length & 0x3) == 0); nit: s/& 0x3/% 4/ just to make it easier to see that this matches what the comment says? https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pat... media/mp2t/ts_section_pat.cc:89: return false; nit: A DVLOG saying multiple programs were detected would probably be a good idea. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pat.h File media/mp2t/ts_section_pat.h (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pat... media/mp2t/ts_section_pat.h:20: explicit TsSectionPat(RegisterPmtCb register_pmt_cb); nit: const& https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pes.cc File media/mp2t/ts_section_pes.cc (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pes... media/mp2t/ts_section_pes.cc:23: int64 time, int nbits) { nit: Since nbits is always 33, there is no need to pass it in. Just put the constant inside this method so it doesn't seem like this method has more general use than it actually does. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pes... media/mp2t/ts_section_pes.cc:90: DCHECK(es_parser_ != NULL); nit: s/!= NULL// https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pes... media/mp2t/ts_section_pes.cc:250: pes_packet_length -= 5; nit: You shouldn't need to do these length computations. Using bits_available() should allow you to derive this down at the bottom. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pes... media/mp2t/ts_section_pes.cc:254: RCHECK(pes_header_data_length >= 10); nit: You don't need this since you are using RCHECK below. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pes... media/mp2t/ts_section_pes.cc:289: for (int k = 0; k < pes_header_data_length; k++) { Use SkipBits(8*pes_header_data_length) since you don't care about the actual values here. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pmt.cc File media/mp2t/ts_section_pmt.cc (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pmt... media/mp2t/ts_section_pmt.cc:40: RCHECK(section_length >= 5); nit: not needed since you are using RCHECK here. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pmt... media/mp2t/ts_section_pmt.cc:56: RCHECK(section_length <= 1021); Does this need to be adjusted because of the -=5 above? https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pmt... media/mp2t/ts_section_pmt.cc:65: RCHECK(section_length >= 4); nit: not needed https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pmt... media/mp2t/ts_section_pmt.cc:73: section_length -= 4; nit: You shouldn't need these types of updates. You should be able to derive this with bits_available() at the end. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pmt... media/mp2t/ts_section_pmt.cc:81: RCHECK(bit_reader->ReadBits(8, &dummy)); nit: Use SkipBits https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pmt... media/mp2t/ts_section_pmt.cc:111: RCHECK(bit_reader->ReadBits(8, &dummy)); nit: SkipBits instead of loop https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_psi.cc File media/mp2t/ts_section_psi.cc (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_psi... media/mp2t/ts_section_psi.cc:121: for (int k = 0; k < psi_length; k++) nit: Since this CRC class doesn't appear to be ever used anywhere else or updated outside of this loop, perhaps just making this a helper function would be better. bool IsPsiCRCValid(const uint8* buf, int size)?
Thanks for the comments. Main changes: - some cleanup based on the comments - re-architectured the H264 ES parser However: I still need to find a clean solution for the AudioDecoderConfig/VideoDecoderConfig in the Mp2tStreamParser. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts.cc File media/mp2t/es_parser_adts.cc (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:12: #include "media/base/audio_decoder_config.h" On 2013/09/16 06:19:30, acolwell wrote: > nit: Shouldn't need this now since it is in the header. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:81: int max_offset = raw_es_size - kAdtsHeaderMinSize; On 2013/09/16 06:19:30, acolwell wrote: > nit: > DCHECK_GE(pos, 0); > DCHECK_LT(pos, raw_es_size); Done. Except that this should be DCHECK_LE(pos, raw_es_size); As |pos| can point to the 1st byte that was not processed yet. So if the last byte of the buffer was processed, pos is pointing to |raw_es_size|. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:83: max_offset = 0; On 2013/09/16 06:19:30, acolwell wrote: > nit: I think it might be clearer if you just set *new_pos and early return here. > > Also I think there might be a bug in the existing code if pos > 0 && max_offset > < 0. *new_pos should always be == to pos in this case right? Yes, the new position should remain unchanged if max_offset < 0. I updated the behavior. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:102: if ((cur_buf[frame_size] != 0xff) || On 2013/09/16 06:19:30, acolwell wrote: > nit: Replace this and the check above w/ isSyncWord() helper call just to make > the code a little clearer? Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:133: const uint8* raw_es = NULL; On 2013/09/16 06:19:30, acolwell wrote: > nit: var init isn't needed here. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:167: DCHECK(audio_timestamp_helper_); On 2013/09/16 06:19:30, acolwell wrote: > nit: No need for DCHECK here since the code will crash in 4 or 7 lines later. > Usually in these types of situations we just let the code crash instead of > putting a DCHECK. We tend to use NULL DCHECKs only for method param > verification. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:170: audio_timestamp_helper_->SetBaseTimestamp(pts_list_.front().second); On 2013/09/16 06:19:30, acolwell wrote: > Is this here to compensate for gaps in the audio? You shouldn't need to do this > otherwise since the accumulation of samples should be more accurate and ensures > that gaps/overlaps aren't created. I think you should only reset the base if the > timestamp is more than a few samples different. 1) The timestamp helper has a resolution of 1 sample. The Mpeg2 TS timestamps have a resolution of 90kHz which in case of AAC is less than a sample (except for 96kHz where it's slightly more). 2) The Mpeg2 TS timestamp should be the one to use as there might be some GAP in the audio as well. I think the ADTS parser should not be the one to decide which PTS to use when there is one coming from the stream. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:207: pts_list_.clear(); On 2013/09/16 06:19:30, acolwell wrote: > This doesn't seem consistent w/ the description in es_parser.h. I'd expect this > function to be a noop, but it looks like it is effecting the parser state. Am I > misunderstanding something here? I made it a noop to match the description. It does not impact the behavior since a Flush is always followed by a Reset in practice. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:230: On 2013/09/16 06:19:30, acolwell wrote: > I think you need the following since I don't believe we support inband-channel > layout. > > // TODO: Add support for inband channel configurations. > if (channel_configuration == 0) > return false; Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:268: it->first -= nbytes; On 2013/09/16 06:19:30, acolwell wrote: > How big is this list likely to be? It might be more efficient to keep a > bytes_consumed_ count and then just add the offset when you insert the elements > & subract it off when you are popping them off. In the general case, the size of the list will be 0 at this time or possibly 1 (if there is no full ADTS frame in the last reassembled PES). "List size > 1" is very unlikely since this would mean: - an ADTS frame spreads over multiple reassembled PES - or no ADTS header was found over the last couple of reassembled PES At any time in this code, the PTS list size should be between 0 and 2 included. Any other value (> 2) is very unlikely (though not impossible). https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts.h File media/mp2t/es_parser_adts.h (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.h:20: class AudioDecoderConfig; On 2013/09/16 06:19:30, acolwell wrote: > nit: Not needed since you are including the .h Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264.cc File media/mp2t/es_parser_h264.cc (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:20: static const int kTableSarWidth[14] = { On 2013/09/16 06:19:30, acolwell wrote: > nit: spec reference here and below. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:88: // to an h264 access unit, although the HLS recommandation is to use one PES On 2013/09/16 06:19:30, acolwell wrote: > nit: s/recommandation/recommendation here and everywhere below Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:108: std::pair<int, TimingDesc>(raw_es_size, timing_desc)); On 2013/09/16 06:19:30, acolwell wrote: > Should we check to see if dts & pts actually match an entry that is already in > the list? It seems like extra entries w/ the same info wouldn't really be > helpful. I don't think this is useful since in a "correct" stream, the PTS/DTS will be correctly updated by the encoder (so you shouldn't have any duplicated entries). https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:112: es_byte_queue_.Peek(&raw_es, &raw_es_size); On 2013/09/16 06:19:30, acolwell wrote: > nit: You can avoid the extra Peek above and in the adts code by just subtracting > size from raw_es_size after this Peek. Comment not relevant anymore as the code has changed. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:115: FindNals(); On 2013/09/16 06:19:30, acolwell wrote: > WDYT about change this to something like "nal_es_pos_ = FindNals(nal_es_pos_);" > so that it is more explicit that nal_es_pos_ is getting modified. I'm only > suggesting it because the DiscardEs(nal_eo_pos_) is a little surprising below > since it isn't clear from this context that it was ever modified. FindNals has been renamed to ParseInternal and includes frame emission as well. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:126: std::list<NalDescList::const_iterator>::iterator it0 = On 2013/09/16 06:19:30, acolwell wrote: > nit: how about using cur_frame, nxt_frame here? Overall h264 code simplified: comment not relevant anymore. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:130: LOG_IF(WARNING, (*it0)->position != 0) On 2013/09/16 06:19:30, acolwell wrote: > nit: convert to DVLOG Comment not relevant anymore. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:137: Reset(); On 2013/09/16 06:19:30, acolwell wrote: > nit: You shouldn't need to Reset() here. If a format error occurs, you should be > able to assume that you won't get called again. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:143: int last_position = access_unit_list.back()->position; On 2013/09/16 06:19:30, acolwell wrote: > nit: Just inline below since you don't use the value anywhere else and it is > pretty cleay that this is the last position. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:156: if (!access_unit_list.empty()) { On 2013/09/16 06:19:30, acolwell wrote: > nit: Remove this check if the list is always supposed to be non-empty like the > DCHECK above implies. Otherwise, remove the DCHECK & reverse the condition & > early return. Comment not relevant anymore. That was my mistake, this should be a DCHECK_LE (and most of the time, list size will be one, since video access unit are emitting only when the next access unit is coming so there is always a pending one... except for special cases, for example, calling Flush when no AUD was ever found before). https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:159: const uint8* raw_es = NULL; On 2013/09/16 06:19:30, acolwell wrote: > nit: init not needed Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:209: DVLOG(LOG_LEVEL_ES) << "nal: offset=" << nal_desc.position On 2013/09/16 06:19:30, acolwell wrote: > nit: Add an extra line before this line to visually separate this. When I > originally was scanning it, I thought that only invalid NALs were being added to > nal_desc_list_. :) Comment not relevant anymore. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:256: for (NalDescList::const_iterator it = cur_frame; it != nxt_frame; ++it) { On 2013/09/16 06:19:30, acolwell wrote: > It feels like this loop could be done as part of the FindNals() loop and then > you wouldn't have to keep all the NAL boundary state. Am I missing something > here? Done. I simplified the H264 parser significantly. I now use a bitstream approach and don't do multiple pass: it parses and emits frames at the same time (FindNals & EmitFrame are now part of the same function: ParseInternal). https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:266: bool nal_status = NalParser(&raw_es[cur_nal_position], nal_size); On 2013/09/16 06:19:30, acolwell wrote: > nit: s/NalParser/ParseNAL/ & inline below. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:296: it->position -= nbytes; On 2013/09/16 06:19:30, acolwell wrote: > This list could be potentially big right? Perhaps keeping a bytes_consumed_ > counter like I mentioned for the ADTS code would remove the need for this loop. Comment not relevant anymore as in the re-designed code, I don't store NAL positions. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:306: timing_it->first -= nbytes; On 2013/09/16 06:19:30, acolwell wrote: > ditto In the usual case, the list of timestamp will be very small because usually one access unit corresponds to one reassembled PES. Most likely, the list size will be smaller or equal to 2 at any time (only special cases can trigger a list size greater than 2). https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:344: // before parsing the NAL content. On 2013/09/16 06:19:30, acolwell wrote: > content/common/gpu/media/h264_bit_reader.h can probably help with this, but it > would need to be moved to media/base first. Best to do that in a separate CL. :) Added a TODO to unify H264 bit readers. But I agree that there should be only one. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:347: if (nal_unit_type == kNalUnitTypeSPS) { On 2013/09/16 06:19:30, acolwell wrote: > nit: use switch(nal_unit_type) for this and the code below instead. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:394: if (pic_order_cnt_type > 2) On 2013/09/16 06:19:30, acolwell wrote: > nit: RCHECK(pic_order_cnt_type <= 2); Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:400: NOTIMPLEMENTED(); On 2013/09/16 06:19:30, acolwell wrote: > nit: Please add a TODO or comment about why this isn't implemented. Now it's implemented. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... File media/mp2t/mp2t_stream_parser.cc (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:48: bool PushTsPacket(TsPacket* ts_packet); On 2013/09/16 06:19:30, acolwell wrote: > nit: const TsPacket& since NULL shouldn't be allowed and that packet isn't > getting modified. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:141: class Mp2tStreamParser::AudioBufferWithConfig { On 2013/09/16 06:19:30, acolwell wrote: > I am still not a fan of this. The decoder configs are not intended to be copied > for every buffer. Copies and comparison should not be considered cheap. That is > why there are Get/SetConfigId() methods on StreamParserBuffer. Please convert > the code to use this mechanism instead. I need to think a bit more about it. Will do it in another patch set. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:287: bool status = it->second->PushTsPacket(ts_packet.get()); On 2013/09/16 06:19:30, acolwell wrote: > nit: inline below. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:316: LOG_IF(WARNING, pmt_pid != pid) << "More than one program is defined"; On 2013/09/16 06:19:30, acolwell wrote: > nit: convert to DVLOG Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:334: int pes_pid, On 2013/09/16 06:19:30, acolwell wrote: > nit: fix indent Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:389: std::map<int, PidState*>::iterator lowest_audio_pid = pids_.end(); On 2013/09/16 06:19:30, acolwell wrote: > nit: use PidMap here and elsewhere below. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:399: ((lowest_video_pid == pids_.end() || pid < lowest_video_pid->first))) On 2013/09/16 06:19:30, acolwell wrote: > nit: It looks like you have an extra set of () here and above. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... File media/mp2t/mp2t_stream_parser.h (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.h:21: class AudioDecoderConfig; On 2013/09/16 06:19:30, acolwell wrote: > nit: Not needed since you are including the .h. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.h:23: class VideoDecoderConfig; On 2013/09/16 06:19:30, acolwell wrote: > Ditto Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.h:69: // changing. On 2013/09/16 06:19:30, acolwell wrote: > nit: s/changing/changed/? Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... File media/mp2t/mp2t_stream_parser_unittest.cc (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser_unittest.cc:68: configs_received_ = true; On 2013/09/16 06:19:30, acolwell wrote: > nit: You don't appear to check the value of this anywhere. Either remove the > variable or verify that it is set somewhere. Removed. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser_unittest.cc:94: dts < video_min_dts_) On 2013/09/16 06:19:30, acolwell wrote: > nit: Add {} or move second condition to the line above. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser_unittest.cc:96: if (video_max_dts_ == kNoTimestamp() || On 2013/09/16 06:19:30, acolwell wrote: > ditto Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser_unittest.cc:99: } On 2013/09/16 06:19:30, acolwell wrote: > Shouldn't video_min_dts_ == video_buffers.front()->GetDecodeTimestamp() > and video_max_dts_ == video_buffers.back()->GetDecodeTimestamp() ? It seems like > you should only need the loop to verify monotonically increasing timestamps. Correct. I modified the code. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.cc File media/mp2t/ts_packet.cc (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:37: if (is_header) { On 2013/09/16 06:19:30, acolwell wrote: > nit: remove {} Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:42: LOG_IF(WARNING, k != 0) << "SYNC: nbytes_skipped=" << k; On 2013/09/16 06:19:30, acolwell wrote: > nit: convert to DVLOG Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:49: LOG(WARNING) << "Buffer does not hold one full TS packet:" On 2013/09/16 06:19:30, acolwell wrote: > nit: convert to DVLOG Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:70: ts_packet->payload_ = buf + (kPacketSize - ts_packet->payload_size_); On 2013/09/16 06:19:30, acolwell wrote: > nit: These seems out of place. How about just passing buf & size to > ParseHeader() so you don't need to do this? Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:124: // return false; On 2013/09/16 06:19:30, acolwell wrote: > nit: We should not encourage this behavior. I think we should return false here. > Unless the Apple encoder does this, I think we should reject these streams. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:155: adaptation_field_length -= 1; On 2013/09/16 06:19:30, acolwell wrote: > nit: You shouldn't need to do all these length updates. Just capture > bits_available() at the top and then check it again at the bottom. I think that > would be less error prone. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:158: RCHECK(adaptation_field_length >= 6); On 2013/09/16 06:19:30, acolwell wrote: > These types of checks shouldn't be needed since you are using RCHECKs below. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:196: RCHECK(bit_reader->ReadBits(8, &private_data_byte)); On 2013/09/16 06:19:30, acolwell wrote: > nit: You should be able to use RCHECK(bit_reader->SkipBits(8 * > transport_private_data_length)) instead of a loop here. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.cc#n... media/mp2t/ts_packet.cc:210: RCHECK(bit_reader->ReadBits(8, &dummy_byte)); On 2013/09/16 06:19:30, acolwell wrote: > ditto re: SkipBits() Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.h File media/mp2t/ts_packet.h (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.h#ne... media/mp2t/ts_packet.h:49: const uint8* GetPayload() const; On 2013/09/16 06:19:30, acolwell wrote: > nit: s/GetPayload()/payload/ and inline since this is a simple accessor as well. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_packet.h#ne... media/mp2t/ts_packet.h:50: int GetPayloadSize() const; On 2013/09/16 06:19:30, acolwell wrote: > s/GetPayloadSize/payload_size and inline Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pat.cc File media/mp2t/ts_section_pat.cc (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pat... media/mp2t/ts_section_pat.cc:57: RCHECK(section_length <= 1021); On 2013/09/16 06:19:30, acolwell wrote: > s/1021/1016 to compensate for the -= 5 above? Good catch. I moved all the RCHECK together in a previous patch set and I just forgot section_length was modified in between. Thanks. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pat... media/mp2t/ts_section_pat.cc:61: RCHECK((section_length & 0x3) == 0); On 2013/09/16 06:19:30, acolwell wrote: > nit: s/& 0x3/% 4/ just to make it easier to see that this matches what the > comment says? Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pat... media/mp2t/ts_section_pat.cc:89: return false; On 2013/09/16 06:19:30, acolwell wrote: > nit: A DVLOG saying multiple programs were detected would probably be a good > idea. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pat.h File media/mp2t/ts_section_pat.h (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pat... media/mp2t/ts_section_pat.h:20: explicit TsSectionPat(RegisterPmtCb register_pmt_cb); On 2013/09/16 06:19:30, acolwell wrote: > nit: const& Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pes.cc File media/mp2t/ts_section_pes.cc (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pes... media/mp2t/ts_section_pes.cc:23: int64 time, int nbits) { On 2013/09/16 06:19:30, acolwell wrote: > nit: Since nbits is always 33, there is no need to pass it in. Just put the > constant inside this method so it doesn't seem like this method has more general > use than it actually does. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pes... media/mp2t/ts_section_pes.cc:90: DCHECK(es_parser_ != NULL); On 2013/09/16 06:19:30, acolwell wrote: > nit: s/!= NULL// Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pes... media/mp2t/ts_section_pes.cc:250: pes_packet_length -= 5; On 2013/09/16 06:19:30, acolwell wrote: > nit: You shouldn't need to do these length computations. Using bits_available() > should allow you to derive this down at the bottom. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pes... media/mp2t/ts_section_pes.cc:254: RCHECK(pes_header_data_length >= 10); On 2013/09/16 06:19:30, acolwell wrote: > nit: You don't need this since you are using RCHECK below. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pes... media/mp2t/ts_section_pes.cc:289: for (int k = 0; k < pes_header_data_length; k++) { On 2013/09/16 06:19:30, acolwell wrote: > Use SkipBits(8*pes_header_data_length) since you don't care about the actual > values here. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pmt.cc File media/mp2t/ts_section_pmt.cc (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pmt... media/mp2t/ts_section_pmt.cc:40: RCHECK(section_length >= 5); On 2013/09/16 06:19:30, acolwell wrote: > nit: not needed since you are using RCHECK here. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pmt... media/mp2t/ts_section_pmt.cc:56: RCHECK(section_length <= 1021); On 2013/09/16 06:19:30, acolwell wrote: > Does this need to be adjusted because of the -=5 above? My mistake: I was not cautious when I moved some RCHECKs. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pmt... media/mp2t/ts_section_pmt.cc:65: RCHECK(section_length >= 4); On 2013/09/16 06:19:30, acolwell wrote: > nit: not needed Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pmt... media/mp2t/ts_section_pmt.cc:73: section_length -= 4; On 2013/09/16 06:19:30, acolwell wrote: > nit: You shouldn't need these types of updates. You should be able to derive > this with bits_available() at the end. Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pmt... media/mp2t/ts_section_pmt.cc:81: RCHECK(bit_reader->ReadBits(8, &dummy)); On 2013/09/16 06:19:30, acolwell wrote: > nit: Use SkipBits Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_pmt... media/mp2t/ts_section_pmt.cc:111: RCHECK(bit_reader->ReadBits(8, &dummy)); On 2013/09/16 06:19:30, acolwell wrote: > nit: SkipBits instead of loop Done. https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_psi.cc File media/mp2t/ts_section_psi.cc (right): https://codereview.chromium.org/23566013/diff/66001/media/mp2t/ts_section_psi... media/mp2t/ts_section_psi.cc:121: for (int k = 0; k < psi_length; k++) On 2013/09/16 06:19:30, acolwell wrote: > nit: Since this CRC class doesn't appear to be ever used anywhere else or > updated outside of this loop, perhaps just making this a helper function would > be better. > > bool IsPsiCRCValid(const uint8* buf, int size)? Done.
On 2013/09/17 02:58:22, damienv1 wrote: > Thanks for the comments. > > Main changes: > - some cleanup based on the comments > - re-architectured the H264 ES parser > > However: > I still need to find a clean solution for the > AudioDecoderConfig/VideoDecoderConfig in the Mp2tStreamParser. I'll do another review pass later today. If you would like, we can defer the XXXDecoderConfig changes to a follow-up CL so we can at least get the bulk of this code landed. I really want it fixed ASAP, but it might be easier to review it as part of a smaller CL and the current code looks correct, just not ideal.
https://codereview.chromium.org/23566013/diff/115001/media/mp2t/es_parser_h26... File media/mp2t/es_parser_h264.cc (right): https://codereview.chromium.org/23566013/diff/115001/media/mp2t/es_parser_h26... media/mp2t/es_parser_h264.cc:38: if (buf[k] == 0x3 && zero_count >= 2) { zero_count = 0; continue; } https://codereview.chromium.org/23566013/diff/115001/media/mp2t/es_parser_h26... media/mp2t/es_parser_h264.cc:193: // Resume NAL segmentation where it was left. Comment should be updated. https://codereview.chromium.org/23566013/diff/115001/media/mp2t/es_parser_h26... media/mp2t/es_parser_h264.cc:194: for ( ; es_pos_ < raw_es_size - 4; es_pos_++) { Should be "- 5" to account for the size of the syncword and the NAL header byte. https://codereview.chromium.org/23566013/diff/115001/media/mp2t/es_parser_h26... media/mp2t/es_parser_h264.cc:210: RCHECK(NalParser(&raw_es[current_nal_pos_], nal_size)); The NAL parser should receive a pointer to the NAL header, i.e. remove the start code (so we don't have to check the start code again in the NAL parser). Use a |current_nal_syncword_length_| https://codereview.chromium.org/23566013/diff/115001/media/mp2t/es_parser_h26... media/mp2t/es_parser_h264.cc:314: } All this can be removed if we assume |buf| is pointing to the NAL header. https://codereview.chromium.org/23566013/diff/115001/media/mp2t/mp2t_stream_p... File media/mp2t/mp2t_stream_parser.cc (right): https://codereview.chromium.org/23566013/diff/115001/media/mp2t/mp2t_stream_p... media/mp2t/mp2t_stream_parser.cc:141: class Mp2tStreamParser::AudioBufferWithConfig { Use a struct instead. https://codereview.chromium.org/23566013/diff/115001/media/mp2t/mp2t_stream_p... media/mp2t/mp2t_stream_parser.cc:143: scoped_refptr<StreamParserBuffer> buffer; AudioDecoderConfig config; bool is_config_sent; StreamParser::BufferQueue buffer_queue; https://codereview.chromium.org/23566013/diff/115001/media/mp2t/mp2t_stream_p... media/mp2t/mp2t_stream_parser.cc:148: public: Ditto.
https://codereview.chromium.org/23566013/diff/115001/media/mp2t/es_parser_h26... File media/mp2t/es_parser_h264.cc (right): https://codereview.chromium.org/23566013/diff/115001/media/mp2t/es_parser_h26... media/mp2t/es_parser_h264.cc:38: if (buf[k] == 0x3 && zero_count >= 2) On 2013/09/17 16:11:52, damienv1 wrote: > { > zero_count = 0; > continue; > } Done. https://codereview.chromium.org/23566013/diff/115001/media/mp2t/es_parser_h26... media/mp2t/es_parser_h264.cc:193: // Resume NAL segmentation where it was left. On 2013/09/17 16:11:52, damienv1 wrote: > Comment should be updated. Done. https://codereview.chromium.org/23566013/diff/115001/media/mp2t/es_parser_h26... media/mp2t/es_parser_h264.cc:194: for ( ; es_pos_ < raw_es_size - 4; es_pos_++) { On 2013/09/17 16:11:52, damienv1 wrote: > Should be "- 5" to account for the size of the syncword and the NAL header byte. My mistake, 4 is the right value. If syncword_length == 4: es_pos_ + syncword_length < raw_es_size which is good. https://codereview.chromium.org/23566013/diff/115001/media/mp2t/es_parser_h26... media/mp2t/es_parser_h264.cc:210: RCHECK(NalParser(&raw_es[current_nal_pos_], nal_size)); On 2013/09/17 16:11:52, damienv1 wrote: > The NAL parser should receive a pointer to the NAL header, i.e. remove the start > code (so we don't have to check the start code again in the NAL parser). > Use a |current_nal_syncword_length_| Done.
Almost there. Mostly minor nits but the comments about the new_buffer_cb_ sequence and the failure to propagate callback errors are preventing the l g t m. https://codereview.chromium.org/23566013/diff/40001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/23566013/diff/40001/media/media.gyp#newcode864 media/media.gyp:864: 'mp2t/es_parser.h', nit: move these above the mp4 code so that they are in alphabetical order. https://codereview.chromium.org/23566013/diff/40001/media/media.gyp#newcode1149 media/media.gyp:1149: 'mp2t/mp2t_stream_parser_unittest.cc', ditto https://codereview.chromium.org/23566013/diff/40001/media/mp2t/es_parser_adts.cc File media/mp2t/es_parser_adts.cc (right): https://codereview.chromium.org/23566013/diff/40001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:113: continue; nit: Add {} since the comment makes this multi-line https://codereview.chromium.org/23566013/diff/40001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:170: << base::HexEncode(&raw_es[es_position], 7); nit: s/7/kAdtsHeaderMinSize/ https://codereview.chromium.org/23566013/diff/40001/media/mp2t/es_parser_h264.cc File media/mp2t/es_parser_h264.cc (right): https://codereview.chromium.org/23566013/diff/40001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:20: // VUI parameters: Table E-1 "Meaning of sample aspect ration indicator" nit: s/ration/ratio https://codereview.chromium.org/23566013/diff/40001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:211: int nal_header = raw_es[es_pos_ + syncword_length]; nit: use current_nal_pos_ here instead just to make it immediately obvious we are reading the first byte of a NALU? https://codereview.chromium.org/23566013/diff/40001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:232: current_access_unit_pos_ = next_aud_pos; nit: Since current_access_unit_pos_ and is_key_frame appear to always get set together WDYT about using the following helper function to ensure these 2 are always set consistently. void SetAccessUnitPos(int pos) { current_access_unit_pos_ = pos; is_key_frame_ = pos >= 0; } If not, then I think you need to add a is_key_frame_ = false; to Flush() so that it is always false when current_Access_unit_pos_ = -1. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/mp2t_stream_pa... File media/mp2t/mp2t_stream_parser.cc (right): https://codereview.chromium.org/23566013/diff/40001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:359: // The pid filter must be updated. nit: Remove comment since it isn't really adding any value or reword so that it says why the update is necessary. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:450: Shouldn't the is_config_sent properties be set to true above? https://codereview.chromium.org/23566013/diff/40001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:476: if (audio_queue_chain_.empty()) { whoa! How does this happen? ISTM that we should not have this if it can't happen or the ADTS code should not be handing out buffers if it hasn't gotten a valid config yet. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:506: if (video_queue_chain_.empty()) { ditto. The parser calling this shouldn't be handing out buffers if it hasn't sent a config for them yet. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:542: config_cb_.Run(last_audio_config_, last_video_config_); This can return false. If this returns false, it must cause Parse() to return false. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:558: new_buffers_cb_.Run(audio_queue, video_queue); This can return false. If it returns false, it must cause Parse() to return false. IIUC this code could potentially cause queues to be passed in such a way that buffers for the same time range aren't always passed together. For example NewConfig : A1, V1 NewBuffers: A [0-10) V [0-2) NewConfig : A1, V2 NewBuffers: A [) V [2-10) This could become problematic especially once I implement the "sequence" mode. Ideally it should look something like the following NewConfig : A1, V1 NewBuffers: A[0-2) V[0-2) NewConfig : A1, V2 NewBuffers: A[2,10), V[2,10) I realize that the boundaries won't always line up, but insuring that the buffers are always appended in time increasing order would be helpful. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/mp2t_stream_pa... File media/mp2t/mp2t_stream_parser_unittest.cc (right): https://codereview.chromium.org/23566013/diff/40001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser_unittest.cc:58: void InitF(bool init_ok, base::TimeDelta duration) { nit: drop F suffix here and below. You can use On prefix instead (ie OnInit or OnInitDone) if you want to show that these are callback methods. That matches the style we typically use in the media code. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser_unittest.cc:161: TEST_F(Mp2tStreamParserTest, UnalignedAppend) { nit: Add a version that appends using a prime & small piece size like 7 or 17 just to smoke out "power of 2" parser assumptions. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/ts_packet.h File media/mp2t/ts_packet.h (right): https://codereview.chromium.org/23566013/diff/40001/media/mp2t/ts_packet.h#ne... media/mp2t/ts_packet.h:35: int pid() const { nit: use one line for this and any of the ones below that will fit on a single line. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/ts_section_pes.cc File media/mp2t/ts_section_pes.cc (right): https://codereview.chromium.org/23566013/diff/40001/media/mp2t/ts_section_pes... media/mp2t/ts_section_pes.cc:127: parse_result = parse_result && Emit(false); nit: just return here since you don't use the updated value of parse_result https://codereview.chromium.org/23566013/diff/40001/media/mp2t/ts_section_pmt.cc File media/mp2t/ts_section_pmt.cc (right): https://codereview.chromium.org/23566013/diff/40001/media/mp2t/ts_section_pmt... media/mp2t/ts_section_pmt.cc:83: while (section_remaining_size >= 5) { nit: Can't you reorganize the math so this condition can become (bit_reader->bits_available() > pid_map_end_marker) and you don't have to update section_remaining_size inside the loop?
https://codereview.chromium.org/23566013/diff/40001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/23566013/diff/40001/media/media.gyp#newcode864 media/media.gyp:864: 'mp2t/es_parser.h', On 2013/09/18 01:46:05, acolwell wrote: > nit: move these above the mp4 code so that they are in alphabetical order. Done. https://codereview.chromium.org/23566013/diff/40001/media/media.gyp#newcode1149 media/media.gyp:1149: 'mp2t/mp2t_stream_parser_unittest.cc', On 2013/09/18 01:46:05, acolwell wrote: > ditto Done. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/es_parser_adts.cc File media/mp2t/es_parser_adts.cc (right): https://codereview.chromium.org/23566013/diff/40001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:113: continue; On 2013/09/18 01:46:05, acolwell wrote: > nit: Add {} since the comment makes this multi-line Done. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/es_parser_adts... media/mp2t/es_parser_adts.cc:170: << base::HexEncode(&raw_es[es_position], 7); On 2013/09/18 01:46:05, acolwell wrote: > nit: s/7/kAdtsHeaderMinSize/ Done. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/es_parser_h264.cc File media/mp2t/es_parser_h264.cc (right): https://codereview.chromium.org/23566013/diff/40001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:20: // VUI parameters: Table E-1 "Meaning of sample aspect ration indicator" On 2013/09/18 01:46:05, acolwell wrote: > nit: s/ration/ratio Done. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:211: int nal_header = raw_es[es_pos_ + syncword_length]; On 2013/09/18 01:46:05, acolwell wrote: > nit: use current_nal_pos_ here instead just to make it immediately obvious we > are reading the first byte of a NALU? Done. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/es_parser_h264... media/mp2t/es_parser_h264.cc:232: current_access_unit_pos_ = next_aud_pos; On 2013/09/18 01:46:05, acolwell wrote: > nit: Since current_access_unit_pos_ and is_key_frame appear to always get set > together WDYT about using the following helper function to ensure these 2 are > always set consistently. > > void SetAccessUnitPos(int pos) { > current_access_unit_pos_ = pos; > is_key_frame_ = pos >= 0; > } > > If not, then I think you need to add a is_key_frame_ = false; to Flush() so that > it is always false when current_Access_unit_pos_ = -1. Done. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/mp2t_stream_pa... File media/mp2t/mp2t_stream_parser.cc (right): https://codereview.chromium.org/23566013/diff/40001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:359: // The pid filter must be updated. On 2013/09/18 01:46:05, acolwell wrote: > nit: Remove comment since it isn't really adding any value or reword so that it > says why the update is necessary. Done. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:450: On 2013/09/18 01:46:05, acolwell wrote: > Shouldn't the is_config_sent properties be set to true above? Done. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:476: if (audio_queue_chain_.empty()) { On 2013/09/18 01:46:05, acolwell wrote: > whoa! How does this happen? ISTM that we should not have this if it can't happen > or the ADTS code should not be handing out buffers if it hasn't gotten a valid > config yet. For audio (at least for the codecs I know), this should not happen. Each frame includes an audio config. I added the code to be consistent with video. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:506: if (video_queue_chain_.empty()) { On 2013/09/18 01:46:05, acolwell wrote: > ditto. The parser calling this shouldn't be handing out buffers if it hasn't > sent a config for them yet. My goal it to make the ES parser dumb (just segment and output frames, and signal config changes). I think that's the goal of the StreamParser to handle these use cases (especially if we want the StreamParser to be a little bit smarter when dealing with these non-key frames). https://codereview.chromium.org/23566013/diff/40001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:542: config_cb_.Run(last_audio_config_, last_video_config_); On 2013/09/18 01:46:05, acolwell wrote: > This can return false. If this returns false, it must cause Parse() to return > false. Done. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser.cc:558: new_buffers_cb_.Run(audio_queue, video_queue); On 2013/09/18 01:46:05, acolwell wrote: > This can return false. If it returns false, it must cause Parse() to return > false. > > IIUC this code could potentially cause queues to be passed in such a way that > buffers for the same time range aren't always passed together. For example > NewConfig : A1, V1 > NewBuffers: A [0-10) V [0-2) > NewConfig : A1, V2 > NewBuffers: A [) V [2-10) > > This could become problematic especially once I implement the "sequence" mode. > Ideally it should look something like the following > NewConfig : A1, V1 > NewBuffers: A[0-2) V[0-2) > NewConfig : A1, V2 > NewBuffers: A[2,10), V[2,10) > > I realize that the boundaries won't always line up, but insuring that the > buffers are always appended in time increasing order would be helpful. > Done for checking the result of new_buffers_cb_.Run(). I also lined up the segments although I didn't exactly understand the rational behind it. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/mp2t_stream_pa... File media/mp2t/mp2t_stream_parser_unittest.cc (right): https://codereview.chromium.org/23566013/diff/40001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser_unittest.cc:58: void InitF(bool init_ok, base::TimeDelta duration) { On 2013/09/18 01:46:05, acolwell wrote: > nit: drop F suffix here and below. You can use On prefix instead (ie OnInit or > OnInitDone) if you want to show that these are callback methods. That matches > the style we typically use in the media code. That was initially some code I took from the MP4 stream parser unit tests. I am changing it to "On..." https://codereview.chromium.org/23566013/diff/40001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser_unittest.cc:161: TEST_F(Mp2tStreamParserTest, UnalignedAppend) { On 2013/09/18 01:46:05, acolwell wrote: > nit: Add a version that appends using a prime & small piece size like 7 or 17 > just to smoke out "power of 2" parser assumptions. Done. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/ts_packet.h File media/mp2t/ts_packet.h (right): https://codereview.chromium.org/23566013/diff/40001/media/mp2t/ts_packet.h#ne... media/mp2t/ts_packet.h:35: int pid() const { On 2013/09/18 01:46:05, acolwell wrote: > nit: use one line for this and any of the ones below that will fit on a single > line. Done. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/ts_section_pes.cc File media/mp2t/ts_section_pes.cc (right): https://codereview.chromium.org/23566013/diff/40001/media/mp2t/ts_section_pes... media/mp2t/ts_section_pes.cc:127: parse_result = parse_result && Emit(false); On 2013/09/18 01:46:05, acolwell wrote: > nit: just return here since you don't use the updated value of parse_result Done. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/ts_section_pmt.cc File media/mp2t/ts_section_pmt.cc (right): https://codereview.chromium.org/23566013/diff/40001/media/mp2t/ts_section_pmt... media/mp2t/ts_section_pmt.cc:83: while (section_remaining_size >= 5) { On 2013/09/18 01:46:05, acolwell wrote: > nit: Can't you reorganize the math so this condition can become > (bit_reader->bits_available() > pid_map_end_marker) and you don't have to update > section_remaining_size inside the loop? Done.
LGTM % minor nits. https://codereview.chromium.org/23566013/diff/40001/media/mp2t/mp2t_stream_pa... File media/mp2t/mp2t_stream_parser_unittest.cc (right): https://codereview.chromium.org/23566013/diff/40001/media/mp2t/mp2t_stream_pa... media/mp2t/mp2t_stream_parser_unittest.cc:58: void InitF(bool init_ok, base::TimeDelta duration) { On 2013/09/18 21:40:17, damienv1 wrote: > On 2013/09/18 01:46:05, acolwell wrote: > > nit: drop F suffix here and below. You can use On prefix instead (ie OnInit or > > OnInitDone) if you want to show that these are callback methods. That matches > > the style we typically use in the media code. > > That was initially some code I took from the MP4 stream parser unit tests. > I am changing it to "On..." Heh. I just realized this earlier today because I was writing similar tests for the MP3 parser. I'll be sending out a patch to fix the MP4 code. Ultimately, I'd like to reuse most of this boiler plate stuff for all these unittests. Another day.. another CL. :) https://codereview.chromium.org/23566013/diff/129001/media/mp2t/mp2t_stream_p... File media/mp2t/mp2t_stream_parser.cc (right): https://codereview.chromium.org/23566013/diff/129001/media/mp2t/mp2t_stream_p... media/mp2t/mp2t_stream_parser.cc:279: RCHECK(EmitRemainingBuffers()); nit: Just return instead of RCHECK. https://codereview.chromium.org/23566013/diff/129001/media/mp2t/mp2t_stream_p... media/mp2t/mp2t_stream_parser.cc:409: nit: DCHECK(video_decoder_config.IsValid()) since downstream code appears to assume this is true and the caller should be returning a parse error if it isn't. https://codereview.chromium.org/23566013/diff/129001/media/mp2t/mp2t_stream_p... media/mp2t/mp2t_stream_parser.cc:435: nit: DCHECK(audio_decoder_config.IsValid()) . https://codereview.chromium.org/23566013/diff/129001/media/mp2t/mp2t_stream_p... media/mp2t/mp2t_stream_parser.cc:575: last_audio_config_ = queue_with_config.audio_config; nit: It looks like these don't need to be member variables anymore. Please convert to local variables.
https://codereview.chromium.org/23566013/diff/129001/media/mp2t/mp2t_stream_p... File media/mp2t/mp2t_stream_parser.cc (right): https://codereview.chromium.org/23566013/diff/129001/media/mp2t/mp2t_stream_p... media/mp2t/mp2t_stream_parser.cc:279: RCHECK(EmitRemainingBuffers()); On 2013/09/18 22:38:55, acolwell wrote: > nit: Just return instead of RCHECK. Done. https://codereview.chromium.org/23566013/diff/129001/media/mp2t/mp2t_stream_p... media/mp2t/mp2t_stream_parser.cc:409: On 2013/09/18 22:38:55, acolwell wrote: > nit: DCHECK(video_decoder_config.IsValid()) since downstream code appears to > assume this is true and the caller should be returning a parse error if it > isn't. Done. https://codereview.chromium.org/23566013/diff/129001/media/mp2t/mp2t_stream_p... media/mp2t/mp2t_stream_parser.cc:435: On 2013/09/18 22:38:55, acolwell wrote: > nit: DCHECK(audio_decoder_config.IsValid()) . Done. https://codereview.chromium.org/23566013/diff/129001/media/mp2t/mp2t_stream_p... media/mp2t/mp2t_stream_parser.cc:575: last_audio_config_ = queue_with_config.audio_config; On 2013/09/18 22:38:55, acolwell wrote: > nit: It looks like these don't need to be member variables anymore. Please > convert to local variables. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/damienv@chromium.org/23566013/156001
Retried try job too often on win_rel for step(s) sync_integration_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/damienv@chromium.org/23566013/156001
Retried try job too often on win_rel for step(s) sync_integration_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/damienv@chromium.org/23566013/156001
Message was sent while issue was closed.
Change committed as 224404 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
