Mpeg2 TS parser: Es parsing using an ES byte queue.

Mpeg2 TS parser: Es parsing using an ES byte queue.

Before this refactoring, the ES management was left
to the ES parser implementation.
It is now moved to the EsParser class and ES management
now relies explicitly on an ES byte queue.

BUG=None

Committed: https://crrev.com/a671042471260325987f7badae09e4ff9ae63c50
Cr-Commit-Position: refs/heads/master@{#292249}

[damienv1, 2014-08-26 22:06:11]

damienv@chromium.org changed reviewers:
+ acolwell@chromium.org, dalecurtis@chromium.org, wolenetz@chromium.org

[wolenetz, 2014-08-26 23:01:44]

looking pretty good

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser.cc (right):

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser.cc:30: bool EsParser::Parse(const uint8* buf, int
size,
nit: protect borders: DCHECK(buf) + DCHECK_GT(size, 0) here (though the
underlying ByteQueue::Push() does this, too)?

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser.h (right):

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser.h:56: // Get the timing descriptor that applies to
position |es_byte_count| inside
nit: Can there be a position in |es_queue_| for which there is no applicable
timing descriptor, though there might be a prior or later (or both) position in
|es_queue_| for which there is an applicable timing descriptor? I'm trying to
understand the comment here and for |timing_desc_list_|. The latter scopes the
applicability to "the first access unit [at or after this position]", so does
this mean that if the position is not at or within an access unit, there is not
an applicable timing descriptor?

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser.h:69: std::list<std::pair<int64, TimingDesc> >
timing_desc_list_;
lint nit: add #include for pair<>

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser_adts.cc (right):

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_adts.cc:179: EsParser::Reset();
This seems fragile to require overriding method to remember to call the
overridden member. Suggestion: Make the existing EsParser::Reset() be
non-virtual and have it first call an abstract method, like "ResetInternal()"
that each concrete EsParser implementation overrides with their custom reset
logic. (Kind of like how you've refactored Parse into super::Parse() and
child::ParseFromEsQueue().)

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser_adts.h (right):

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_adts.h:74: //scoped_ptr<media::OffsetByteQueue>
es_queue_;
Why include a commented-out member in source code?
Also lint nit: need space after //.

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_adts.h:77: //EsPtsList pts_list_;
ditto

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser_h264.cc (right):

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_h264.cc:31: //es_queue_(new
media::OffsetByteQueue()),
ditto

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_h264.cc:39: 
nit: Did you mean to drop the old ::Parse()'s Note about HLS, and its DVLOG_IF?

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_h264.cc:54: void EsParserH264::Reset() {
ditto: super::Reset() should call some child::ResetInternal().

[damienv1, 2014-08-27 16:23:43]

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser.cc (right):

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser.cc:30: bool EsParser::Parse(const uint8* buf, int
size,
On 2014/08/26 23:01:44, wolenetz wrote:
> nit: protect borders: DCHECK(buf) + DCHECK_GT(size, 0) here (though the
> underlying ByteQueue::Push() does this, too)?

Assertions should be done on variables that are actually used to me.
Here we don't explicitly used |buf| and |size| (so it is the task of
OffetByteQueue::Push to verify that arguments meet the assumptions.

I can add them if you still think this is a good idea.

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser.h (right):

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser.h:56: // Get the timing descriptor that applies to
position |es_byte_count| inside
On 2014/08/26 23:01:44, wolenetz wrote:
> nit: Can there be a position in |es_queue_| for which there is no applicable
> timing descriptor, though there might be a prior or later (or both) position
in
> |es_queue_| for which there is an applicable timing descriptor? I'm trying to
> understand the comment here and for |timing_desc_list_|. The latter scopes the
> applicability to "the first access unit [at or after this position]", so does
> this mean that if the position is not at or within an access unit, there is
not
> an applicable timing descriptor?

There is not necessarily a timing descriptor associated with each access unit
(that's why in ADTS we have an AudioTimestampHelper).
I re-phrased the explanation and added some comments on the relationship with
the Mpeg2TS spec.

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser.h:69: std::list<std::pair<int64, TimingDesc> >
timing_desc_list_;
On 2014/08/26 23:01:44, wolenetz wrote:
> lint nit: add #include for pair<>

Done.

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser_adts.cc (right):

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_adts.cc:179: EsParser::Reset();
On 2014/08/26 23:01:44, wolenetz wrote:
> This seems fragile to require overriding method to remember to call the
> overridden member. Suggestion: Make the existing EsParser::Reset() be
> non-virtual and have it first call an abstract method, like "ResetInternal()"
> that each concrete EsParser implementation overrides with their custom reset
> logic. (Kind of like how you've refactored Parse into super::Parse() and
> child::ParseFromEsQueue().)

Done.

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser_adts.h (right):

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_adts.h:74: //scoped_ptr<media::OffsetByteQueue>
es_queue_;
On 2014/08/26 23:01:44, wolenetz wrote:
> Why include a commented-out member in source code?
> Also lint nit: need space after //.

My mistake, I thought I removed all this temporary code.

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_adts.h:77: //EsPtsList pts_list_;
On 2014/08/26 23:01:44, wolenetz wrote:
> ditto

Same answer.

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser_h264.cc (right):

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_h264.cc:31: //es_queue_(new
media::OffsetByteQueue()),
On 2014/08/26 23:01:44, wolenetz wrote:
> ditto

My mistake. Forgot to remove this line.

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_h264.cc:39: 
On 2014/08/26 23:01:44, wolenetz wrote:
> nit: Did you mean to drop the old ::Parse()'s Note about HLS, and its
DVLOG_IF?

I added back the HLS comment in the header file.
Regarding the DVLOG_IF, it is kind of an artificial restriction. If there is no
timing associated with an access unit, we will fail in EmitFrame when trying to
get the timing (this is really what matters). So I dropped the DVLOG_IF.

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser_h264.cc:54: void EsParserH264::Reset() {
On 2014/08/26 23:01:44, wolenetz wrote:
> ditto: super::Reset() should call some child::ResetInternal().

Done.

[damienv1, 2014-08-27 16:37:21]

https://codereview.chromium.org/497203004/diff/40001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser.h (right):

https://codereview.chromium.org/497203004/diff/40001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser.h:55: virtual bool ParseFromEsQueue() = 0;
Maybe at some point, we should consider making EsParserAdts, EsParserH264 some
delegates instead of using derivation where it seems that composition might be a
better design.
EsParserH264 and EsParserAdts would both derive from EsParser::Delegate. The
delegate would consume data from an EsQueue (which includes both the data and
timing).

Could add a TODO for that purpose ?

[wolenetz, 2014-08-27 18:23:02]

lgtm % s/explicitely/explicitly/ in CL description.

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser.cc (right):

https://codereview.chromium.org/497203004/diff/20001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser.cc:30: bool EsParser::Parse(const uint8* buf, int
size,
On 2014/08/27 16:23:43, damienv1 wrote:
> On 2014/08/26 23:01:44, wolenetz wrote:
> > nit: protect borders: DCHECK(buf) + DCHECK_GT(size, 0) here (though the
> > underlying ByteQueue::Push() does this, too)?
> 
> Assertions should be done on variables that are actually used to me.
> Here we don't explicitly used |buf| and |size| (so it is the task of
> OffetByteQueue::Push to verify that arguments meet the assumptions.
> 
> I can add them if you still think this is a good idea.

I have no strong feeling, though if we ever use sth other than an
OffsetByteQueue for |es_queue_|, we might forget to protect this border.

https://codereview.chromium.org/497203004/diff/40001/media/formats/mp2t/es_pa...
File media/formats/mp2t/es_parser.h (right):

https://codereview.chromium.org/497203004/diff/40001/media/formats/mp2t/es_pa...
media/formats/mp2t/es_parser.h:55: virtual bool ParseFromEsQueue() = 0;
On 2014/08/27 16:37:21, damienv1 wrote:
> Maybe at some point, we should consider making EsParserAdts, EsParserH264 some
> delegates instead of using derivation where it seems that composition might be
a
> better design.
> EsParserH264 and EsParserAdts would both derive from EsParser::Delegate. The
> delegate would consume data from an EsQueue (which includes both the data and
> timing).
> 
> Could add a TODO for that purpose ?

I have no strong feeling here. For now, derivation sgtm.

[damienv1, 2014-08-27 20:59:01]

The CQ bit was checked by damienv@chromium.org

[commit-bot: I haz the power, 2014-08-27 21:00:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/damienv@chromium.org/497203004/60001

[commit-bot: I haz the power, 2014-08-27 21:54:05]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-27 22:34:10]

Committed patchset #4 (id:60001) as 468983dcbc5d0910427810d581c8f932feb7ff16

[commit-bot: I haz the power, 2014-09-10 02:54:58]

Patchset 4 (id:??) landed as
https://crrev.com/a671042471260325987f7badae09e4ff9ae63c50
Cr-Commit-Position: refs/heads/master@{#292249}