Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
883403002:
Parsing of encoded duration for unencrypted opus streams. (Closed)
|
Created:
5 years, 10 months ago by chcunningham Modified:
5 years, 10 months ago Reviewers:
wolenetz CC:
chromium-reviews, feature-media-reviews_chromium.org, DaleCurtis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionParsing of encoded duration for unencrypted opus streams.
BUG=396634
Committed: https://crrev.com/4614245a773c1caa6013f061b3b80151552901f5
Cr-Commit-Position: refs/heads/master@{#315395}
Patch Set 1 #
Total comments: 60
Patch Set 2 : Addressing review feedback #
Total comments: 77
Patch Set 3 : Responding to review feedback, round 2. #
Total comments: 43
Patch Set 4 : Adds missing license header #Patch Set 5 : Addressing feedback, round 3. #
Total comments: 6
Patch Set 6 : Responding to final feedback #
Total comments: 2
Patch Set 7 : Minor fix for log line. #
Messages
Total messages: 39 (13 generated)
chcunningham@chromium.org changed reviewers: + wolenetz@chromium.org
A few comments/questions. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... File media/formats/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:36: audio_config_(audio_config), I'm not doing anything with this in ::Reset. Should I? https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... File media/formats/webm/webm_cluster_parser_unittest.cc (right): https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser_unittest.cc:1034: std::vector<uint8> BuildOpusPacket(int config, I'm expecting this return to be RVO'd. Am I right? Is this good with chromium/team style? https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser_unittest.cc:1121: INSTANTIATE_TEST_CASE_P( This was a fun exploration for me, but I think it may be overkill. Across these 3 instantions we end up running over 3K tests! I'm considering moving the TEST_Ps to TEST_F and dealing with the parameterization by simply calling a helper in both tests that generates an array of the full set of opus configs. The tests would just iterate over that list. 3k tests -> 2 tests. Thoughts?
Initial round of comments. Thanks for putting this together. I didn't do a review of the updated unit tests yet. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... File media/formats/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:36: audio_config_(audio_config), On 2015/01/29 22:14:20, chcunningham wrote: > I'm not doing anything with this in ::Reset. Should I? ::Reset() is only for "let's parse another cluster from that track that we were constructed with", so the audio_config_ from ctor should be left alone in ::Reset(). https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:145: bool WebMClusterParser::TryGetEncodedAudioDuration(const uint8* data, I suggest using a base::TimeDelta duration as retval and kNoTimestamp magic value to indicate lack of success? This reduces # of params. caller should DCHECK that the retval in that case is never otherwise negative, zero, or kInfiniteDuration (which maps to negative, but inclusion in DCEHCK improves readability). https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:155: bool WebMClusterParser::ReadOpusDuration(const uint8* data, ditto on retval comment. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:159: MEDIA_LOG(log_cb_) << "Invalid zero-byte Opus packet."; Could it be possible that an append one-byte-at-a-time logs this on 0 byte data first time parse is attempted, but not once another byte is appended? If so, need to protect against spurious log? https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:178: MEDIA_LOG(log_cb_) << "Second byte missing from 'Code 3' Opus packet."; ditto: protect against spurious log? https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:191: *duration = kOpusFrameDurationsMu[opusConfig] * frame_count; frame_count could be zero in a "code 3 packet". Guard against? Likewise, frame_count could be zero even in a "code 2 packet" due to 0 == frame length coding. Extract and guard against for code 2 packets, too? Further, "Any Opus frame in any mode MAY have a length of 0". Does this mean we should look at |size|-1 ?= 0 for "code 1 packets", too (assuming |size| is the actual full size of the complete data in the webm block)? Would a length 0 in this case also need to be disallowed?? Or just not emit a buffer? (I think the latter is more tolerant with intent of Opus codec itself, though am not clear if webm allows for it.) WDYT? btw, that rfc is full-of-badness. Especially 3.2.1. talking about code 3 packet interpretation of the frame length code that is completely conflicting versus 3.2.5. Did you find any reinforcement that 3.2.5. is indeed the one tru way of interpreting code 3 packet frame count? Next, code 3 packet (3.2.5): M MUST NOT be zero: where is guard against this here? Next, code 3 packet (3.2.5): audio duration contained within a packet MUST NOT exceed 120ms. (and ancilliary text around max frame count for code 3 is 48): where is guard against this here? And finally, I'd like to understand exactly how bits 0-4 in TOC map to the "top five bits". This wording is very strange, leading me to question whether or not the most-significant bit in an Opus bitstream is first or last in a byte. I would assume so, but still... https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:380: int64 encoded_duration = -1; ditto: base::TimeDelta please https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:385: TryGetEncodedAudioDuration(data, size, &encoded_duration); note the retval was ignored previously... not sure why it was there then. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:386: } nit: no {} necessary for one-line block here. I'm fine if you want it there for readability though. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:386: } I'd like to see at least DVLOG for audio block if there is a BlockGroup->BlockDuration and an encoded duration that significantly mismatches. Perhaps always DVLOG here for unenc audio, but at high verbosity level (like DVLOG(3,...))? If significantly mismatching, media_log() could be appropriate to inform webdevs that something's wrong with content or parser. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... File media/formats/webm/webm_cluster_parser.h (right): https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.h:32: // TODO(wolenetz/acolwell): Parse audio codebook to determine missing audio nit: update bug comment :) (wolenetz/chcunningham), vorbis... https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.h:39: // Opus packets encode the duration and other parameters in the 5 most move to impl. drop last ',' https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.h:136: base::TimeDelta default_duration_; nit: Hmm. it seems the comments for how estimation vs extraction vs default vs hardcode are split up all over. Maybe document at top or have a helper method that handles *all* the duration extraction/fallback estimation/hardcoding logic and put the comments there and refer to that documentation here and in |estimated_next_frame_duration|? This is getting harder and harder to read. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.h:246: // and cannot be calculated when for Blocks at the end of a Cluster (The next nits: grammar (when for), un-cap (The https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.h:251: bool TryGetEncodedAudioDuration(const uint8* data, int size, int64* duration); What is bool retval? https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.h:252: // Reads Opus packet header to determine packet duration. nit: insert blank link prior. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.h:260: const AudioDecoderConfig& audio_config_; Do we need the whole config? Or just some specific info like codec id? Also, I don't like refs-as-members in general (and I don't see any usage of refs-as-members elsewhere in chromium). If you need a non-copy of some class, consider smart pointers. Lifetime, backing-store, etc issues... refs-as-members sound like a path to eventual pain ;)
More comments w.r.t. patch set 1, including a few related to the tests: https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... File media/formats/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:159: MEDIA_LOG(log_cb_) << "Invalid zero-byte Opus packet."; On 2015/01/30 01:48:07, wolenetz wrote: > Could it be possible that an append one-byte-at-a-time logs this on 0 byte data > first time parse is attempted, but not once another byte is appended? If so, > need to protect against spurious log? nm. OnBlock is done on complete block, not partial. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:178: MEDIA_LOG(log_cb_) << "Second byte missing from 'Code 3' Opus packet."; On 2015/01/30 01:48:08, wolenetz wrote: > ditto: protect against spurious log? ditto nm https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:190: int opusConfig = (data[0] & 0xf8) >> 3; nit: put "magic" 0xf8 and others like that into an enum with clear naming to help reader. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:191: *duration = kOpusFrameDurationsMu[opusConfig] * frame_count; On 2015/01/30 01:48:08, wolenetz wrote: > ... > And finally, I'd like to understand exactly how bits 0-4 in TOC map to the "top > five bits". This wording is very strange, leading me to question whether or not > the most-significant bit in an Opus bitstream is first or last in a byte. I > would assume so, but still... The strange wording is clarified elsewhere in that rfc. Thanks for pointing that out. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... File media/formats/webm/webm_cluster_parser_unittest.cc (right): https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser_unittest.cc:1008: class OpusWebMClusterParserTest This looks like a good candidate for splitting out into a separate test file. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser_unittest.cc:1034: std::vector<uint8> BuildOpusPacket(int config, On 2015/01/29 22:14:20, chcunningham wrote: > I'm expecting this return to be RVO'd. Am I right? Is this good with > chromium/team style? This is fine with me for these tests. For production code, we'd usually encapsulate within something like ClusterBuilder, with an accessor that returns a reference or copy of the constructed blob of data. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser_unittest.cc:1117: // From Opus RFC. provide link please https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser_unittest.cc:1118: const int kNumPossibleOpusConfigs = 32; constants like these, we more often than not put into enums. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser_unittest.cc:1121: INSTANTIATE_TEST_CASE_P( On 2015/01/29 22:14:20, chcunningham wrote: > This was a fun exploration for me, but I think it may be overkill. Across these > 3 instantions we end up running over 3K tests! > > I'm considering moving the TEST_Ps to TEST_F and dealing with the > parameterization by simply calling a helper in both tests that generates an > array of the full set of opus configs. The tests would just iterate over that > list. 3k tests -> 2 tests. Thoughts? yeah, overkill. Please consider categories of tests (such as negative tests (that confirm expected parse failure), and the other edge cases like 0 duration, duration of packet exceeding spec'ed max 120ms, etc). Since you're reusing the same kOpusFrameDurationsMu as the real implementation, I'm not sure the value of exhaustive testing is worth the number of test increase. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser_unittest.cc:1134: // Lets VBR flag varry, as this affects TOC config for 2 frame packets. nit: varry spelling
Thanks Matt! Comments addressed. Some small open questions to consider. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... File media/formats/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:36: audio_config_(audio_config), On 2015/01/30 01:48:08, wolenetz wrote: > On 2015/01/29 22:14:20, chcunningham wrote: > > I'm not doing anything with this in ::Reset. Should I? > > ::Reset() is only for "let's parse another cluster from that track that we were > constructed with", so the audio_config_ from ctor should be left alone in > ::Reset(). Acknowledged. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:145: bool WebMClusterParser::TryGetEncodedAudioDuration(const uint8* data, On 2015/01/30 01:48:07, wolenetz wrote: > I suggest using a base::TimeDelta duration as retval and kNoTimestamp magic > value to indicate lack of success? This reduces # of params. caller should > DCHECK that the retval in that case is never otherwise negative, zero, or > kInfiniteDuration (which maps to negative, but inclusion in DCEHCK improves > readability). Done, but a question about asserts. I haven't yet added the assert for duration > 0. I notice in OnBlock that the existing code that sets the duration from BlockDuration allows that duraiton to be == 0 (check is >= 0). The assert > 0 for the encoded duration seems inconsistent with this existing check. Is the existing check wrong? Should we relax the assert you proposed to >= 0? While Opus can't give a duration of 0, it may be too strong to assume for this API which will one day serve Vorbis and potentially other codecs. I'm personally a fan of letting WebMClusterParser allow for zero duration packets - I'm not aware of this being in violation of WebM spec. If MSE chokes on this, can we handle it downstream in a centralized place? https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:155: bool WebMClusterParser::ReadOpusDuration(const uint8* data, On 2015/01/30 01:48:08, wolenetz wrote: > ditto on retval comment. Done. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:159: MEDIA_LOG(log_cb_) << "Invalid zero-byte Opus packet."; On 2015/01/30 20:46:12, wolenetz wrote: > On 2015/01/30 01:48:07, wolenetz wrote: > > Could it be possible that an append one-byte-at-a-time logs this on 0 byte > data > > first time parse is attempted, but not once another byte is appended? If so, > > need to protect against spurious log? > > nm. OnBlock is done on complete block, not partial. Acknowledged. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:178: MEDIA_LOG(log_cb_) << "Second byte missing from 'Code 3' Opus packet."; On 2015/01/30 20:46:12, wolenetz wrote: > On 2015/01/30 01:48:08, wolenetz wrote: > > ditto: protect against spurious log? > > ditto nm Acknowledged. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:190: int opusConfig = (data[0] & 0xf8) >> 3; On 2015/01/30 20:46:12, wolenetz wrote: > nit: put "magic" 0xf8 and others like that into an enum with clear naming to > help reader. Done, sort of. Did it right here in the .cc - header file has enough Opus mess I think. Also, rather than use an enum I just made some static const types. This allows me to save a little memory. WDYT? I can also do an enum right here in the cc function... https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:191: *duration = kOpusFrameDurationsMu[opusConfig] * frame_count; On 2015/01/30 20:46:12, wolenetz wrote: > On 2015/01/30 01:48:08, wolenetz wrote: > > ... > > And finally, I'd like to understand exactly how bits 0-4 in TOC map to the > "top > > five bits". This wording is very strange, leading me to question whether or > not > > the most-significant bit in an Opus bitstream is first or last in a byte. I > > would assume so, but still... > > The strange wording is clarified elsewhere in that rfc. Thanks for pointing that > out. Agree this is hard to read. I think I have an interpretation that makes it at least mostly consistent with itself... I think we have to de-tangle the notions of VBR frame count (M) and VBR frame byte length (noted in the 1|2 bytes before the frame). When I read "M MUST NOT be zero" then later see that VBR frames can have zero data, may take is that a frame with zero data is still a frame which is counted by M. Also, we know its impossible to describe a duration of 0ms within the Opus TOC byte. So I believe that frames with zero data still have a non-zero duration (which ends up just being the duration of the discontinuity). Basically, in the case where a VBR frame's data is dropped by transmission/encoder, the remaining parts of the packet are useful for you to know that the frame was dropped and what that frame's duration would have been. I don't think the parser should guard against these zero-data frames or change what's emitted. This is still a valid Opus packet and the decoder should handle it without issue. What this means for duration depends on how the decoder reacts. Does it heal the gap? Play silence? If so, we're fine. If it just plays the next frame and skips this duration, I guess the timeline may get just a little off. Do we worry about this? We could do crazy things like remove any mention of the frame from the packet and adjust the duration... but it seems like way too much effort and layer violating for what is hopefully a rare edge case. I vote we just emit the buffer and carry on. If we see Opus timeline bugs we can re-consider. But I bet this dropped frame problem is not specific to just Opus and seeing such bugs might motivate a decoder/architecture change instead of additional complexity in layer violating parser code. WDYT? https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:380: int64 encoded_duration = -1; On 2015/01/30 01:48:08, wolenetz wrote: > ditto: base::TimeDelta please Done. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:385: TryGetEncodedAudioDuration(data, size, &encoded_duration); On 2015/01/30 01:48:08, wolenetz wrote: > note the retval was ignored previously... not sure why it was there then. Done. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:386: } On 2015/01/30 01:48:08, wolenetz wrote: > nit: no {} necessary for one-line block here. I'm fine if you want it there for > readability though. It became a mutli-line block after adding the DVLOG. But also, my style is to always do the {}. I was once bitten by a nasty bug that came back to this... never again. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:386: } On 2015/01/30 01:48:08, wolenetz wrote: > I'd like to see at least DVLOG for audio block if there is a > BlockGroup->BlockDuration and an encoded duration that significantly mismatches. > Perhaps always DVLOG here for unenc audio, but at high verbosity level (like > DVLOG(3,...))? If significantly mismatching, media_log() could be appropriate to > inform webdevs that something's wrong with content or parser. Done. How does 10ms for a threshold sound? https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... File media/formats/webm/webm_cluster_parser.h (right): https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.h:32: // TODO(wolenetz/acolwell): Parse audio codebook to determine missing audio On 2015/01/30 01:48:08, wolenetz wrote: > nit: update bug comment :) (wolenetz/chcunningham), vorbis... Done. I pulled the TODO out of this comment and put the TODO you proposed inside the impl for TryGetEncodedAudioDuration. I think its a little more intuitive when placed there. For e.g., when I come back to implement Vorbis, I won't really be touching these estimation defaults at all, but I will be changing the impl of TryGetEncodedAudioDuration. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.h:39: // Opus packets encode the duration and other parameters in the 5 most On 2015/01/30 01:48:08, wolenetz wrote: > move to impl. drop last ',' Done. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.h:136: base::TimeDelta default_duration_; On 2015/01/30 01:48:08, wolenetz wrote: > nit: Hmm. it seems the comments for how estimation vs extraction vs default vs > hardcode are split up all over. Maybe document at top or have a helper method > that handles *all* the duration extraction/fallback estimation/hardcoding logic > and put the comments there and refer to that documentation here and in > |estimated_next_frame_duration|? This is getting harder and harder to read. I really pondered this (and we chatted). I think a centralized comment is only better if we can also centralize the duration calc/estimation code. Right now we have fragmentation across these 4 places... 1. ClusterParser::OnBlock 1. encoded duration 2. block duration 3. track default duration 2. Track::AddBuffer 1. set delta duration on *previous* buffer when needed 2. mark this buffer as needing delta if it doesn't have one 3. Track::ApplyDurationEstimateIfNeeded 1. Last buffer missing duration? Set from GetDurationEstimate 4. Track::GetDurationEstimate 1. first try's to use estimated_next_frame_duration (min) 2. fallback to hardcoded The conservative approach: Each of these 4 places has a good comment in the .h file. Perhaps the only thing that's missing is a comment in OnBlock impl that mentions how estimation is handled in end of cluster Scenario. I've added a little blurb there. A possibility for consolidation: We could move the calling of estimation code (3 & 4) into OnBlock, but only if we can know that the Block we're parsing is the last in the cluster. That info is known by the underlying list parser, which has a "done" state. Right now it only enters the "done" state *AFTER* OnBlock is called. *BUT* we could make it transition just before calling OnBlock. To make it less subtle, we could additionally add a flag to OnListEnd (which calls OnBlock, passing the flag along) to indicate that this list (Block) is the last in the cluster. What do you think? It would allow us to completely remove the ApplyDurationEstimateIfNeeded in the Get{Audio/Video}Buffers path. I say we go for it, but want you to call out the things I'm probably overlooking ;) https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.h:246: // and cannot be calculated when for Blocks at the end of a Cluster (The next On 2015/01/30 01:48:08, wolenetz wrote: > nits: grammar (when for), un-cap (The Done. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.h:251: bool TryGetEncodedAudioDuration(const uint8* data, int size, int64* duration); On 2015/01/30 01:48:08, wolenetz wrote: > What is bool retval? Now base::TimeDelta https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.h:252: // Reads Opus packet header to determine packet duration. On 2015/01/30 01:48:08, wolenetz wrote: > nit: insert blank link prior. Done. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.h:260: const AudioDecoderConfig& audio_config_; On 2015/01/30 01:48:08, wolenetz wrote: > Do we need the whole config? Or just some specific info like codec id? > Also, I don't like refs-as-members in general (and I don't see any usage of > refs-as-members elsewhere in chromium). If you need a non-copy of some class, > consider smart pointers. Lifetime, backing-store, etc issues... refs-as-members > sound like a path to eventual pain ;) Done. Now just taking in the codec. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... File media/formats/webm/webm_cluster_parser_unittest.cc (right): https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser_unittest.cc:1008: class OpusWebMClusterParserTest On 2015/01/30 20:46:12, wolenetz wrote: > This looks like a good candidate for splitting out into a separate test file. I split the Builder pieces into a separate file. With those gone, I feel like the opus test functions are hopefully small enough to stay here. If you'd prefer I can pull the tests out to a separate file as well, but it will require a larger refactoring of this test file to put things like BlockInfo and VerifyBuffers into common places. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser_unittest.cc:1034: std::vector<uint8> BuildOpusPacket(int config, On 2015/01/30 20:46:12, wolenetz wrote: > On 2015/01/29 22:14:20, chcunningham wrote: > > I'm expecting this return to be RVO'd. Am I right? Is this good with > > chromium/team style? > > This is fine with me for these tests. For production code, we'd usually > encapsulate within something like ClusterBuilder, with an accessor that returns > a reference or copy of the constructed blob of data. Acknowledged. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser_unittest.cc:1117: // From Opus RFC. On 2015/01/30 20:46:12, wolenetz wrote: > provide link please Done. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser_unittest.cc:1118: const int kNumPossibleOpusConfigs = 32; On 2015/01/30 20:46:12, wolenetz wrote: > constants like these, we more often than not put into enums. Done. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser_unittest.cc:1121: INSTANTIATE_TEST_CASE_P( On 2015/01/30 20:46:12, wolenetz wrote: > On 2015/01/29 22:14:20, chcunningham wrote: > > This was a fun exploration for me, but I think it may be overkill. Across > these > > 3 instantions we end up running over 3K tests! > > > > I'm considering moving the TEST_Ps to TEST_F and dealing with the > > parameterization by simply calling a helper in both tests that generates an > > array of the full set of opus configs. The tests would just iterate over that > > list. 3k tests -> 2 tests. Thoughts? > > yeah, overkill. Please consider categories of tests (such as negative tests > (that confirm expected parse failure), and the other edge cases like 0 duration, > duration of packet exceeding spec'ed max 120ms, etc). Since you're reusing the > same kOpusFrameDurationsMu as the real implementation, I'm not sure the value of > exhaustive testing is worth the number of test increase. Done, mostly. We chatted about not adding the negative tests, but instead adding a warning to the parser in cases of having > 120 ms (too many frames in the frame count byte). I've added that warning :) https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser_unittest.cc:1134: // Lets VBR flag varry, as this affects TOC config for 2 frame packets. On 2015/01/30 20:46:12, wolenetz wrote: > nit: varry spelling Done.
Thanks for updating. This is looking even better. Many nits and a few comments: https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... File media/formats/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:145: bool WebMClusterParser::TryGetEncodedAudioDuration(const uint8* data, On 2015/02/03 20:34:42, chcunningham wrote: > On 2015/01/30 01:48:07, wolenetz wrote: > > I suggest using a base::TimeDelta duration as retval and kNoTimestamp magic > > value to indicate lack of success? This reduces # of params. caller should > > DCHECK that the retval in that case is never otherwise negative, zero, or > > kInfiniteDuration (which maps to negative, but inclusion in DCEHCK improves > > readability). > > Done, but a question about asserts. I haven't yet added the assert for duration > > 0. I notice in OnBlock that the existing code that sets the duration from > BlockDuration allows that duraiton to be == 0 (check is >= 0). The assert > 0 > for the encoded duration seems inconsistent with this existing check. Is the > existing check wrong? Should we relax the assert you proposed to >= 0? While > Opus can't give a duration of 0, it may be too strong to assume for this API > which will one day serve Vorbis and potentially other codecs. > > I'm personally a fan of letting WebMClusterParser allow for zero duration > packets - I'm not aware of this being in violation of WebM spec. If MSE chokes > on this, can we handle it downstream in a centralized place? For now, yeah, let frame processor/etc deal with 0 duration coded frames. Targeted strengthening of the requirements may come in later CL(s). https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:190: int opusConfig = (data[0] & 0xf8) >> 3; On 2015/02/03 20:34:43, chcunningham wrote: > On 2015/01/30 20:46:12, wolenetz wrote: > > nit: put "magic" 0xf8 and others like that into an enum with clear naming to > > help reader. > > Done, sort of. Did it right here in the .cc - header file has enough Opus mess I > think. Also, rather than use an enum I just made some static const types. This > allows me to save a little memory. WDYT? I can also do an enum right here in the > cc function... I'm not sure how an enum consumes more mem that a static const. However, static const is fine with me for now. If vorbis CL splits out an abstraction for this interface, then enums in the concrete impl can be done at that time. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:191: *duration = kOpusFrameDurationsMu[opusConfig] * frame_count; On 2015/02/03 20:34:42, chcunningham wrote: > On 2015/01/30 20:46:12, wolenetz wrote: > > On 2015/01/30 01:48:08, wolenetz wrote: > > > ... > > > And finally, I'd like to understand exactly how bits 0-4 in TOC map to the > > "top > > > five bits". This wording is very strange, leading me to question whether or > > not > > > the most-significant bit in an Opus bitstream is first or last in a byte. I > > > would assume so, but still... > > > > The strange wording is clarified elsewhere in that rfc. Thanks for pointing > that > > out. > > Agree this is hard to read. I think I have an interpretation that makes it at > least mostly consistent with itself... > > I think we have to de-tangle the notions of VBR frame count (M) and VBR frame > byte length (noted in the 1|2 bytes before the frame). > > When I read "M MUST NOT be zero" then later see that VBR frames can have zero > data, may take is that a frame with zero data is still a frame which is counted > by M. Also, we know its impossible to describe a duration of 0ms within the Opus > TOC byte. So I believe that frames with zero data still have a non-zero duration > (which ends up just being the duration of the discontinuity). > > Basically, in the case where a VBR frame's data is dropped by > transmission/encoder, the remaining parts of the packet are useful for you to > know that the frame was dropped and what that frame's duration would have been. > > I don't think the parser should guard against these zero-data frames or change > what's emitted. This is still a valid Opus packet and the decoder should handle > it without issue. What this means for duration depends on how the decoder > reacts. Does it heal the gap? Play silence? If so, we're fine. If it just plays > the next frame and skips this duration, I guess the timeline may get just a > little off. Do we worry about this? We could do crazy things like remove any > mention of the frame from the packet and adjust the duration... but it seems > like way too much effort and layer violating for what is hopefully a rare edge > case. > > I vote we just emit the buffer and carry on. If we see Opus timeline bugs we can > re-consider. But I bet this dropped frame problem is not specific to just Opus > and seeing such bugs might motivate a decoder/architecture change instead of > additional complexity in layer violating parser code. WDYT? I agree. See my comment on current PS. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.cc:386: } On 2015/02/03 20:34:42, chcunningham wrote: > On 2015/01/30 01:48:08, wolenetz wrote: > > I'd like to see at least DVLOG for audio block if there is a > > BlockGroup->BlockDuration and an encoded duration that significantly > mismatches. > > Perhaps always DVLOG here for unenc audio, but at high verbosity level (like > > DVLOG(3,...))? If significantly mismatching, media_log() could be appropriate > to > > inform webdevs that something's wrong with content or parser. > > Done. How does 10ms for a threshold sound? See my comment on current PS. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... File media/formats/webm/webm_cluster_parser.h (right): https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser.h:136: base::TimeDelta default_duration_; On 2015/02/03 20:34:43, chcunningham wrote: > On 2015/01/30 01:48:08, wolenetz wrote: > > nit: Hmm. it seems the comments for how estimation vs extraction vs default vs > > hardcode are split up all over. Maybe document at top or have a helper method > > that handles *all* the duration extraction/fallback estimation/hardcoding > logic > > and put the comments there and refer to that documentation here and in > > |estimated_next_frame_duration|? This is getting harder and harder to read. > > I really pondered this (and we chatted). I think a centralized comment is only > better if we can also centralize the duration calc/estimation code. Right now we > have fragmentation across these 4 places... > > 1. ClusterParser::OnBlock > 1. encoded duration > 2. block duration > 3. track default duration > > 2. Track::AddBuffer > 1. set delta duration on *previous* buffer when needed > 2. mark this buffer as needing delta if it doesn't have one > > 3. Track::ApplyDurationEstimateIfNeeded > 1. Last buffer missing duration? Set from GetDurationEstimate > > 4. Track::GetDurationEstimate > 1. first try's to use estimated_next_frame_duration (min) > 2. fallback to hardcoded > > > The conservative approach: > Each of these 4 places has a good comment in the .h file. Perhaps the only thing > that's missing is a comment in OnBlock impl that mentions how estimation is > handled in end of cluster Scenario. I've added a little blurb there. > > A possibility for consolidation: > We could move the calling of estimation code (3 & 4) into OnBlock, but only if > we can know that the Block we're parsing is the last in the cluster. That info > is known by the underlying list parser, which has a "done" state. Right now it > only enters the "done" state *AFTER* OnBlock is called. *BUT* we could make it > transition just before calling OnBlock. To make it less subtle, we could > additionally add a flag to OnListEnd (which calls OnBlock, passing the flag > along) to indicate that this list (Block) is the last in the cluster. What do > you think? It would allow us to completely remove the > ApplyDurationEstimateIfNeeded in the Get{Audio/Video}Buffers path. I say we go > for it, but want you to call out the things I'm probably overlooking ;) Refactoring the estimation stuff to occur in OnBlock is too big for inclusion in this CL. Please file a P3 bug to track this potential clean-up. https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... File media/formats/webm/webm_cluster_parser_unittest.cc (right): https://codereview.chromium.org/883403002/diff/1/media/formats/webm/webm_clus... media/formats/webm/webm_cluster_parser_unittest.cc:1121: INSTANTIATE_TEST_CASE_P( On 2015/02/03 20:34:43, chcunningham wrote: > On 2015/01/30 20:46:12, wolenetz wrote: > > On 2015/01/29 22:14:20, chcunningham wrote: > > > This was a fun exploration for me, but I think it may be overkill. Across > > these > > > 3 instantions we end up running over 3K tests! > > > > > > I'm considering moving the TEST_Ps to TEST_F and dealing with the > > > parameterization by simply calling a helper in both tests that generates an > > > array of the full set of opus configs. The tests would just iterate over > that > > > list. 3k tests -> 2 tests. Thoughts? > > > > yeah, overkill. Please consider categories of tests (such as negative tests > > (that confirm expected parse failure), and the other edge cases like 0 > duration, > > duration of packet exceeding spec'ed max 120ms, etc). Since you're reusing the > > same kOpusFrameDurationsMu as the real implementation, I'm not sure the value > of > > exhaustive testing is worth the number of test increase. > > Done, mostly. We chatted about not adding the negative tests, but instead adding > a warning to the parser in cases of having > 120 ms (too many frames in the > frame count byte). I've added that warning :) Acknowledged. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/opus_... File media/formats/webm/opus_packet_builder.cc (right): https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.cc:18: scoped_ptr<OpusPacketInfo> BuildOpusPacket(int config, ISTM that this could be a constructor. Also, the data member could lazily be constructed in a .data() accessor. OpusPacketInfo could then be renamed to OpusPacket. This seems a little more aligned with existing test data abstractions like ClusterBuilder. WDYT? https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.cc:29: uint8 frame_count_byte; _t https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.cc:56: // Builds an exhaustive collection of Opus packet configurations. nit: move method comment to .h https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/opus_... File media/formats/webm/opus_packet_builder.h (right): https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. s/2012/2015/ https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.h:19: std::vector<uint8> data; _t (here and elsewhere, s/uint8/uint8_t/ and similar for 16 or whatever. See current chromium style guide for this.) As always, if modifying a file that uses older style, either fix older->newer or just be consistent with older. It looks on quick inspection that most/all of this CL touches files that didn't previously use a specific-width type. Note: I might not have commented on all instances, so please do the needed here. THanks! https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.h:29: std::vector<scoped_ptr<OpusPacketInfo>> BuildAllOpusPackets(); This is dangerous. See "I want to use an STL container to hold pointers. Can I use smart pointers?" at http://www.chromium.org/developers/smart-pointer-guidelines Use ScopedVector? Or just return a vector of raw pointers and clean up manually... https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... File media/formats/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:155: // TODO(wolenetz/chcunningham): Implement duration reading for Vorbis. nits: let's start a new or reference an existing crbug here please. Also, blank line after } and before full line comment is more common. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:162: // Masks for Opus TOC and Frame Count bytes. See http://goo.gl/2RmoxA ditto full URL... sorry https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:163: static const uint8 kTocConfigMask = 0xf8; ditto: _t https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:195: MEDIA_LOG(log_cb_) << "Illegal Opus packet frame_count: " << frame_count nit: s/frame_count:/frame count:/ https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:195: MEDIA_LOG(log_cb_) << "Illegal Opus packet frame_count: " << frame_count nit: comment that we are explicitly allowing these to pass through the check with just a log message for now at least. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:207: return base::TimeDelta::FromMicroseconds(kOpusFrameDurationsMu[opusConfig] * nit: kFrameCountMax is lower for longer frame sizes (see rfc: "...lower limits for longer frame sizes"). Consider adding log message specifically instead for just the return timedelta being too large or 0 (see rfc: "MUST NOT exceed 120 ms (or be 0)"). Take this comment modulo: I agree that we should emit the packet regardless and let decoder fuss about it. But we should also log (perhaps at most once per webm_cluster_parser instance) when extracted encoded packet duration >120ms or <=0 occurs. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:488: // Prefer encoded duration when available. This layering violatiaon is a nit: s/violatiaon/violation/ https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:488: // Prefer encoded duration when available. This layering violatiaon is a nit: s/when available/over BlockGroup->BlockDuration or TrackEntry->DefaultDuration when available/. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:492: // feasabile given flexibility of cluster ordering and MSE APIs. Duration nit: s/feasabile/feasible/ https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:493: // estimation may still apply in cases of encryption and unsupported codecs. nit: s/unsupported codecs/codecs for which we do not extract encoded block duration/ https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:495: // parsed. See ApplyDurationEstimateIfNeeded for more on estimation. nit: add () https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:502: << "Using encoded duartion " << encoded_duration.InSecondsF(); nit: s/duartion/duration/ https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:508: const auto kWarnDurationDiff = base::TimeDelta::FromMilliseconds(10); Hmmm. 10ms seems a little arbitrary (and big). Consider: Can we do something like: precalculate the microsecond granularity allowed by current timecodescale and warn if the encoded duration is >= 2x that granularity? (e.g. if granularity is 1ms, then warn if abs_diff(encoded vs blockgroup->blockduration) >= 2ms? I think that might lead to too many media_log if we don't throttle it. Also, some form of UMA to let us know the answers to something like "how many webm streams containing opus are there" and "of these, how many had at least one occurrence of >= 2x granularity mismatch of encoded vs blockgroup duration".. AND "of all webm streams containing opus, how many had at least once occurrence of >= 2x granularity mismatch of encoded vs intra-cluster-block-timecode-diff". If we could obtain a bucketized distribution of count of granularity mismatches, it would really help us understand the state of our assumptions about how frequently we play badly muxed webm w.r.t. duration. WDYT? This UMA stuff would be a separate CL, perhaps along with indicators like how often an encrypted webm stream had a cluster without a trailing blockgroup for every track. For now (this CL), to at least prevent spamming log, I recommend 2x granularity logic, and log if it's exceeded at most once per instance of webm_cluster_parser. Then the log messages would be much less spammy, and more informative. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:511: << "(" << block_duration_time_delta << ") " nit: Units. Is this in secondsF? (use .InSecondsF() ?) https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:512: << "differs signifcantly from encoded duration " nit:s/signifcantly/significantly/ https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:513: << "(" << encoded_duration << ")."; nit ditto: InSecondsF() https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... File media/formats/webm/webm_cluster_parser.h (right): https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.h:25: public: fyi: I just noticed this class has multiple public: and multiple private: sections. Is this necessary? (If not, seems a clean-up could be fodder for a separate CL.) https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.h:40: // http://goo.gl/2RmoxA I was wrong. Using full URL is more common. Also lets readers see real target before/without loading it. s/full/shortened/ URL here and in .h please. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.h:41: static const uint16 kOpusFrameDurationsMu[]; _t https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.h:242: // output. See implementation for supported codecs. nit: though private, would be nice to have retval documented here. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.h:243: base::TimeDelta TryGetEncodedAudioDuration(const uint8* data, int size); nit: const method? https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.h:245: // Reads Opus packet header to determine packet duration. nit: ditto retval documentation https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.h:246: base::TimeDelta ReadOpusDuration(const uint8* data, int size); nit: const method? https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... File media/formats/webm/webm_cluster_parser_unittest.cc (left): https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:942: ParseDegenerateClusterWithDefaultDurationsYieldsDefaultDurations) { Hmm. Do we no longer have DefaultDuration tested? Perhaps keep this by testing with a codec that indicates the data is encrypted (and therefore skips the attempt at extracting the encoded duration from the data, and falls back to TrackEntry->DefaultDuration parsing)? https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... File media/formats/webm/webm_cluster_parser_unittest.cc (right): https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:86: uint8 kDefaultBlockData[] = {0x00}; nit: space after { and before } https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:994: int block_count = arraysize(kBlockInfo); isn't this 1 always? https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:1020: int block_count = arraysize(block_infos); isn't this 1 always? https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:1027: block_infos[0].duration = packet_ptr->duration_ms; nit: I assume that the next line would fail without this line. Can you please add a line that does ASSERT_FALSE(VerifyBuffers(....)) before this line to confirm my assumption :) Note: Such change will likely require fixing VerifyBuffers to not have "EXPECT_EQ" on duration, but rather return false if duration mismatches.
Thanks Matt! https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/opus_... File media/formats/webm/opus_packet_builder.cc (right): https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.cc:18: scoped_ptr<OpusPacketInfo> BuildOpusPacket(int config, On 2015/02/03 22:47:01, wolenetz wrote: > ISTM that this could be a constructor. Also, the data member could lazily be > constructed in a .data() accessor. OpusPacketInfo could then be renamed to > OpusPacket. This seems a little more aligned with existing test data > abstractions like ClusterBuilder. WDYT? Done. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.cc:29: uint8 frame_count_byte; On 2015/02/03 22:47:01, wolenetz wrote: > _t Done. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.cc:56: // Builds an exhaustive collection of Opus packet configurations. On 2015/02/03 22:47:01, wolenetz wrote: > nit: move method comment to .h Done. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/opus_... File media/formats/webm/opus_packet_builder.h (right): https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2015/02/03 22:47:01, wolenetz wrote: > s/2012/2015/ Done. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.h:19: std::vector<uint8> data; On 2015/02/03 22:47:01, wolenetz wrote: > _t (here and elsewhere, s/uint8/uint8_t/ and similar for 16 or whatever. See > current chromium style guide for this.) As always, if modifying a file that uses > older style, either fix older->newer or just be consistent with older. It looks > on quick inspection that most/all of this CL touches files that didn't > previously use a specific-width type. > Note: I might not have commented on all instances, so please do the needed here. > THanks! Done. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.h:29: std::vector<scoped_ptr<OpusPacketInfo>> BuildAllOpusPackets(); On 2015/02/03 22:47:01, wolenetz wrote: > This is dangerous. See "I want to use an STL container to hold pointers. Can I > use smart pointers?" at > http://www.chromium.org/developers/smart-pointer-guidelines > > Use ScopedVector? Or just return a vector of raw pointers and clean up > manually... Done. ScopedVector! https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... File media/formats/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:155: // TODO(wolenetz/chcunningham): Implement duration reading for Vorbis. On 2015/02/03 22:47:02, wolenetz wrote: > nits: let's start a new or reference an existing crbug here please. Also, blank > line after } and before full line comment is more common. Done. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:162: // Masks for Opus TOC and Frame Count bytes. See http://goo.gl/2RmoxA On 2015/02/03 22:47:01, wolenetz wrote: > ditto full URL... sorry Done. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:163: static const uint8 kTocConfigMask = 0xf8; On 2015/02/03 22:47:01, wolenetz wrote: > ditto: _t Done. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:195: MEDIA_LOG(log_cb_) << "Illegal Opus packet frame_count: " << frame_count On 2015/02/03 22:47:01, wolenetz wrote: > nit: s/frame_count:/frame count:/ Done. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:195: MEDIA_LOG(log_cb_) << "Illegal Opus packet frame_count: " << frame_count On 2015/02/03 22:47:01, wolenetz wrote: > nit: comment that we are explicitly allowing these to pass through the check > with just a log message for now at least. Done. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:207: return base::TimeDelta::FromMicroseconds(kOpusFrameDurationsMu[opusConfig] * On 2015/02/03 22:47:01, wolenetz wrote: > nit: kFrameCountMax is lower for longer frame sizes (see rfc: "...lower limits > for longer frame sizes"). Consider adding log message specifically instead for > just the return timedelta being too large or 0 (see rfc: "MUST NOT exceed 120 ms > (or be 0)"). > Take this comment modulo: I agree that we should emit the packet regardless and > let decoder fuss about it. But we should also log (perhaps at most once per > webm_cluster_parser instance) when extracted encoded packet duration >120ms or > <=0 occurs. Done. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:488: // Prefer encoded duration when available. This layering violatiaon is a On 2015/02/03 22:47:01, wolenetz wrote: > nit: s/when available/over BlockGroup->BlockDuration or > TrackEntry->DefaultDuration when available/. Done. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:488: // Prefer encoded duration when available. This layering violatiaon is a On 2015/02/03 22:47:02, wolenetz wrote: > nit: s/violatiaon/violation/ Done. Oh, and I found a sublime text spell checker :) https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:492: // feasabile given flexibility of cluster ordering and MSE APIs. Duration On 2015/02/03 22:47:01, wolenetz wrote: > nit: s/feasabile/feasible/ Done. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:493: // estimation may still apply in cases of encryption and unsupported codecs. On 2015/02/03 22:47:01, wolenetz wrote: > nit: s/unsupported codecs/codecs for which we do not extract encoded block > duration/ Done. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:495: // parsed. See ApplyDurationEstimateIfNeeded for more on estimation. On 2015/02/03 22:47:01, wolenetz wrote: > nit: add () Done. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:502: << "Using encoded duartion " << encoded_duration.InSecondsF(); On 2015/02/03 22:47:01, wolenetz wrote: > nit: s/duartion/duration/ Done. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:508: const auto kWarnDurationDiff = base::TimeDelta::FromMilliseconds(10); On 2015/02/03 22:47:01, wolenetz wrote: > Hmmm. 10ms seems a little arbitrary (and big). > Consider: > Can we do something like: precalculate the microsecond granularity allowed by > current timecodescale and warn if the encoded duration is >= 2x that > granularity? (e.g. if granularity is 1ms, then warn if abs_diff(encoded vs > blockgroup->blockduration) >= 2ms? > I think that might lead to too many media_log if we don't throttle it. > Also, some form of UMA to let us know the answers to something like "how many > webm streams containing opus are there" and "of these, how many had at least one > occurrence of >= 2x granularity mismatch of encoded vs blockgroup duration".. > AND "of all webm streams containing opus, how many had at least once occurrence > of >= 2x granularity mismatch of encoded vs intra-cluster-block-timecode-diff". > If we could obtain a bucketized distribution of count of granularity mismatches, > it would really help us understand the state of our assumptions about how > frequently we play badly muxed webm w.r.t. duration. > WDYT? This UMA stuff would be a separate CL, perhaps along with indicators like > how often an encrypted webm stream had a cluster without a trailing blockgroup > for every track. > For now (this CL), to at least prevent spamming log, I recommend 2x granularity > logic, and log if it's exceeded at most once per instance of > webm_cluster_parser. Then the log messages would be much less spammy, and more > informative. Done. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:511: << "(" << block_duration_time_delta << ") " On 2015/02/03 22:47:01, wolenetz wrote: > nit: Units. Is this in secondsF? (use .InSecondsF() ?) It is in secondsF. I can be explicit, but I think its probably ok as is. I can't see anyone changing that default without taking the proper precautions. LMK if you still want explicit. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:512: << "differs signifcantly from encoded duration " On 2015/02/03 22:47:01, wolenetz wrote: > nit:s/signifcantly/significantly/ Done. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:513: << "(" << encoded_duration << ")."; On 2015/02/03 22:47:01, wolenetz wrote: > nit ditto: InSecondsF() See other reply https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... File media/formats/webm/webm_cluster_parser.h (right): https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.h:25: public: On 2015/02/03 22:47:02, wolenetz wrote: > fyi: I just noticed this class has multiple public: and multiple private: > sections. Is this necessary? (If not, seems a clean-up could be fodder for a > separate CL.) I don't think its needed. Happy to clean up in later cl. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.h:40: // http://goo.gl/2RmoxA On 2015/02/03 22:47:02, wolenetz wrote: > I was wrong. Using full URL is more common. Also lets readers see real target > before/without loading it. s/full/shortened/ URL here and in .h please. Done. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.h:41: static const uint16 kOpusFrameDurationsMu[]; On 2015/02/03 22:47:02, wolenetz wrote: > _t Done. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.h:242: // output. See implementation for supported codecs. On 2015/02/03 22:47:02, wolenetz wrote: > nit: though private, would be nice to have retval documented here. Done. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.h:243: base::TimeDelta TryGetEncodedAudioDuration(const uint8* data, int size); On 2015/02/03 22:47:02, wolenetz wrote: > nit: const method? We chatted. The MEDIA_LOGS break const-ness :( https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.h:245: // Reads Opus packet header to determine packet duration. On 2015/02/03 22:47:02, wolenetz wrote: > nit: ditto retval documentation Done. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.h:246: base::TimeDelta ReadOpusDuration(const uint8* data, int size); On 2015/02/03 22:47:02, wolenetz wrote: > nit: const method? Same issue with MEDIA_LOG https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... File media/formats/webm/webm_cluster_parser_unittest.cc (left): https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:942: ParseDegenerateClusterWithDefaultDurationsYieldsDefaultDurations) { On 2015/02/03 22:47:02, wolenetz wrote: > Hmm. Do we no longer have DefaultDuration tested? Perhaps keep this by testing > with a codec that indicates the data is encrypted (and therefore skips the > attempt at extracting the encoded duration from the data, and falls back to > TrackEntry->DefaultDuration parsing)? Done with tweaks. As you know, the preference of duration sources is: 1. encoded 2. BlockDuration 3. track DefaultDuration 4. estimation... I didn't mean to remove this test. I think it covers well the fallback from #2 to #3, so I've re-added it as-is. For the encryption proposal, I've added a new test with encrypted opus data that should fallback to BlockDuration. I think this is good to cover the fallback from #1 to #2 and is more direct than checking for #1 -> #3. I think with this, we're pretty well covered. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... File media/formats/webm/webm_cluster_parser_unittest.cc (right): https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:86: uint8 kDefaultBlockData[] = {0x00}; On 2015/02/03 22:47:02, wolenetz wrote: > nit: space after { and before } Done. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:994: int block_count = arraysize(kBlockInfo); On 2015/02/03 22:47:02, wolenetz wrote: > isn't this 1 always? Yeah, the whole file is silly this way. Would you support changing the whole file? I'm flexible, I just want to be consistent. Changing to literals seems aligned with the wisdom of this TotT, but its really minor... http://googletesting.blogspot.com/2014/07/testing-on-toilet-dont-put-logic-in... https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:1020: int block_count = arraysize(block_infos); On 2015/02/03 22:47:02, wolenetz wrote: > isn't this 1 always? See other reply https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:1027: block_infos[0].duration = packet_ptr->duration_ms; On 2015/02/03 22:47:02, wolenetz wrote: > nit: I assume that the next line would fail without this line. Can you please > add a line that does ASSERT_FALSE(VerifyBuffers(....)) before this line to > confirm my assumption :) > Note: Such change will likely require fixing VerifyBuffers to not have > "EXPECT_EQ" on duration, but rather return false if duration mismatches. I think this test is quite strong. I can't think of way to "fake" passing outside of making BlockDuraiton = EncodedDuration, which we are certain don't match because we set them to be different right there in the test... int block_duration_ms = packet_ptr->duration_ms() + 10; IMHO Adding the proposed line might help readability, but changing the return of the VerifyBuffers to specialize for duration smells funny - the return signal is too narrow given all the things the function verifies. I considered calling out the buffer duration explicitly like so... const scoped_refptr<media::StreamParserBuffer> audio_buffer = parser_->GetAudioBuffers()[0]; EXPECT_NE(block_duration_ms, audio_buffer->duration().InMillisecondsF()); EXPECT_EQ(packet_ptr->duration_ms(), audio_buffer->duration().InMillisecondsF()); But to me it might actually be more confusing as long as I'm still changing the duration in BlockInfo and calling VerifyBuffers after that (which is worth doing given all the other things it verifies).
The CQ bit was checked by chcunningham@chromium.org
The CQ bit was unchecked by chcunningham@chromium.org
The CQ bit was checked by chcunningham@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/883403002/40001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Just testing this out. Failure expected :) On Thu Feb 05 2015 at 2:57:05 PM <commit-bot@chromium.org> wrote: > No LGTM from a valid reviewer yet. Only full committers are accepted. > Even if an LGTM may have been provided, it was from a non-committer, > _not_ a full super star committer. > See http://www.chromium.org/getting-involved/become-a-committer > Note that this has nothing to do with OWNERS files. > > https://codereview.chromium.org/883403002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/02/05 22:58:00, chromium-reviews wrote: > Just testing this out. Failure expected :) > > On Thu Feb 05 2015 at 2:57:05 PM <mailto:commit-bot@chromium.org> wrote: > > > No LGTM from a valid reviewer yet. Only full committers are accepted. > > Even if an LGTM may have been provided, it was from a non-committer, > > _not_ a full super star committer. > > See http://www.chromium.org/getting-involved/become-a-committer > > Note that this has nothing to do with OWNERS files. > > > > https://codereview.chromium.org/883403002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Please file a crbug though, since you really aren't chromium-reviews, chcunningham :)
Looking pretty good. Some more nits and a few comments. Also, I'll do a 'git cl try' of your PS4 shortly since your trybot permissions might not yet have percolated through the infra. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... File media/formats/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:488: // Prefer encoded duration when available. This layering violatiaon is a On 2015/02/05 02:48:21, chcunningham wrote: > On 2015/02/03 22:47:02, wolenetz wrote: > > nit: s/violatiaon/violation/ > > Done. Oh, and I found a sublime text spell checker :) Please share link offline with me :) https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:511: << "(" << block_duration_time_delta << ") " On 2015/02/05 02:48:21, chcunningham wrote: > On 2015/02/03 22:47:01, wolenetz wrote: > > nit: Units. Is this in secondsF? (use .InSecondsF() ?) > > It is in secondsF. I can be explicit, but I think its probably ok as is. I can't > see anyone changing that default without taking the proper precautions. LMK if > you still want explicit. Oh. I didn't realize that. And it comes with nice "s" appended, too. Fine as-is then :) https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:513: << "(" << encoded_duration << ")."; On 2015/02/05 02:48:21, chcunningham wrote: > On 2015/02/03 22:47:01, wolenetz wrote: > > nit ditto: InSecondsF() > > See other reply Acknowledged. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... File media/formats/webm/webm_cluster_parser.h (right): https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.h:25: public: On 2015/02/05 02:48:22, chcunningham wrote: > On 2015/02/03 22:47:02, wolenetz wrote: > > fyi: I just noticed this class has multiple public: and multiple private: > > sections. Is this necessary? (If not, seems a clean-up could be fodder for a > > separate CL.) > > I don't think its needed. Happy to clean up in later cl. Acknowledged. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.h:243: base::TimeDelta TryGetEncodedAudioDuration(const uint8* data, int size); On 2015/02/05 02:48:22, chcunningham wrote: > On 2015/02/03 22:47:02, wolenetz wrote: > > nit: const method? > > We chatted. The MEDIA_LOGS break const-ness :( Acknowledged. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.h:246: base::TimeDelta ReadOpusDuration(const uint8* data, int size); On 2015/02/05 02:48:22, chcunningham wrote: > On 2015/02/03 22:47:02, wolenetz wrote: > > nit: const method? > > Same issue with MEDIA_LOG Acknowledged. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... File media/formats/webm/webm_cluster_parser_unittest.cc (right): https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:994: int block_count = arraysize(kBlockInfo); On 2015/02/05 02:48:22, chcunningham wrote: > On 2015/02/03 22:47:02, wolenetz wrote: > > isn't this 1 always? > > Yeah, the whole file is silly this way. Would you support changing the whole > file? I'm flexible, I just want to be consistent. Changing to literals seems > aligned with the wisdom of this TotT, but its really minor... > > http://googletesting.blogspot.com/2014/07/testing-on-toilet-dont-put-logic-in... tott aside, let's keep consistent. Maybe a helper would work better. I'm fine with this as-is. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:1020: int block_count = arraysize(block_infos); On 2015/02/05 02:48:22, chcunningham wrote: > On 2015/02/03 22:47:02, wolenetz wrote: > > isn't this 1 always? > > See other reply Acknowledged. https://codereview.chromium.org/883403002/diff/20001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:1027: block_infos[0].duration = packet_ptr->duration_ms; On 2015/02/05 02:48:22, chcunningham wrote: > On 2015/02/03 22:47:02, wolenetz wrote: > > nit: I assume that the next line would fail without this line. Can you please > > add a line that does ASSERT_FALSE(VerifyBuffers(....)) before this line to > > confirm my assumption :) > > Note: Such change will likely require fixing VerifyBuffers to not have > > "EXPECT_EQ" on duration, but rather return false if duration mismatches. > > I think this test is quite strong. I can't think of way to "fake" passing > outside of making BlockDuraiton = EncodedDuration, which we are certain don't > match because we set them to be different right there in the test... > > int block_duration_ms = packet_ptr->duration_ms() + 10; > > IMHO Adding the proposed line might help readability, but changing the return of > the VerifyBuffers to specialize for duration smells funny - the return signal is > too narrow given all the things the function verifies. > > I considered calling out the buffer duration explicitly like so... > > const scoped_refptr<media::StreamParserBuffer> audio_buffer > = parser_->GetAudioBuffers()[0]; > EXPECT_NE(block_duration_ms, audio_buffer->duration().InMillisecondsF()); > EXPECT_EQ(packet_ptr->duration_ms(), > audio_buffer->duration().InMillisecondsF()); > > But to me it might actually be more confusing as long as I'm still changing the > duration in BlockInfo and calling VerifyBuffers after that (which is worth doing > given all the other things it verifies). The test is strong. I think changing the EXPECT_EQ to return DVLOG + return false in VerifyBuffers() would be good regardless, since ALL calls to VerifyBuffers() previously asserted the retval was true. This enables us to do a negative test to enhance readability here. Please also add some verification that at least some nonzero number of iterations of this inner loop actually occur. https://codereview.chromium.org/883403002/diff/40001/media/base/media_log.h File media/base/media_log.h (right): https://codereview.chromium.org/883403002/diff/40001/media/base/media_log.h#n... media/base/media_log.h:43: LAZY_STREAM(MEDIA_LOG(log_cb), count <= max && count++) macros-r-fun :) s/<=/</. Also, caller might have funky count and max. Consider count=-1, max=1. count++ would be 0, resulting in false evaluation for the macro's |condition| on first evaluation. Also, wrap the params inside () to make evaluation order explicit. Therefore, I think a better |condition| expression would be (count) < (max) && ((count++) || 1). I think it is understood by both users and implementors of LAZY_STREAM that |condition| should only be evaluated once. However, I could be wrong. In that case, the side-effect of incrementing count for every evaluation of |condition| within LAZY_STREAM (when count is still < max) would result in unexpected behavior if LAZY_STREAM implementation change is modified to perhaps evaluate |condition| N times, where N is not always 1. I'm not sure the best way to account for this, but a starting point would be to add to LAZY_STREAM's implementation documentation (and get appropriate OWNER l*tm) that |condition| must only be evaluated once within the implementation, as part of this CL. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/opus_... File media/formats/webm/opus_packet_builder.cc (right): https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.cc:13: OpusPacket::OpusPacket(int config, int frame_count, bool is_VBR) { narrow param types (uint8_t config and frame_count?) likewise, narrow param types of local (frame_count_code) https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.cc:14: duration_ms_ = frame_count * nit: protect borders (DCHECK fail for invalid config, frame_count, is_VBR). Or are invalid OpusPacket creations allowed? https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.cc:34: // For code 3 packets, the number of frames is signaled in the "frame For code 2 packets, the compressed length.. is also signaled in the frame count byte(or 2 bytes). It looks like we're assuming the special "No frame"/DTX/Lost packet condition for code 2, and somehow subdividing the one byte of frame data (0x0) in code 1 and 3 into multiple frames. Document these assumptions please. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.cc:59: ScopedVector<OpusPacket> BuildAllOpusPackets() { Are there illegal Opus Packet config + frame_count combos? (such as 48 frames, each with a config indicating a duration that in aggregate exceeds 120ms) If so, comment that we create these, and the parser is currently expected to allow them (with a warning). https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.cc:66: bool is_VBR = false; nit: is_VBR can be inlined in each of the two calls explicitly. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/opus_... File media/formats/webm/opus_packet_builder.h (right): https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.h:27: }; DISALLOW_COPY_AND_ASSIGN(OpusPacket); in private: section of class. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... File media/formats/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:182: << "Invalid zero-byte Opus packet."; nit: add ", so demuxed block duration may be imprecise." https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:202: << "Second byte missing from 'Code 3' Opus packet."; nit: ditto ", so demuxed"... https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:210: << "Illegal opus packet with frame count zero."; nit: ditto ", so demuxed"... Also mention "'Code 3'"? https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:217: << "Unexpected Opus frame count type: " << frame_count_type; nit: ditto ", so demuxed"... https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:221: int opusConfig = (data[0] & kTocConfigMask) >> 3; nit: harden the code against regressions by DCHECK(frame_count > 0) here please. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:222: base::TimeDelta duration = base::TimeDelta::FromMicroseconds( nit: harden the code against maintenance regressions by bounds-checking opusConfig vs the actual array size of kOpusFrameDurationsMu (in CHECK here, not DCHECK). https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:231: << "Should be no greater than " << kPacketDurationMax; nit: clarify log msg slightly to indicate there's no parse err. like maybe prepending "Warning, demuxed Opus packet with encoded duration:" ... https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:527: // we do not extract encoded duration. Estimates are applied at the end of not all estimates. some are inter-block, intra-cluster, block timecode deltas, which could be imprecise based on webm timecodescale. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... File media/formats/webm/webm_cluster_parser_unittest.cc (right): https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:57: float duration; In[Milli]SecondsF returns a double. why float elsewhere in this CL? https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:1027: for (const auto& packet_ptr : BuildAllOpusPackets()) { s/const auto& packet_ptr/const auto* packet/ per Range-Based For Loops at https://chromium-cpp.appspot.com/ https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:1053: // opus duration is usually preferred but cannot be known when encrypted. nit: s/opus/Opus/ https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:1055: // Non-empty dummy value for signals encryption is active for audio. nit: clean up wording. "for" what? https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:1069: kEncryptedFrame, // Encrypted frame data How about we try feeding it a "clear" OpusPacket so that we can observe more explicitly that the parser is using the blockgroup->blockduration and not the "clear frame"'s encoded duration. Just make sure that the OpusPacket's duration_ms() is significantly different from the BlockGroup->BlockDuration before parsing and verifying.
https://codereview.chromium.org/883403002/diff/40001/media/base/media_log.h File media/base/media_log.h (right): https://codereview.chromium.org/883403002/diff/40001/media/base/media_log.h#n... media/base/media_log.h:43: LAZY_STREAM(MEDIA_LOG(log_cb), count <= max && count++) On 2015/02/05 23:04:59, wolenetz wrote: > macros-r-fun :) > > s/<=/</. Also, caller might have funky count and max. Consider count=-1, max=1. > count++ would be 0, resulting in false evaluation for the macro's |condition| on > first evaluation. > Also, wrap the params inside () to make evaluation order explicit. > > Therefore, I think a better |condition| expression would be (count) < (max) && > ((count++) || 1). > > I think it is understood by both users and implementors of LAZY_STREAM that > |condition| should only be evaluated once. However, I could be wrong. In that > case, the side-effect of incrementing count for every evaluation of |condition| > within LAZY_STREAM (when count is still < max) would result in unexpected > behavior if LAZY_STREAM implementation change is modified to perhaps evaluate > |condition| N times, where N is not always 1. I'm not sure the best way to > account for this, but a starting point would be to add to LAZY_STREAM's > implementation documentation (and get appropriate OWNER l*tm) that |condition| > must only be evaluated once within the implementation, as part of this CL. Done. Adding akalin@chromium.org to review this change to LAZY_STREAM documentation. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/opus_... File media/formats/webm/opus_packet_builder.cc (right): https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.cc:13: OpusPacket::OpusPacket(int config, int frame_count, bool is_VBR) { On 2015/02/05 23:05:00, wolenetz wrote: > narrow param types (uint8_t config and frame_count?) > > likewise, narrow param types of local (frame_count_code) Done. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.cc:14: duration_ms_ = frame_count * On 2015/02/05 23:04:59, wolenetz wrote: > nit: protect borders (DCHECK fail for invalid config, frame_count, is_VBR). Or > are invalid OpusPacket creations allowed? Done. Decided to not allow invalid packets at this time. Added DCHECKS https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.cc:34: // For code 3 packets, the number of frames is signaled in the "frame On 2015/02/05 23:04:59, wolenetz wrote: > For code 2 packets, the compressed length.. is also signaled in the frame count > byte(or 2 bytes). It looks like we're assuming the special "No frame"/DTX/Lost > packet condition for code 2, and somehow subdividing the one byte of frame data > (0x0) in code 1 and 3 into multiple frames. Document these assumptions please. Clarified the comment that we're not really attempting to conform to the bit layout beyond the toc and framecount bytes. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.cc:59: ScopedVector<OpusPacket> BuildAllOpusPackets() { On 2015/02/05 23:04:59, wolenetz wrote: > Are there illegal Opus Packet config + frame_count combos? (such as 48 frames, > each with a config indicating a duration that in aggregate exceeds 120ms) If so, > comment that we create these, and the parser is currently expected to allow them > (with a warning). Done. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.cc:66: bool is_VBR = false; On 2015/02/05 23:04:59, wolenetz wrote: > nit: is_VBR can be inlined in each of the two calls explicitly. I like this for self documenting the meaning of the parameter. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/opus_... File media/formats/webm/opus_packet_builder.h (right): https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.h:27: }; On 2015/02/05 23:05:00, wolenetz wrote: > DISALLOW_COPY_AND_ASSIGN(OpusPacket); in private: section of class. Done. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... File media/formats/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:182: << "Invalid zero-byte Opus packet."; On 2015/02/05 23:05:00, wolenetz wrote: > nit: add ", so demuxed block duration may be imprecise." Done. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:202: << "Second byte missing from 'Code 3' Opus packet."; On 2015/02/05 23:05:00, wolenetz wrote: > nit: ditto ", so demuxed"... Done. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:210: << "Illegal opus packet with frame count zero."; On 2015/02/05 23:05:00, wolenetz wrote: > nit: ditto ", so demuxed"... Also mention "'Code 3'"? Done. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:217: << "Unexpected Opus frame count type: " << frame_count_type; On 2015/02/05 23:05:00, wolenetz wrote: > nit: ditto ", so demuxed"... Done. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:221: int opusConfig = (data[0] & kTocConfigMask) >> 3; On 2015/02/05 23:05:00, wolenetz wrote: > nit: harden the code against regressions by DCHECK(frame_count > 0) here please. Done. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:222: base::TimeDelta duration = base::TimeDelta::FromMicroseconds( On 2015/02/05 23:05:00, wolenetz wrote: > nit: harden the code against maintenance regressions by bounds-checking > opusConfig vs the actual array size of kOpusFrameDurationsMu (in CHECK here, not > DCHECK). Done. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:231: << "Should be no greater than " << kPacketDurationMax; On 2015/02/05 23:05:00, wolenetz wrote: > nit: clarify log msg slightly to indicate there's no parse err. like maybe > prepending "Warning, demuxed Opus packet with encoded duration:" ... Done. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:527: // we do not extract encoded duration. Estimates are applied at the end of On 2015/02/05 23:05:00, wolenetz wrote: > not all estimates. some are inter-block, intra-cluster, block timecode deltas, > which could be imprecise based on webm timecodescale. Done. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... File media/formats/webm/webm_cluster_parser_unittest.cc (right): https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:57: float duration; On 2015/02/05 23:05:00, wolenetz wrote: > In[Milli]SecondsF returns a double. why float elsewhere in this CL? Done. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:1027: for (const auto& packet_ptr : BuildAllOpusPackets()) { On 2015/02/05 23:05:00, wolenetz wrote: > s/const auto& packet_ptr/const auto* packet/ per Range-Based For Loops at > https://chromium-cpp.appspot.com/ Done. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:1053: // opus duration is usually preferred but cannot be known when encrypted. On 2015/02/05 23:05:00, wolenetz wrote: > nit: s/opus/Opus/ Done. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:1055: // Non-empty dummy value for signals encryption is active for audio. On 2015/02/05 23:05:00, wolenetz wrote: > nit: clean up wording. "for" what? Done. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:1069: kEncryptedFrame, // Encrypted frame data On 2015/02/05 23:05:00, wolenetz wrote: > How about we try feeding it a "clear" OpusPacket so that we can observe more > explicitly that the parser is using the blockgroup->blockduration and not the > "clear frame"'s encoded duration. Just make sure that the OpusPacket's > duration_ms() is significantly different from the BlockGroup->BlockDuration > before parsing and verifying. We chatted and ultimately decided to just use something that isnt the default duration... but I can't recall why that sounded good. It seems silly now. The duration we're using isn't known to the parser outside of us providing via block duration in this way. This default duration differs from the hard-coded estimation defaults used by the parser. I looked into the encryption spec, and indeed you can have un-encrypted blocks among encrypted blocks. Relevant quote: "Encrypted bit. If set the Block MUST contain an IV immediately followed by an encrypted frame. If not set the Block MUST NOT include an IV and the frame MUST be unencrypted. The unencrypted frame MUST immediately follow the Signal Byte." So we can consider digging into unencrypted frames among the encrypted onces to get a precise duration. Worth it? Eh... parser complexity increase is moderate. Expected gain is pretty slight. I doubt we often see unecrypted frames among encrypted. WDYT?
chcunningham@chromium.org changed reviewers: + akalin@chromium.org
Hey akalin, I made a very minor change to some documentation in logging.h. Just adding you for owners/approval. Basically, we're just being OCD in documenting current behavior with LAZY_STREAM, that the condition be evaluated once and only once. This affords us a minor boost in confidence as we use LAZY_STREAM in the media folder. Thanks, Chris
akalin@ left google ~year ago, you'll need to find another base reviewer :)
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
(That said you should probably split the base change out)
dalecurtis@chromium.org changed reviewers: - dalecurtis@chromium.org
On 2015/02/06 03:23:02, chcunningham wrote: > Hey akalin, > > I made a very minor change to some documentation in logging.h. Just adding you > for owners/approval. > > Basically, we're just being OCD in documenting current behavior with > LAZY_STREAM, that the condition be evaluated once and only once. This affords us > a minor boost in confidence as we use LAZY_STREAM in the media folder. > > Thanks, > Chris Chris, fyi - akalin committed the most recent LAZY_STREAM stuff, but is not an OWNER of base/logging.h. You'll need an OWNER to l*tm the change to that file.
On 2015/02/06 19:05:27, wolenetz wrote: > On 2015/02/06 03:23:02, chcunningham wrote: > > Hey akalin, > > > > I made a very minor change to some documentation in logging.h. Just adding you > > for owners/approval. > > > > Basically, we're just being OCD in documenting current behavior with > > LAZY_STREAM, that the condition be evaluated once and only once. This affords > us > > a minor boost in confidence as we use LAZY_STREAM in the media folder. > > > > Thanks, > > Chris > > Chris, fyi - akalin committed the most recent LAZY_STREAM stuff, but is not an > OWNER of base/logging.h. You'll need an OWNER to l*tm the change to that file. ah - and I see I duplicated much of Dale's reply... sorry :)
LGTM % nits! Thanks for putting this together. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/opus_... File media/formats/webm/opus_packet_builder.cc (right): https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.cc:66: bool is_VBR = false; On 2015/02/06 03:20:09, chcunningham wrote: > On 2015/02/05 23:04:59, wolenetz wrote: > > nit: is_VBR can be inlined in each of the two calls explicitly. > > I like this for self documenting the meaning of the parameter. Acknowledged. https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... File media/formats/webm/webm_cluster_parser_unittest.cc (right): https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:1069: kEncryptedFrame, // Encrypted frame data On 2015/02/06 03:20:10, chcunningham wrote: > On 2015/02/05 23:05:00, wolenetz wrote: > > How about we try feeding it a "clear" OpusPacket so that we can observe more > > explicitly that the parser is using the blockgroup->blockduration and not the > > "clear frame"'s encoded duration. Just make sure that the OpusPacket's > > duration_ms() is significantly different from the BlockGroup->BlockDuration > > before parsing and verifying. > > We chatted and ultimately decided to just use something that isnt the default > duration... but I can't recall why that sounded good. It seems silly now. The > duration we're using isn't known to the parser outside of us providing via block > duration in this way. This default duration differs from the hard-coded > estimation defaults used by the parser. > > I looked into the encryption spec, and indeed you can have un-encrypted blocks > among encrypted blocks. Relevant quote: > > "Encrypted bit. If set the Block MUST contain an IV immediately followed by an > encrypted frame. If not set the Block MUST NOT include an IV and the frame MUST > be unencrypted. The unencrypted frame MUST immediately follow the Signal Byte." > > So we can consider digging into unencrypted frames among the encrypted onces to > get a precise duration. Worth it? Eh... parser complexity increase is moderate. > Expected gain is pretty slight. I doubt we often see unecrypted frames among > encrypted. WDYT? Extracting encoded duration from an encrypted track containing unencrypted blocks can be saved for a later CL. I suggest you comment on this edge case in the bug; and maybe file a separate dependent bug (cc'ing fgalligan@, and reference it here? https://codereview.chromium.org/883403002/diff/80001/media/formats/webm/opus_... File media/formats/webm/opus_packet_builder.h (right): https://codereview.chromium.org/883403002/diff/80001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.h:25: DISALLOW_COPY_AND_ASSIGN(OpusPacket); nit: move to end of private section (see Declaration Order in http://google-styleguide.googlecode.com/svn/trunk/cppguide.html and every place we use these macros in Chromium.) https://codereview.chromium.org/883403002/diff/80001/media/formats/webm/webm_... File media/formats/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/883403002/diff/80001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:212: << "Illegal 'Code 3' opus packet with frame count zero; demuxed " nit: s/opus/Opus/ https://codereview.chromium.org/883403002/diff/80001/media/formats/webm/webm_... File media/formats/webm/webm_cluster_parser_unittest.cc (right): https://codereview.chromium.org/883403002/diff/80001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:1027: for (const auto* packet_ptr : BuildAllOpusPackets()) { nit: ASSERT(number of times this loop body executes is more than 0) please (if not some more precise prediction of the cardinality of BuildAllOpusPackets() retval).
New patchsets have been uploaded after l-g-t-m from wolenetz@chromium.org
chcunningham@chromium.org changed reviewers: - akalin@chromium.org
Thanks Matt https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... File media/formats/webm/webm_cluster_parser_unittest.cc (right): https://codereview.chromium.org/883403002/diff/40001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:1069: kEncryptedFrame, // Encrypted frame data On 2015/02/06 19:48:01, wolenetz wrote: > On 2015/02/06 03:20:10, chcunningham wrote: > > On 2015/02/05 23:05:00, wolenetz wrote: > > > How about we try feeding it a "clear" OpusPacket so that we can observe more > > > explicitly that the parser is using the blockgroup->blockduration and not > the > > > "clear frame"'s encoded duration. Just make sure that the OpusPacket's > > > duration_ms() is significantly different from the BlockGroup->BlockDuration > > > before parsing and verifying. > > > > We chatted and ultimately decided to just use something that isnt the default > > duration... but I can't recall why that sounded good. It seems silly now. The > > duration we're using isn't known to the parser outside of us providing via > block > > duration in this way. This default duration differs from the hard-coded > > estimation defaults used by the parser. > > > > I looked into the encryption spec, and indeed you can have un-encrypted blocks > > among encrypted blocks. Relevant quote: > > > > "Encrypted bit. If set the Block MUST contain an IV immediately followed by an > > encrypted frame. If not set the Block MUST NOT include an IV and the frame > MUST > > be unencrypted. The unencrypted frame MUST immediately follow the Signal > Byte." > > > > So we can consider digging into unencrypted frames among the encrypted onces > to > > get a precise duration. Worth it? Eh... parser complexity increase is > moderate. > > Expected gain is pretty slight. I doubt we often see unecrypted frames among > > encrypted. WDYT? > > Extracting encoded duration from an encrypted track containing unencrypted > blocks can be saved for a later CL. I suggest you comment on this edge case in > the bug; and maybe file a separate dependent bug (cc'ing fgalligan@, and > reference it here? Done. https://codereview.chromium.org/883403002/diff/80001/media/formats/webm/opus_... File media/formats/webm/opus_packet_builder.h (right): https://codereview.chromium.org/883403002/diff/80001/media/formats/webm/opus_... media/formats/webm/opus_packet_builder.h:25: DISALLOW_COPY_AND_ASSIGN(OpusPacket); On 2015/02/06 19:48:01, wolenetz wrote: > nit: move to end of private section (see Declaration Order in > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html and every place > we use these macros in Chromium.) Done. https://codereview.chromium.org/883403002/diff/80001/media/formats/webm/webm_... File media/formats/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/883403002/diff/80001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser.cc:212: << "Illegal 'Code 3' opus packet with frame count zero; demuxed " On 2015/02/06 19:48:01, wolenetz wrote: > nit: s/opus/Opus/ Done. https://codereview.chromium.org/883403002/diff/80001/media/formats/webm/webm_... File media/formats/webm/webm_cluster_parser_unittest.cc (right): https://codereview.chromium.org/883403002/diff/80001/media/formats/webm/webm_... media/formats/webm/webm_cluster_parser_unittest.cc:1027: for (const auto* packet_ptr : BuildAllOpusPackets()) { On 2015/02/06 19:48:01, wolenetz wrote: > nit: ASSERT(number of times this loop body executes is more than 0) please (if > not some more precise prediction of the cardinality of BuildAllOpusPackets() > retval). Done.
The CQ bit was checked by chcunningham@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/883403002/100001
The CQ bit was unchecked by chcunningham@chromium.org
please fix before CQ: https://codereview.chromium.org/883403002/diff/100001/media/formats/webm/webm... File media/formats/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/883403002/diff/100001/media/formats/webm/webm... media/formats/webm/webm_cluster_parser.cc:219: << "Illegal 'Code 3' pus packet with frame count zero; demuxed " Oh noes! s/pus/Opus/ please
Fixed :) https://codereview.chromium.org/883403002/diff/100001/media/formats/webm/webm... File media/formats/webm/webm_cluster_parser.cc (right): https://codereview.chromium.org/883403002/diff/100001/media/formats/webm/webm... media/formats/webm/webm_cluster_parser.cc:219: << "Illegal 'Code 3' pus packet with frame count zero; demuxed " On 2015/02/07 00:23:55, wolenetz wrote: > Oh noes! s/pus/Opus/ please Done.
On 2015/02/09 19:21:51, chcunningham wrote: > Fixed :) > > https://codereview.chromium.org/883403002/diff/100001/media/formats/webm/webm... > File media/formats/webm/webm_cluster_parser.cc (right): > > https://codereview.chromium.org/883403002/diff/100001/media/formats/webm/webm... > media/formats/webm/webm_cluster_parser.cc:219: << "Illegal 'Code 3' pus packet > with frame count zero; demuxed " > On 2015/02/07 00:23:55, wolenetz wrote: > > Oh noes! s/pus/Opus/ please > > Done. still lgtm. thanks for fixing this.
The CQ bit was checked by chcunningham@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/883403002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4614245a773c1caa6013f061b3b80151552901f5 Cr-Commit-Position: refs/heads/master@{#315395} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
