Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
399433003:
Mpeg2 TS - Fail when no valid timestamp in the ADTS parser. (Closed)
|
Created:
6 years, 5 months ago by damienv1 Modified:
6 years, 4 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionMpeg2 TS - Fail when no valid timestamp in the ADTS parser.
BUG=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289944
Patch Set 1 #Patch Set 2 : #
Total comments: 5
Patch Set 3 : Add a baseline ADTS parser unit test. #
Total comments: 11
Patch Set 4 : Add an helper class for ES parser unit tests. #Patch Set 5 : Add missing files. #Patch Set 6 : #Patch Set 7 : Update ComputePacketSize prototype. #
Total comments: 17
Patch Set 8 : Address some of the nits. #Patch Set 9 : Adding missing MEDIA_EXPORT needed by unit tests. #Patch Set 10 : #
Total comments: 2
Patch Set 11 : ES parser test base. #Patch Set 12 : #
Total comments: 15
Patch Set 13 : Fix comments from patch set #12. #
Total comments: 7
Patch Set 14 : Rebase #Patch Set 15 : Fix rebase: do not set dts in adts parser. #
Messages
Total messages: 27 (0 generated)
PTAL, thanks.
A couple questions. I would also like to see some test(s) that would show the issue (at least on dcheck-enabled builds). https://codereview.chromium.org/399433003/diff/20001/media/formats/mp2t/es_pa... File media/formats/mp2t/es_parser_adts.cc (right): https://codereview.chromium.org/399433003/diff/20001/media/formats/mp2t/es_pa... media/formats/mp2t/es_parser_adts.cc:148: while (!pts_list_.empty() && I'm curious since I haven't looked through this code in detail before now: Is it possible for |pts_list_| to contain more than one PTS, each with a queue offset <= the adts_frame.queue_offset here? Why? (And if now, why loop with a while(){}?) https://codereview.chromium.org/399433003/diff/20001/media/formats/mp2t/es_pa... media/formats/mp2t/es_parser_adts.cc:255: base::TimeDelta base_timestamp = audio_timestamp_helper_->GetTimestamp(); What if base timestamp was not previously set? Is this possible, and if so, it looks to me like a DCHECK would be hit at https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_t...
https://codereview.chromium.org/399433003/diff/20001/media/formats/mp2t/es_pa... File media/formats/mp2t/es_parser_adts.cc (right): https://codereview.chromium.org/399433003/diff/20001/media/formats/mp2t/es_pa... media/formats/mp2t/es_parser_adts.cc:148: while (!pts_list_.empty() && On 2014/07/15 19:57:42, wolenetz wrote: > I'm curious since I haven't looked through this code in detail before now: Is it > possible for |pts_list_| to contain more than one PTS, each with a queue offset > <= the adts_frame.queue_offset here? Why? (And if now, why loop with a > while(){}?) How many PTS are in this list all depend on the stream: e.g. if you have multiple frames per PES, if an ADTS frame spans over multiple PES (although this is not advised to do so). The loop aims at getting the PTS associated to the byte position that comes just before the audio frame byte position (that's how the PTS is defined in Mpeg2 TS). https://codereview.chromium.org/399433003/diff/20001/media/formats/mp2t/es_pa... media/formats/mp2t/es_parser_adts.cc:255: base::TimeDelta base_timestamp = audio_timestamp_helper_->GetTimestamp(); On 2014/07/15 19:57:42, wolenetz wrote: > What if base timestamp was not previously set? Is this possible, and if so, it > looks to me like a DCHECK would be hit at > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_t... Good catch. Thanks.
Thanks for adding tests. Looking pretty good, just some test cleanup and thoroughness comments/nits: https://codereview.chromium.org/399433003/diff/20001/media/formats/mp2t/es_pa... File media/formats/mp2t/es_parser_adts.cc (right): https://codereview.chromium.org/399433003/diff/20001/media/formats/mp2t/es_pa... media/formats/mp2t/es_parser_adts.cc:148: while (!pts_list_.empty() && On 2014/07/15 21:26:59, damienv1 wrote: > On 2014/07/15 19:57:42, wolenetz wrote: > > I'm curious since I haven't looked through this code in detail before now: Is > it > > possible for |pts_list_| to contain more than one PTS, each with a queue > offset > > <= the adts_frame.queue_offset here? Why? (And if now, why loop with a > > while(){}?) > > How many PTS are in this list all depend on the stream: e.g. if you have > multiple frames per PES, if an ADTS frame spans over multiple PES (although this > is not advised to do so). > The loop aims at getting the PTS associated to the byte position that comes just > before the audio frame byte position (that's how the PTS is defined in Mpeg2 > TS). Acknowledged. https://codereview.chromium.org/399433003/diff/40001/media/formats/mp2t/es_pa... File media/formats/mp2t/es_parser_adts_unittest.cc (right): https://codereview.chromium.org/399433003/diff/40001/media/formats/mp2t/es_pa... media/formats/mp2t/es_parser_adts_unittest.cc:27: struct Packet { Some of this (struct Packet, void ComputePacketSize, and various utility methods in the test class) looks very much in common with es_parser_h264_unittest.cc. ILTM like it would a good idea to split out a common test header/class, possible as a common superclass of EsParserH264Test and EsParserAdtsTest. https://codereview.chromium.org/399433003/diff/40001/media/formats/mp2t/es_pa... media/formats/mp2t/es_parser_adts_unittest.cc:40: void ComputePacketSize(std::vector<Packet>& packets, size_t stream_size) { lint nit: Is this a non-const reference? If so, make const or use a pointer. https://codereview.chromium.org/399433003/diff/40001/media/formats/mp2t/es_pa... media/formats/mp2t/es_parser_adts_unittest.cc:145: EXPECT_FALSE(ProcessPesPackets(pes_packets, false)); To verify the first condition added in this change ("don't emit a buffer if no valid pts yet"), EXPECT_EQ(1u, config_count_); and EXPECT_EQ(0u, buffer_count_); after confirming failure of ProcessPesPackets() here? https://codereview.chromium.org/399433003/diff/40001/media/formats/mp2t/es_pa... media/formats/mp2t/es_parser_adts_unittest.cc:147: Is it possible to distinctly verify the second condition added in this change ("don't GetTimestamp() a kNoTimestamp()-as-base_timestamp() audio helper when updating audio config")? IMO, this would be a lower pri test versus your "NoInitialPts" test. https://codereview.chromium.org/399433003/diff/40001/media/formats/mp2t/es_pa... media/formats/mp2t/es_parser_adts_unittest.cc:155: EXPECT_EQ(config_count_, 1u); nit: here and elsewhere in this file, EXPECT_EQ(<expectation>, <actual>) per https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...
https://codereview.chromium.org/399433003/diff/40001/media/formats/mp2t/es_pa... File media/formats/mp2t/es_parser_adts_unittest.cc (right): https://codereview.chromium.org/399433003/diff/40001/media/formats/mp2t/es_pa... media/formats/mp2t/es_parser_adts_unittest.cc:27: struct Packet { On 2014/07/15 21:59:57, wolenetz wrote: > Some of this (struct Packet, void ComputePacketSize, and various utility methods > in the test class) looks very much in common with es_parser_h264_unittest.cc. > ILTM like it would a good idea to split out a common test header/class, possible > as a common superclass of EsParserH264Test and EsParserAdtsTest. Will start an es_parser_test_helper.* https://codereview.chromium.org/399433003/diff/40001/media/formats/mp2t/es_pa... media/formats/mp2t/es_parser_adts_unittest.cc:40: void ComputePacketSize(std::vector<Packet>& packets, size_t stream_size) { On 2014/07/15 21:59:57, wolenetz wrote: > lint nit: Is this a non-const reference? If so, make const or use a pointer. I made it a reference since otherwise packets[k] does not have the same meaning. In that case, I'll have to do sth like: packets->at(k) which becomes less readable. https://codereview.chromium.org/399433003/diff/40001/media/formats/mp2t/es_pa... media/formats/mp2t/es_parser_adts_unittest.cc:145: EXPECT_FALSE(ProcessPesPackets(pes_packets, false)); On 2014/07/15 21:59:57, wolenetz wrote: > To verify the first condition added in this change ("don't emit a buffer if no > valid pts yet"), EXPECT_EQ(1u, config_count_); and EXPECT_EQ(0u, buffer_count_); > after confirming failure of ProcessPesPackets() here? I'll check the buffers. On the other side, whether the implementation emits the corresponding audio config is an implementation detail (we could have an implementation which does not emit the audio config when the frame is not valid from a timestamp perspective). https://codereview.chromium.org/399433003/diff/40001/media/formats/mp2t/es_pa... media/formats/mp2t/es_parser_adts_unittest.cc:155: EXPECT_EQ(config_count_, 1u); On 2014/07/15 21:59:57, wolenetz wrote: > nit: here and elsewhere in this file, EXPECT_EQ(<expectation>, <actual>) per > https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl... Done.
https://codereview.chromium.org/399433003/diff/40001/media/formats/mp2t/es_pa... File media/formats/mp2t/es_parser_adts_unittest.cc (right): https://codereview.chromium.org/399433003/diff/40001/media/formats/mp2t/es_pa... media/formats/mp2t/es_parser_adts_unittest.cc:40: void ComputePacketSize(std::vector<Packet>& packets, size_t stream_size) { On 2014/07/15 22:22:40, damienv1 wrote: > On 2014/07/15 21:59:57, wolenetz wrote: > > lint nit: Is this a non-const reference? If so, make const or use a pointer. > > I made it a reference since otherwise packets[k] does not have the same meaning. > In that case, I'll have to do sth like: packets->at(k) which becomes less > readable. non-const ref violates http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Refere... Would using a const pointer, and sth like: (*packets)[k] work? (And add some ASSERT_TRUE(packets) to protect against null |packets| deref...) https://codereview.chromium.org/399433003/diff/40001/media/formats/mp2t/es_pa... media/formats/mp2t/es_parser_adts_unittest.cc:145: EXPECT_FALSE(ProcessPesPackets(pes_packets, false)); On 2014/07/15 22:22:40, damienv1 wrote: > On 2014/07/15 21:59:57, wolenetz wrote: > > To verify the first condition added in this change ("don't emit a buffer if no > > valid pts yet"), EXPECT_EQ(1u, config_count_); and EXPECT_EQ(0u, > buffer_count_); > > after confirming failure of ProcessPesPackets() here? > > I'll check the buffers. > On the other side, whether the implementation emits the corresponding audio > config is an implementation detail (we could have an implementation which does > not emit the audio config when the frame is not valid from a timestamp > perspective). Acknowledged.
Mostly just nits left. Thanks for breaking out the common code from the two test files. https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_test_helper.cc (right): https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_test_helper.cc:16: size(0u) { I'm not sure, but you might want to default-initialize packet PTS to kNoTimestamp(). https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_test_helper.cc:19: void ComputePacketSize(std::vector<Packet>* packets, size_t stream_size) { nit: Following convention in media/base/test_helpers.h, append "ForTest" to these function names (and s/Packet/TestPacket/) that are test-only but in the media::mp2t namespace? https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_test_helper.cc:24: Packet* cur = &packets->at(0); nit: I'm not convinced you need to use at(k) or at(k+1) instead of just [k] or [k+1], eg: Packet* cur = &packets->front(); (or) Packet* cur = &(*packets)[0]; ... Packet* next = &(*packets)[k+1]; In both cases, & has lower precedence than ->, (), []. https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_test_helper.cc:31: cur->size = stream_size - cur->offset; nit: First, DCHECK_GE(stream_size, cur->offset); ? https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_test_helper.cc:39: for (size_t k = 0; k < pes_packets.size(); k++) { nit: First, ASSERT_TRUE(es_parser); ? https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_test_helper.cc:48: if (!es_parser->Parse(&stream[cur_pes_offset], cur_pes_size, pts, dts)) nit: just pass kNoTimestamp() here instead of dts (and drop dts from this block), or is sth other than kNoTimestamp() sometimes expected for dts? https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_test_helper.h (right): https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_test_helper.h:31: void ComputePacketSize(std::vector<Packet>* packets, size_t stream_size); Hmm. I'm not absolutely sure of convention here, but |stream_size| is definitely an input, and |packets| is both input and output, so I would recommend reversing the argument sequence so all inputs are prior to outputs.
On 2014/07/16 00:21:04, wolenetz wrote: > Mostly just nits left. Thanks for breaking out the common code from the two test > files. > > https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... > File media/formats/mp2t/es_parser_test_helper.cc (right): > > https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... > media/formats/mp2t/es_parser_test_helper.cc:16: size(0u) { > I'm not sure, but you might want to default-initialize packet PTS to > kNoTimestamp(). > > https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... > media/formats/mp2t/es_parser_test_helper.cc:19: void > ComputePacketSize(std::vector<Packet>* packets, size_t stream_size) { > nit: Following convention in media/base/test_helpers.h, append "ForTest" to > these function names (and s/Packet/TestPacket/) that are test-only but in the > media::mp2t namespace? > > https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... > media/formats/mp2t/es_parser_test_helper.cc:24: Packet* cur = &packets->at(0); > nit: I'm not convinced you need to use at(k) or at(k+1) instead of just [k] or > [k+1], eg: > Packet* cur = &packets->front(); > (or) > Packet* cur = &(*packets)[0]; > ... > Packet* next = &(*packets)[k+1]; > > In both cases, & has lower precedence than ->, (), []. > > https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... > media/formats/mp2t/es_parser_test_helper.cc:31: cur->size = stream_size - > cur->offset; > nit: First, DCHECK_GE(stream_size, cur->offset); ? > > https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... > media/formats/mp2t/es_parser_test_helper.cc:39: for (size_t k = 0; k < > pes_packets.size(); k++) { > nit: First, ASSERT_TRUE(es_parser); ? > > https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... > media/formats/mp2t/es_parser_test_helper.cc:48: if > (!es_parser->Parse(&stream[cur_pes_offset], cur_pes_size, pts, dts)) > nit: just pass kNoTimestamp() here instead of dts (and drop dts from this > block), or is sth other than kNoTimestamp() sometimes expected for dts? > > https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... > File media/formats/mp2t/es_parser_test_helper.h (right): > > https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... > media/formats/mp2t/es_parser_test_helper.h:31: void > ComputePacketSize(std::vector<Packet>* packets, size_t stream_size); > Hmm. I'm not absolutely sure of convention here, but |stream_size| is definitely > an input, and |packets| is both input and output, so I would recommend reversing > the argument sequence so all inputs are prior to outputs. acolwell@: For common test helpers, is convention to use sth like EsParserTestBase, or standalone "XxxxForTest" methods and standalone "XxxxxTest" structs. The latter is the route this CL in PS7 is taking. I've seen both routes in media. What's your guidance here?
Damien - it looks like the android bots fail compile in PS7, like: ../../media/formats/mp2t/es_parser_adts_unittest.cc:84:18:error: 'class std::vector<unsigned char>' has no member named 'data' memcpy(stream_.data(), stream.data(), stream_.size());
One more nit... https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_test_helper.h (right): https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_test_helper.h:31: void ComputePacketSize(std::vector<Packet>* packets, size_t stream_size); nit: s/* packets/* const packets/ ?
https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_test_helper.cc (right): https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_test_helper.cc:16: size(0u) { On 2014/07/16 00:21:03, wolenetz wrote: > I'm not sure, but you might want to default-initialize packet PTS to > kNoTimestamp(). Done. https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_test_helper.cc:19: void ComputePacketSize(std::vector<Packet>* packets, size_t stream_size) { On 2014/07/16 00:21:03, wolenetz wrote: > nit: Following convention in media/base/test_helpers.h, append "ForTest" to > these function names (and s/Packet/TestPacket/) that are test-only but in the > media::mp2t namespace? In fact, I checked at this file and not all functions/class/struct are prefixed/suffixed with Test or ForTest. I don't know what is the rule here either (Seems like adding Test for everything might make the code a bit less readable). https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_test_helper.cc:24: Packet* cur = &packets->at(0); On 2014/07/16 00:21:03, wolenetz wrote: > nit: I'm not convinced you need to use at(k) or at(k+1) instead of just [k] or > [k+1], eg: > Packet* cur = &packets->front(); > (or) > Packet* cur = &(*packets)[0]; > ... > Packet* next = &(*packets)[k+1]; > > In both cases, & has lower precedence than ->, (), []. Done. https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_test_helper.cc:31: cur->size = stream_size - cur->offset; On 2014/07/16 00:21:03, wolenetz wrote: > nit: First, DCHECK_GE(stream_size, cur->offset); ? Done. https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_test_helper.cc:48: if (!es_parser->Parse(&stream[cur_pes_offset], cur_pes_size, pts, dts)) On 2014/07/16 00:21:04, wolenetz wrote: > nit: just pass kNoTimestamp() here instead of dts (and drop dts from this > block), or is sth other than kNoTimestamp() sometimes expected for dts? I was finding it more explicit to pass a variable name so that I know just by looking at the code that it refers to a DTS. (and test code does not need to be as optimized as production code). Please let me know if you really prefer your suggestion. https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_test_helper.h (right): https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_test_helper.h:31: void ComputePacketSize(std::vector<Packet>* packets, size_t stream_size); On 2014/07/16 00:21:04, wolenetz wrote: > Hmm. I'm not absolutely sure of convention here, but |stream_size| is definitely > an input, and |packets| is both input and output, so I would recommend reversing > the argument sequence so all inputs are prior to outputs. I reversed the 2 arguments.
On 2014/07/16 00:26:07, wolenetz wrote: > acolwell@: For common test helpers, is convention to use sth like > EsParserTestBase, or standalone "XxxxForTest" methods and standalone "XxxxxTest" > structs. The latter is the route this CL in PS7 is taking. I've seen both routes > in media. What's your guidance here? I think putting these structs an functions in a EsParserTestBase class and having the 2 tests derive from that is the better path. That way these objects/methods don't pollute the namespace. I think using derivation here shouldn't cause any clutter since the declarations will be in the scope of the test object. I don't think we actually have a firm rule about this, but I just prefer to have test-only contained within something that has Test in its name.
Just a few nits, though I concur with acolwell's recommendation to restructure the helper into a base test class. https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_test_helper.cc (right): https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_test_helper.cc:48: if (!es_parser->Parse(&stream[cur_pes_offset], cur_pes_size, pts, dts)) On 2014/07/16 02:34:33, damienv1 wrote: > On 2014/07/16 00:21:04, wolenetz wrote: > > nit: just pass kNoTimestamp() here instead of dts (and drop dts from this > > block), or is sth other than kNoTimestamp() sometimes expected for dts? > > I was finding it more explicit to pass a variable name so that I know just by > looking at the code that it refers to a DTS. > (and test code does not need to be as optimized as production code). > Please let me know if you really prefer your suggestion. I have no strong feeling here. Maybe s/dts/no_timestamp_dts/ to make it clear to maintainers that dts is still expected to be kNoTimestamp() when passed as param here? https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_test_helper.h (right): https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_test_helper.h:31: void ComputePacketSize(std::vector<Packet>* packets, size_t stream_size); On 2014/07/16 00:32:28, wolenetz wrote: > nit: s/* packets/* const packets/ ? never mind. I don't see * const for vectors anywhere in media code. https://codereview.chromium.org/399433003/diff/110001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_test_helper.h:31: void ComputePacketSize(std::vector<Packet>* packets, size_t stream_size); On 2014/07/16 02:34:34, damienv1 wrote: > On 2014/07/16 00:21:04, wolenetz wrote: > > Hmm. I'm not absolutely sure of convention here, but |stream_size| is > definitely > > an input, and |packets| is both input and output, so I would recommend > reversing > > the argument sequence so all inputs are prior to outputs. > > I reversed the 2 arguments. I don't see this reversal of arguments in PS10. https://codereview.chromium.org/399433003/diff/170001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_adts.h (right): https://codereview.chromium.org/399433003/diff/170001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_adts.h:29: class MEDIA_EXPORT EsParserAdts : public EsParser { nit: seems to me it would be good to add similar MEDIA_EXPORT to EsParser; it would be more consistent with other media base class exports, eg Demuxer. https://codereview.chromium.org/399433003/diff/170001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_adts_unittest.cc (right): https://codereview.chromium.org/399433003/diff/170001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_adts_unittest.cc:84: memcpy(&stream_[0], stream.data(), stream_.size()); nit: only do this line if stream.length() > 0. Perhaps you might want to error out if stream.length() <= 0 prior to the resize, above?
No need to review patch set #11 (at best, just look at it at a high level to confirm this is the right approach, no need to waste some time on details that might change). This is an intermediate drop (probably further cleanup).
PTAL. Thanks.
Ping on this. Thanks !
lgtm https://codereview.chromium.org/399433003/diff/210001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_h264_unittest.cc (right): https://codereview.chromium.org/399433003/diff/210001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_h264_unittest.cc:138: void EsParserH264Test::GetPesTimestamps(std::vector<Packet>& pes_packets) { nit: This should probably be std::vector<Packet>* since this is an out parameter.
+dalecurtis@ for src/media/BUILD.gn lgtm % nits: https://codereview.chromium.org/399433003/diff/210001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_adts.cc (right): https://codereview.chromium.org/399433003/diff/210001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_adts.cc:158: base::TimeDelta current_pts = audio_timestamp_helper_->GetTimestamp(); nit: I prefer a blank line between } and non-}. https://codereview.chromium.org/399433003/diff/210001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_h264_unittest.cc (right): https://codereview.chromium.org/399433003/diff/210001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_h264_unittest.cc:11: #include "base/command_line.h" Here and in the other class derived from EsParserTestBase, are all of these includes necessary? Items like file loading moved to the base class, so shouldn't the includes move there, too? https://codereview.chromium.org/399433003/diff/210001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_test_base.cc (right): https://codereview.chromium.org/399433003/diff/210001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_test_base.cc:45: void EsParserTestBase::NewAudioConfig(const AudioDecoderConfig& config) { nit: add include https://codereview.chromium.org/399433003/diff/210001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_test_base.cc:49: void EsParserTestBase::NewVideoConfig(const VideoDecoderConfig& config) { nit: ditto https://codereview.chromium.org/399433003/diff/210001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/399433003/diff/210001/media/media.gyp#newcode... media/media.gyp:1256: 'formats/mp2t/es_parser_adts_unittest.cc', Please similarly update src/media/BUILD.gn (dalecurtis@ added to CR for this).
https://codereview.chromium.org/399433003/diff/210001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_adts.cc (right): https://codereview.chromium.org/399433003/diff/210001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_adts.cc:158: base::TimeDelta current_pts = audio_timestamp_helper_->GetTimestamp(); On 2014/08/14 18:46:29, wolenetz wrote: > nit: I prefer a blank line between } and non-}. I usually put code in logical groups: here we really can call GetTimestamp only because we check previously that base_timestamp() is valid. So to me this belongs to the same logical group. Let me know if this is a strong preference, thanks. https://codereview.chromium.org/399433003/diff/210001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_h264_unittest.cc (right): https://codereview.chromium.org/399433003/diff/210001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_h264_unittest.cc:11: #include "base/command_line.h" On 2014/08/14 18:46:29, wolenetz wrote: > Here and in the other class derived from EsParserTestBase, are all of these > includes necessary? Items like file loading moved to the base class, so > shouldn't the includes move there, too? Done. https://codereview.chromium.org/399433003/diff/210001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_h264_unittest.cc:138: void EsParserH264Test::GetPesTimestamps(std::vector<Packet>& pes_packets) { On 2014/08/14 17:45:48, acolwell wrote: > nit: This should probably be std::vector<Packet>* since this is an out > parameter. Done. https://codereview.chromium.org/399433003/diff/210001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_test_base.cc (right): https://codereview.chromium.org/399433003/diff/210001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_test_base.cc:45: void EsParserTestBase::NewAudioConfig(const AudioDecoderConfig& config) { On 2014/08/14 18:46:29, wolenetz wrote: > nit: add include We only have a const reference to a structure we don't use. So we should not have to include "audio_decoder_config.h" https://codereview.chromium.org/399433003/diff/210001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_test_base.cc:49: void EsParserTestBase::NewVideoConfig(const VideoDecoderConfig& config) { On 2014/08/14 18:46:29, wolenetz wrote: > nit: ditto Same answer. https://codereview.chromium.org/399433003/diff/210001/media/media.gyp File media/media.gyp (right): https://codereview.chromium.org/399433003/diff/210001/media/media.gyp#newcode... media/media.gyp:1256: 'formats/mp2t/es_parser_adts_unittest.cc', On 2014/08/14 18:46:29, wolenetz wrote: > Please similarly update src/media/BUILD.gn (dalecurtis@ added to CR for this). Done.
gn changes lgtm
lgtm % nits and bots applying patch :) https://codereview.chromium.org/399433003/diff/210001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_adts.cc (right): https://codereview.chromium.org/399433003/diff/210001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_adts.cc:158: base::TimeDelta current_pts = audio_timestamp_helper_->GetTimestamp(); On 2014/08/14 22:31:22, damienv1 wrote: > On 2014/08/14 18:46:29, wolenetz wrote: > > nit: I prefer a blank line between } and non-}. > > I usually put code in logical groups: here we really can call GetTimestamp only > because we check previously that base_timestamp() is valid. So to me this > belongs to the same logical group. > > Let me know if this is a strong preference, thanks. Acknowledged. https://codereview.chromium.org/399433003/diff/210001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_test_base.cc (right): https://codereview.chromium.org/399433003/diff/210001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_test_base.cc:45: void EsParserTestBase::NewAudioConfig(const AudioDecoderConfig& config) { On 2014/08/14 22:31:22, damienv1 wrote: > On 2014/08/14 18:46:29, wolenetz wrote: > > nit: add include > > We only have a const reference to a structure we don't use. > So we should not have to include "audio_decoder_config.h" Acknowledged. https://codereview.chromium.org/399433003/diff/210001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_test_base.cc:49: void EsParserTestBase::NewVideoConfig(const VideoDecoderConfig& config) { On 2014/08/14 22:31:22, damienv1 wrote: > On 2014/08/14 18:46:29, wolenetz wrote: > > nit: ditto > > Same answer. Acknowledged. https://codereview.chromium.org/399433003/diff/230001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_h264_unittest.cc (right): https://codereview.chromium.org/399433003/diff/230001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_h264_unittest.cc:134: const std::vector<Packet>& pes_packets = *pes_packets_ptr; nit: first, guard against NULL, like DCHECK(pes_packets_ptr); https://codereview.chromium.org/399433003/diff/230001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_h264_unittest.cc:142: (*pes_packets_ptr)[k].pts = base::TimeDelta::FromMilliseconds(-1); nit: why not s/(*pes_packets_ptr)/pes_packets/ ? https://codereview.chromium.org/399433003/diff/230001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_h264_unittest.cc:154: (*pes_packets_ptr)[pes_idx].pts = access_units_[k].pts; nit: ditto
https://codereview.chromium.org/399433003/diff/230001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_h264_unittest.cc (right): https://codereview.chromium.org/399433003/diff/230001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_h264_unittest.cc:134: const std::vector<Packet>& pes_packets = *pes_packets_ptr; On 2014/08/14 23:13:53, wolenetz wrote: > nit: first, guard against NULL, like DCHECK(pes_packets_ptr); Done. https://codereview.chromium.org/399433003/diff/230001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_h264_unittest.cc:142: (*pes_packets_ptr)[k].pts = base::TimeDelta::FromMilliseconds(-1); On 2014/08/14 23:13:53, wolenetz wrote: > nit: why not s/(*pes_packets_ptr)/pes_packets/ ? Does not work. https://codereview.chromium.org/399433003/diff/230001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_h264_unittest.cc:154: (*pes_packets_ptr)[pes_idx].pts = access_units_[k].pts; On 2014/08/14 23:13:53, wolenetz wrote: > nit: ditto Same.
https://codereview.chromium.org/399433003/diff/230001/media/formats/mp2t/es_p... File media/formats/mp2t/es_parser_h264_unittest.cc (right): https://codereview.chromium.org/399433003/diff/230001/media/formats/mp2t/es_p... media/formats/mp2t/es_parser_h264_unittest.cc:142: (*pes_packets_ptr)[k].pts = base::TimeDelta::FromMilliseconds(-1); On 2014/08/14 23:30:03, damienv1 wrote: > On 2014/08/14 23:13:53, wolenetz wrote: > > nit: why not s/(*pes_packets_ptr)/pes_packets/ ? > > Does not work. Acknowledged. (I'm sorry!)
The CQ bit was checked by damienv@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/damienv@chromium.org/399433003/270001
Message was sent while issue was closed.
Committed patchset #15 (270001) as 289944
Message was sent while issue was closed.
On 2014/08/15 18:26:01, I haz the power (commit-bot) wrote:
> Committed patchset #15 (270001) as 289944
Hello!
Syncing to head gives me a build failure that is, I think, caused by this patch:
[4/46] CXX obj\media\formats\mp2t\media.es_parser_adts.obj
FAILED: ninja -t msvc -e environment.x86 -- "C:\depot_tools\win_toolchain\vs2013
_files\VC\bin\amd64_x86\cl.exe" /nologo /showIncludes /FC @obj\media\formats\mp2
t\media.es_parser_adts.obj.rsp /c ..\..\media\formats\mp2t\es_parser_adts.cc /Fo
obj\media\formats\mp2t\media.es_parser_adts.obj /Fdobj\media\media.cc.pdb
d:\src\chromium\src\media\formats\mp2t\es_parser_adts.h(29) : error C2220: warni
ng treated as error - no 'object' file generated
d:\src\chromium\src\media\formats\mp2t\es_parser_adts.h(29) : warning C4275: non
dll-interface class 'media::mp2t::EsParser' used as base for dll-interface clas
s 'media::mp2t::EsParserAdts'
d:\src\chromium\src\media\formats\mp2t\es_parser.h(20) : see declaration
of 'media::mp2t::EsParser'
d:\src\chromium\src\media\formats\mp2t\es_parser_adts.h(29) : see declar
ation of 'media::mp2t::EsParserAdts'
[4/46] CXX obj\media\formats\mp2t\media.mp2t_stream_parser.obj
FAILED: ninja -t msvc -e environment.x86 -- "C:\depot_tools\win_toolchain\vs2013
_files\VC\bin\amd64_x86\cl.exe" /nologo /showIncludes /FC @obj\media\formats\mp2
t\media.mp2t_stream_parser.obj.rsp /c ..\..\media\formats\mp2t\mp2t_stream_parse
r.cc /Foobj\media\formats\mp2t\media.mp2t_stream_parser.obj /Fdobj\media\media.c
c.pdb
d:\src\chromium\src\media\formats\mp2t\es_parser_adts.h(29) : error C2220: warni
ng treated as error - no 'object' file generated
d:\src\chromium\src\media\formats\mp2t\es_parser_adts.h(29) : warning C4275: non
dll-interface class 'media::mp2t::EsParser' used as base for dll-interface clas
s 'media::mp2t::EsParserAdts'
d:\src\chromium\src\media\formats\mp2t\es_parser.h(20) : see declaration
of 'media::mp2t::EsParser'
d:\src\chromium\src\media\formats\mp2t\es_parser_adts.h(29) : see declar
ation of 'media::mp2t::EsParserAdts'
ninja: build stopped: subcommand failed.
I am building a Debug component build. I found that the following patch allows
compilation to complete:
diff --git a/media/formats/mp2t/es_parser.h b/media/formats/mp2t/es_parser.h
index 96785ea..1519e73 100644
--- a/media/formats/mp2t/es_parser.h
+++ b/media/formats/mp2t/es_parser.h
@@ -17,7 +17,7 @@ class StreamParserBuffer;
namespace mp2t {
-class EsParser {
+class MEDIA_EXPORT EsParser {
public:
typedef base::Callback<void(scoped_refptr<StreamParserBuffer>)> EmitBufferCB;
I'm pretty unfamiliar with this code though and don't know if the above is
correct. Also none of the waterfall bots seem affected, which is odd.
In any case, could we either revert or fix the compilation error? Thanks!
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
