Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1316273002:
Updated AAC:Parse() to emit better error logs. (Closed)
|
Created:
5 years, 3 months ago by msu.koo Modified:
5 years, 3 months ago Reviewers:
wolenetz CC:
chromium-reviews, feature-media-reviews_chromium.org, pangu Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdated AAC:Parse() to emit better error logs.
This patch includes
- Update AAC:Parse() to log specific reasons for RCHECK failures.
- Update AAC:Parse() to log config details when parsing succeeds.
BUG=524159
Committed: https://crrev.com/c77e527bbd46edb3ea0fbe728500c266735d23d7
Cr-Commit-Position: refs/heads/master@{#350650}
Patch Set 1 #
Total comments: 8
Patch Set 2 : #
Total comments: 12
Patch Set 3 : #
Total comments: 1
Patch Set 4 : #
Total comments: 28
Patch Set 5 : #Patch Set 6 : #
Total comments: 13
Patch Set 7 : #
Total comments: 4
Patch Set 8 : #Messages
Total messages: 32 (5 generated)
msu.koo@samsung.com changed reviewers: + wolenetz@chromium.org
I hope this patch to meet your comments. PTAL
Thanks for preparing this change! It looks pretty good. I'd like to (re)land my mp4 parser unit test media log verification patch before this, and I also need to do a little deeper review to make sure the final media log strings in your patch reference the right tables and so forth. https://codereview.chromium.org/1316273002/diff/1/media/formats/mp4/aac.cc File media/formats/mp4/aac.cc (right): https://codereview.chromium.org/1316273002/diff/1/media/formats/mp4/aac.cc#ne... media/formats/mp4/aac.cc:98: "Unsupported Sampling Frequency Index with value 0x" Does this build and do the various log messages show as expected? Note that RCHECK_MEDIA_LOGGED has () around its msg parameter, and I'm not totally convinced that StringStream << ("foo" << 42 << "bar") works as expected. Note also that I'll be (re-)landing soon (probably tomorrow) a commit that adds strict MEDIA_LOG verification to mp4 parser for any logs that are emitted during the course of the existing mp4 unit tests. Since logs are changing in this patch set, those unit tests will need their logging expectations updated. https://codereview.chromium.org/1316273002/diff/1/media/formats/mp4/aac.cc#ne... media/formats/mp4/aac.cc:130: RCHECK_MEDIA_LOGGED(channel_layout_ == CHANNEL_LAYOUT_NONE, media_log, NONE == 0 is potentially a fragile assumption. I suggest either a compile assert that CHANNEL_LAYOUT_NONE is 0 here, or just << CHANNEL_LAYOUT_NONE in the log text instead of 0. https://codereview.chromium.org/1316273002/diff/1/media/formats/mp4/aac.cc#ne... media/formats/mp4/aac.cc:140: << "extension_frequency : " << extension_frequency_ << "Hz" Here and elsewhere in some of these new logs, needs delimiters. Currently, this would show like ....Hzchannel_layout : ... https://codereview.chromium.org/1316273002/diff/1/media/formats/mp4/aac.cc#ne... media/formats/mp4/aac.cc:141: << "channel_layout : " << channel_layout_; Please format the various fields consistently (Sampling frequency vs extension_frequency, etc, would look strange in chrome://media-internals.)
On 2015/08/27 01:52:17, wolenetz wrote: > Thanks for preparing this change! It looks pretty good. > I'd like to (re)land my mp4 parser unit test media log verification patch before > this, and I also need to do a little deeper review to make sure the final media > log strings in your patch reference the right tables and so forth. > > https://codereview.chromium.org/1316273002/diff/1/media/formats/mp4/aac.cc > File media/formats/mp4/aac.cc (right): > > https://codereview.chromium.org/1316273002/diff/1/media/formats/mp4/aac.cc#ne... > media/formats/mp4/aac.cc:98: "Unsupported Sampling Frequency Index with value > 0x" > Does this build and do the various log messages show as expected? > Note that RCHECK_MEDIA_LOGGED has () around its msg parameter, and I'm not > totally convinced that StringStream << ("foo" << 42 << "bar") works as expected. > Note also that I'll be (re-)landing soon (probably tomorrow) a commit that adds > strict MEDIA_LOG verification to mp4 parser for any logs that are emitted during > the course of the existing mp4 unit tests. Since logs are changing in this patch > set, those unit tests will need their logging expectations updated. > > https://codereview.chromium.org/1316273002/diff/1/media/formats/mp4/aac.cc#ne... > media/formats/mp4/aac.cc:130: RCHECK_MEDIA_LOGGED(channel_layout_ == > CHANNEL_LAYOUT_NONE, media_log, > NONE == 0 is potentially a fragile assumption. I suggest either a compile assert > that CHANNEL_LAYOUT_NONE is 0 here, or just << CHANNEL_LAYOUT_NONE in the log > text instead of 0. > > https://codereview.chromium.org/1316273002/diff/1/media/formats/mp4/aac.cc#ne... > media/formats/mp4/aac.cc:140: << "extension_frequency : " << > extension_frequency_ << "Hz" > Here and elsewhere in some of these new logs, needs delimiters. Currently, this > would show like ....Hzchannel_layout : ... > > https://codereview.chromium.org/1316273002/diff/1/media/formats/mp4/aac.cc#ne... > media/formats/mp4/aac.cc:141: << "channel_layout : " << channel_layout_; > Please format the various fields consistently (Sampling frequency vs > extension_frequency, etc, would look strange in chrome://media-internals.) As noted, this patch will need rebasing (and test updates) once https://codereview.chromium.org/1317063003/ lands (it's in CQ now).
Thank you for your valuable comments :) I'll address your comments and rebase this after your patch is landed. On 2015/08/27 02:15:37, wolenetz wrote: > On 2015/08/27 01:52:17, wolenetz wrote: > > Thanks for preparing this change! It looks pretty good. > > I'd like to (re)land my mp4 parser unit test media log verification patch > before > > this, and I also need to do a little deeper review to make sure the final > media > > log strings in your patch reference the right tables and so forth. > > > > https://codereview.chromium.org/1316273002/diff/1/media/formats/mp4/aac.cc > > File media/formats/mp4/aac.cc (right): > > > > > https://codereview.chromium.org/1316273002/diff/1/media/formats/mp4/aac.cc#ne... > > media/formats/mp4/aac.cc:98: "Unsupported Sampling Frequency Index with value > > 0x" > > Does this build and do the various log messages show as expected? > > Note that RCHECK_MEDIA_LOGGED has () around its msg parameter, and I'm not > > totally convinced that StringStream << ("foo" << 42 << "bar") works as > expected. > > Note also that I'll be (re-)landing soon (probably tomorrow) a commit that > adds > > strict MEDIA_LOG verification to mp4 parser for any logs that are emitted > during > > the course of the existing mp4 unit tests. Since logs are changing in this > patch > > set, those unit tests will need their logging expectations updated. > > > > > https://codereview.chromium.org/1316273002/diff/1/media/formats/mp4/aac.cc#ne... > > media/formats/mp4/aac.cc:130: RCHECK_MEDIA_LOGGED(channel_layout_ == > > CHANNEL_LAYOUT_NONE, media_log, > > NONE == 0 is potentially a fragile assumption. I suggest either a compile > assert > > that CHANNEL_LAYOUT_NONE is 0 here, or just << CHANNEL_LAYOUT_NONE in the log > > text instead of 0. > > > > > https://codereview.chromium.org/1316273002/diff/1/media/formats/mp4/aac.cc#ne... > > media/formats/mp4/aac.cc:140: << "extension_frequency : " << > > extension_frequency_ << "Hz" > > Here and elsewhere in some of these new logs, needs delimiters. Currently, > this > > would show like ....Hzchannel_layout : ... > > > > > https://codereview.chromium.org/1316273002/diff/1/media/formats/mp4/aac.cc#ne... > > media/formats/mp4/aac.cc:141: << "channel_layout : " << channel_layout_; > > Please format the various fields consistently (Sampling frequency vs > > extension_frequency, etc, would look strange in chrome://media-internals.) > > As noted, this patch will need rebasing (and test updates) once > https://codereview.chromium.org/1317063003/ lands (it's in CQ now).
Patchset #2 (id:20001) has been deleted
Thank you for your comments. :) I addressed all of your comments regarding your new acc_unittest. PTAL https://codereview.chromium.org/1316273002/diff/1/media/formats/mp4/aac.cc File media/formats/mp4/aac.cc (right): https://codereview.chromium.org/1316273002/diff/1/media/formats/mp4/aac.cc#ne... media/formats/mp4/aac.cc:98: "Unsupported Sampling Frequency Index with value 0x" On 2015/08/27 01:52:16, wolenetz wrote: > Does this build and do the various log messages show as expected? > Note that RCHECK_MEDIA_LOGGED has () around its msg parameter, and I'm not > totally convinced that StringStream << ("foo" << 42 << "bar") works as expected. > Note also that I'll be (re-)landing soon (probably tomorrow) a commit that adds > strict MEDIA_LOG verification to mp4 parser for any logs that are emitted during > the course of the existing mp4 unit tests. Since logs are changing in this patch > set, those unit tests will need their logging expectations updated. Done. https://codereview.chromium.org/1316273002/diff/1/media/formats/mp4/aac.cc#ne... media/formats/mp4/aac.cc:130: RCHECK_MEDIA_LOGGED(channel_layout_ == CHANNEL_LAYOUT_NONE, media_log, On 2015/08/27 01:52:16, wolenetz wrote: > NONE == 0 is potentially a fragile assumption. I suggest either a compile assert > that CHANNEL_LAYOUT_NONE is 0 here, or just << CHANNEL_LAYOUT_NONE in the log > text instead of 0. Done. https://codereview.chromium.org/1316273002/diff/1/media/formats/mp4/aac.cc#ne... media/formats/mp4/aac.cc:140: << "extension_frequency : " << extension_frequency_ << "Hz" On 2015/08/27 01:52:16, wolenetz wrote: > Here and elsewhere in some of these new logs, needs delimiters. Currently, this > would show like ....Hzchannel_layout : ... Done. https://codereview.chromium.org/1316273002/diff/1/media/formats/mp4/aac.cc#ne... media/formats/mp4/aac.cc:141: << "channel_layout : " << channel_layout_; On 2015/08/27 01:52:16, wolenetz wrote: > Please format the various fields consistently (Sampling frequency vs > extension_frequency, etc, would look strange in chrome://media-internals.) Done.
Thanks for contributing this! This looks good % nits and % the following: Since we now have an easy way to verify what the log messages look like, please expand the scope of this CL to include the following item from the bug to cover the new logs introduced in this CL. Updating the tests in a later CL would allow the possibility that the new log messages might regress in the interim, and generally tests are added in the same change as the production code change that underlies the tests. "4) Updating the associated unit tests to have updated (and perhaps more variety) of EXPECT_MEDIA_LOG()." https://codereview.chromium.org/1316273002/diff/40001/media/formats/mp4/aac.cc File media/formats/mp4/aac.cc (right): https://codereview.chromium.org/1316273002/diff/40001/media/formats/mp4/aac.c... media/formats/mp4/aac.cc:110: << "Unsupported Sampling Frequency Index with value 0x" ISTM text like "Unsupported Extension Sampling Frequency Index..." would be more specifically correct in this particular log message. Am I mistaken? https://codereview.chromium.org/1316273002/diff/40001/media/formats/mp4/aac.c... media/formats/mp4/aac.cc:126: << static_cast<int>(channel_config_) << "." nit: s/"."/". "/ https://codereview.chromium.org/1316273002/diff/40001/media/formats/mp4/aac.c... media/formats/mp4/aac.cc:139: if (profile_ < 1 || profile_ > 4) { side note: Looking over this logic, I've filed crbug 526314 since SBR-in-mimetype logic, above, is confusing to me when profile 5 is "SBR". There's probably a good reason why the logic is the way it is, but I'd like to understand better why (hence that bug to track follow-up). https://codereview.chromium.org/1316273002/diff/40001/media/formats/mp4/aac.c... media/formats/mp4/aac.cc:142: << " is not supported." nit ditto: s/"."/". "/ https://codereview.chromium.org/1316273002/diff/40001/media/formats/mp4/aac.c... media/formats/mp4/aac.cc:152: << ", Channel_layout: " << channel_layout_; nit: s/Channel_layout/Channel layout/ https://codereview.chromium.org/1316273002/diff/40001/media/formats/mp4/aac_u... File media/formats/mp4/aac_unittest.cc (right): https://codereview.chromium.org/1316273002/diff/40001/media/formats/mp4/aac_u... media/formats/mp4/aac_unittest.cc:165: EXPECT_MEDIA_LOG(AudioCodecLog("mp4a.40.1")).Times(1); nit: .Times(1) is redundant. Drop this part of this line please.
Patchset #3 (id:60001) has been deleted
Your comments were all addressed. PTAL https://codereview.chromium.org/1316273002/diff/40001/media/formats/mp4/aac.cc File media/formats/mp4/aac.cc (right): https://codereview.chromium.org/1316273002/diff/40001/media/formats/mp4/aac.c... media/formats/mp4/aac.cc:110: << "Unsupported Sampling Frequency Index with value 0x" On 2015/08/28 23:29:35, wolenetz wrote: > ISTM text like "Unsupported Extension Sampling Frequency Index..." would be more > specifically correct in this particular log message. Am I mistaken? Done. https://codereview.chromium.org/1316273002/diff/40001/media/formats/mp4/aac.c... media/formats/mp4/aac.cc:126: << static_cast<int>(channel_config_) << "." On 2015/08/28 23:29:35, wolenetz wrote: > nit: s/"."/". "/ Done. https://codereview.chromium.org/1316273002/diff/40001/media/formats/mp4/aac.c... media/formats/mp4/aac.cc:139: if (profile_ < 1 || profile_ > 4) { On 2015/08/28 23:29:35, wolenetz wrote: > side note: Looking over this logic, I've filed crbug 526314 since > SBR-in-mimetype logic, above, is confusing to me when profile 5 is "SBR". > There's probably a good reason why the logic is the way it is, but I'd like to > understand better why (hence that bug to track follow-up). OK, I also agree with you opinion. More investigation is required whether we can support Audio Object type 5. Let's see on your new filed issue. https://codereview.chromium.org/1316273002/diff/40001/media/formats/mp4/aac.c... media/formats/mp4/aac.cc:142: << " is not supported." On 2015/08/28 23:29:35, wolenetz wrote: > nit ditto: s/"."/". "/ Done. https://codereview.chromium.org/1316273002/diff/40001/media/formats/mp4/aac.c... media/formats/mp4/aac.cc:152: << ", Channel_layout: " << channel_layout_; On 2015/08/28 23:29:35, wolenetz wrote: > nit: s/Channel_layout/Channel layout/ Done. https://codereview.chromium.org/1316273002/diff/40001/media/formats/mp4/aac_u... File media/formats/mp4/aac_unittest.cc (right): https://codereview.chromium.org/1316273002/diff/40001/media/formats/mp4/aac_u... media/formats/mp4/aac_unittest.cc:165: EXPECT_MEDIA_LOG(AudioCodecLog("mp4a.40.1")).Times(1); On 2015/08/28 23:29:35, wolenetz wrote: > nit: .Times(1) is redundant. Drop this part of this line please. Done.
Thanks for addressing my in-line comments. I also had one further out-of-line comment in previous CR iteration that I would like addressed before this lands: Since we now have an easy way to verify what the log messages look like, please expand the scope of this CL to include the following item from the bug to cover the new logs introduced in this CL. Updating the tests in a later CL would allow the possibility that the new log messages might regress in the interim, and generally tests are added in the same change as the production code change that underlies the tests. "4) Updating the associated unit tests to have updated (and perhaps more variety) of EXPECT_MEDIA_LOG()." https://codereview.chromium.org/1316273002/diff/80001/media/formats/mp4/aac.cc File media/formats/mp4/aac.cc (right): https://codereview.chromium.org/1316273002/diff/80001/media/formats/mp4/aac.c... media/formats/mp4/aac.cc:110: << "Unsupported Extenson Sampling Frequency Index with value 0x" nit: s/Extenson/Extension/
Thank you for your review. I forgot to address your last comments so it was reflected in this patch set. PTAL
Thanks for addressing my concerns. This is looking much better. I have mostly just a few remaining nits and additional test coverage requests: https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac.cc File media/formats/mp4/aac.cc (right): https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:98: << "Unsupported Sampling Frequency Index with value 0x" please add a unit test that exposes this log and verifies it occurs. https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:110: << "Unsupported Extension Sampling Frequency Index with value 0x" ditto https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:125: << "Unsupported Channel Configuration with value " ditto https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:135: "Audio frequency is invalid with value 0."); ditto https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:141: << "Audio codec: mp4a.40." << static_cast<int>(profile_) ditto https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac_... File media/formats/mp4/aac_unittest.cc (right): https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac_... media/formats/mp4/aac_unittest.cc:24: MATCHER_P(AudioSamplingFrequencyLog, codec_string, "") { nit: s/codec/frequency/ https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac_... media/formats/mp4/aac_unittest.cc:26: "Sampling frequency: " + std::string(codec_string)); nit ditto https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac_... media/formats/mp4/aac_unittest.cc:29: MATCHER_P(AudioExtensionSamplingFrequencyLog, codec_string, "") { nit: s/codec/extension/ https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac_... media/formats/mp4/aac_unittest.cc:31: arg, "Sampling frequency(Extension): " + std::string(codec_string)); nit ditto https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac_... media/formats/mp4/aac_unittest.cc:34: MATCHER_P(AudioChannelLayoutLog, codec_string, "") { nit: s/codec/layout/ https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac_... media/formats/mp4/aac_unittest.cc:35: return CONTAINS_STRING(arg, "Channel layout: " + std::string(codec_string)); nit ditto https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac_... media/formats/mp4/aac_unittest.cc:56: EXPECT_MEDIA_LOG(AllOf(AudioCodecLog("mp4a.40.2"), Neat usage of AllOf. I like it. However, we really just have one "success" log entry with a static format, so we could just have one MATCHER_P4 for it, parameterized by each of the codec_string, frequency_string, extension_string, and layout_string. Then, AllOf wouldn't really be needed. I don't really have a strong feeling other than changing to my suggestion here would prevent regression if someone forgot to copy one of the "AllOf" entries in some test. It also makes the overall success string format more easy to verify. In fact, if somehow the log string contained a layout_string of "31", a matcher looking for just "3" would pass and give false success. So please also add an end-of-string delimiter like "." to the success log, and s/AllOf(multiple matchers)/one success log matcher/
On 2015/09/01 21:22:37, wolenetz wrote: > ... > > https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac_... > media/formats/mp4/aac_unittest.cc:56: > EXPECT_MEDIA_LOG(AllOf(AudioCodecLog("mp4a.40.2"), > Neat usage of AllOf. I like it. However, we really just have one "success" log > entry with a static format, so we could just have one MATCHER_P4 for it, > parameterized by each of the codec_string, frequency_string, extension_string, > and layout_string. Then, AllOf wouldn't really be needed. I don't really have a > strong feeling other than changing to my suggestion here would prevent > regression if someone forgot to copy one of the "AllOf" entries in some test. It > also makes the overall success string format more easy to verify. > > In fact, if somehow the log string contained a layout_string of "31", a matcher > looking for just "3" would pass and give false success. So please also add an > end-of-string delimiter like "." to the success log, and s/AllOf(multiple > matchers)/one success log matcher/ Since this "check for success and success log" is duplicated in multiple places, ISTM also like a good opportunity for a helper method that both calls the parse and expects the successful log, and the helper could be parameterized by all the substrings it should expect to be part of the matched successful log entry.
Also, fyi, I'll be out-of-office Wednesday PM through Monday PM (Sep 2 PM - Sep 7 PM Pacific timezone), so please pardon delayed code reviews during that time.
On 2015/09/01 21:27:35, wolenetz wrote: > Also, fyi, I'll be out-of-office Wednesday PM through Monday PM (Sep 2 PM - Sep > 7 PM Pacific timezone), so please pardon delayed code reviews during that time. fyi - I'm back and available to review further patch sets. Thanks!
https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac_... File media/formats/mp4/aac_unittest.cc (right): https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac_... media/formats/mp4/aac_unittest.cc:56: EXPECT_MEDIA_LOG(AllOf(AudioCodecLog("mp4a.40.2"), Thank you for your review. I also considered about using MATCHER_P4 before using Allof, because it's easier to implement :). But regarding the expandability and already-implemented approach(dedicated MATCHER functions by parameter, ex codec_string), I took the current way. It's more concise to use MATCHER_P4, but It'll also cause some duplicated matcher codes with "AudioSamplingFrequencyLog", and will be too dependent with current output log. So I would leave as it is, but will address your "delimiter" comment. WDYT? On 2015/09/01 21:22:37, wolenetz wrote: > Neat usage of AllOf. I like it. However, we really just have one "success" log > entry with a static format, so we could just have one MATCHER_P4 for it, > parameterized by each of the codec_string, frequency_string, extension_string, > and layout_string. Then, AllOf wouldn't really be needed. I don't really have a > strong feeling other than changing to my suggestion here would prevent > regression if someone forgot to copy one of the "AllOf" entries in some test. It > also makes the overall success string format more easy to verify. > > In fact, if somehow the log string contained a layout_string of "31", a matcher > looking for just "3" would pass and give false success. So please also add an > end-of-string delimiter like "." to the success log, and s/AllOf(multiple > matchers)/one success log matcher/
PTAL :) https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac.cc File media/formats/mp4/aac.cc (right): https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:98: << "Unsupported Sampling Frequency Index with value 0x" On 2015/09/01 21:22:37, wolenetz wrote: > please add a unit test that exposes this log and verifies it occurs. I understood your intention about this comments, but I'm not sure these are inevitable. I tried to add some MATCHERS to verify the error cases, but it's hard to find the cases because if the incoming data is invalid, Parse() already handles it as error before it comes to this line. I think the current unittest already well figures out the success cases, and also errors are well emitted when it happens. So I would leave the unittest as it is. Or do you have any specific data to check the error handling on unittest? https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:110: << "Unsupported Extension Sampling Frequency Index with value 0x" On 2015/09/01 21:22:37, wolenetz wrote: > ditto ditto https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:125: << "Unsupported Channel Configuration with value " On 2015/09/01 21:22:37, wolenetz wrote: > ditto ditto https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:135: "Audio frequency is invalid with value 0."); On 2015/09/01 21:22:37, wolenetz wrote: > ditto This line is dead. removed. https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:141: << "Audio codec: mp4a.40." << static_cast<int>(profile_) On 2015/09/01 21:22:37, wolenetz wrote: > ditto ditto https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac_... File media/formats/mp4/aac_unittest.cc (right): https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac_... media/formats/mp4/aac_unittest.cc:24: MATCHER_P(AudioSamplingFrequencyLog, codec_string, "") { On 2015/09/01 21:22:37, wolenetz wrote: > nit: s/codec/frequency/ Done. https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac_... media/formats/mp4/aac_unittest.cc:26: "Sampling frequency: " + std::string(codec_string)); On 2015/09/01 21:22:37, wolenetz wrote: > nit ditto Done. https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac_... media/formats/mp4/aac_unittest.cc:29: MATCHER_P(AudioExtensionSamplingFrequencyLog, codec_string, "") { On 2015/09/01 21:22:37, wolenetz wrote: > nit: s/codec/extension/ Done. https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac_... media/formats/mp4/aac_unittest.cc:31: arg, "Sampling frequency(Extension): " + std::string(codec_string)); On 2015/09/01 21:22:37, wolenetz wrote: > nit ditto Done. https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac_... media/formats/mp4/aac_unittest.cc:34: MATCHER_P(AudioChannelLayoutLog, codec_string, "") { On 2015/09/01 21:22:37, wolenetz wrote: > nit: s/codec/layout/ Done. https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac_... media/formats/mp4/aac_unittest.cc:35: return CONTAINS_STRING(arg, "Channel layout: " + std::string(codec_string)); On 2015/09/01 21:22:37, wolenetz wrote: > nit ditto Done.
https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac.cc File media/formats/mp4/aac.cc (right): https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:98: << "Unsupported Sampling Frequency Index with value 0x" On 2015/09/11 23:57:23, esnada wrote: > On 2015/09/01 21:22:37, wolenetz wrote: > > please add a unit test that exposes this log and verifies it occurs. > > I understood your intention about this comments, but I'm not sure these are > inevitable. I tried to add some MATCHERS to verify the error cases, but it's > hard to find the cases because if the incoming data is invalid, Parse() already > handles it as error before it comes to this line. > I think the current unittest already well figures out the success cases, and > also errors are well emitted when it happens. > So I would leave the unittest as it is. Or do you have any specific data to > check the error handling on unittest? Is it true that Parse() would error first always? If so, then this is dead code. I suspect it isn't dead code, and the test data could be constructed to exercise the error log. Same for those ditto'ed below. https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:135: "Audio frequency is invalid with value 0."); On 2015/09/11 23:57:23, esnada wrote: > On 2015/09/01 21:22:37, wolenetz wrote: > > ditto > > This line is dead. removed. Acknowledged. https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac_... File media/formats/mp4/aac_unittest.cc (right): https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac_... media/formats/mp4/aac_unittest.cc:56: EXPECT_MEDIA_LOG(AllOf(AudioCodecLog("mp4a.40.2"), On 2015/09/11 05:15:37, esnada wrote: > Thank you for your review. > I also considered about using MATCHER_P4 before using Allof, because it's easier > to implement :). But regarding the expandability and already-implemented > approach(dedicated MATCHER functions by parameter, ex codec_string), I took the > current way. > > It's more concise to use MATCHER_P4, but It'll also cause some duplicated > matcher codes with "AudioSamplingFrequencyLog", and will be too dependent with > current output log. > > So I would leave as it is, but will address your "delimiter" comment. > WDYT? > > On 2015/09/01 21:22:37, wolenetz wrote: > > Neat usage of AllOf. I like it. However, we really just have one "success" log > > entry with a static format, so we could just have one MATCHER_P4 for it, > > parameterized by each of the codec_string, frequency_string, extension_string, > > and layout_string. Then, AllOf wouldn't really be needed. I don't really have > a > > strong feeling other than changing to my suggestion here would prevent > > regression if someone forgot to copy one of the "AllOf" entries in some test. > It > > also makes the overall success string format more easy to verify. > > > > In fact, if somehow the log string contained a layout_string of "31", a > matcher > > looking for just "3" would pass and give false success. So please also add an > > end-of-string delimiter like "." to the success log, and s/AllOf(multiple > > matchers)/one success log matcher/ > Thanks. The new delimiters in the matchers are sufficient to prevent false matching. Using AllOf instead of MATCHER_P4 is not a big deal to me, so it's ok.
All your comments are addressed. Now error messages are also checked during unittests. PTAL :) https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac.cc File media/formats/mp4/aac.cc (right): https://codereview.chromium.org/1316273002/diff/100001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:98: << "Unsupported Sampling Frequency Index with value 0x" On 2015/09/14 23:17:57, wolenetz wrote: > On 2015/09/11 23:57:23, esnada wrote: > > On 2015/09/01 21:22:37, wolenetz wrote: > > > please add a unit test that exposes this log and verifies it occurs. > > > > I understood your intention about this comments, but I'm not sure these are > > inevitable. I tried to add some MATCHERS to verify the error cases, but it's > > hard to find the cases because if the incoming data is invalid, Parse() > already > > handles it as error before it comes to this line. > > I think the current unittest already well figures out the success cases, and > > also errors are well emitted when it happens. > > So I would leave the unittest as it is. Or do you have any specific data to > > check the error handling on unittest? > > Is it true that Parse() would error first always? If so, then this is dead code. > I suspect it isn't dead code, and the test data could be constructed to exercise > the error log. Same for those ditto'ed below. Done. and ditto'ed below.
Looking really good. Thanks for your patience and diligence getting this code updated. Just a few small nits left, I hope: :) https://codereview.chromium.org/1316273002/diff/140001/media/formats/mp4/aac.cc File media/formats/mp4/aac.cc (right): https://codereview.chromium.org/1316273002/diff/140001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:41: // The following code is written according to ISO 14496 Part 3 Table 1.13 - nit: s/14496 Part 3 Table 1.13/14496-3-2009 Table 1.15/, though profile_ doesn't seem to allow for audioObjectTypeExt in the stream. Given this and similar I saw on simple digging, I think we should keep this CL simpler (refer to 14496-3-2005, not ...2009 table references) and add a TODO (and file related crbug) to consider any necessary updates to comments/logic to conform to the ...2009 spec. So, please leave this comment as-is, but adjust the new MEDIA_LOG table references to reference explicitly the 14496-3-2005 version and table numbers. And add the TODO referencing a newly filed crbug please :) https://codereview.chromium.org/1316273002/diff/140001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:134: RCHECK_MEDIA_LOGGED(channel_layout_ != CHANNEL_LAYOUT_NONE, media_log, nit: it looks like this log is also possible to test. https://codereview.chromium.org/1316273002/diff/140001/media/formats/mp4/aac_... File media/formats/mp4/aac_unittest.cc (right): https://codereview.chromium.org/1316273002/diff/140001/media/formats/mp4/aac_... media/formats/mp4/aac_unittest.cc:274: EXPECT_MEDIA_LOG(UnsupportedFrquencyIndexLog("e")); nit: s/Frq/Freq/ https://codereview.chromium.org/1316273002/diff/140001/media/formats/mp4/aac_... media/formats/mp4/aac_unittest.cc:291: EXPECT_MEDIA_LOG(UnsupportedExtensionFrquencyIndexLog("e")); nit ditto
Patchset #7 (id:160001) has been deleted
Your comments was addressed. Hope this will be final :) PTAL https://codereview.chromium.org/1316273002/diff/140001/media/formats/mp4/aac.cc File media/formats/mp4/aac.cc (right): https://codereview.chromium.org/1316273002/diff/140001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:41: // The following code is written according to ISO 14496 Part 3 Table 1.13 - On 2015/09/15 23:53:17, wolenetz wrote: > nit: s/14496 Part 3 Table 1.13/14496-3-2009 Table 1.15/, though profile_ doesn't > seem to allow for audioObjectTypeExt in the stream. > > Given this and similar I saw on simple digging, I think we should keep this CL > simpler (refer to 14496-3-2005, not ...2009 table references) and add a TODO > (and file related crbug) to consider any necessary updates to comments/logic to > conform to the ...2009 spec. > > So, please leave this comment as-is, but adjust the new MEDIA_LOG table > references to reference explicitly the 14496-3-2005 version and table numbers. > And add the TODO referencing a newly filed crbug please :) Thank you for your comments. I tried to post a new issue for this, but maybe I don't have enough permissions to raise all-platform type issue. Can you raise it and let me know the url? https://codereview.chromium.org/1316273002/diff/140001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:134: RCHECK_MEDIA_LOGGED(channel_layout_ != CHANNEL_LAYOUT_NONE, media_log, On 2015/09/15 23:53:17, wolenetz wrote: > nit: it looks like this log is also possible to test. This code block is dead. "|channel_config_| == 0" is filtered out during "RCHECK(SkipDecoderGASpecificConfig(&reader));"(line 61), which tested at line 263. https://codereview.chromium.org/1316273002/diff/140001/media/formats/mp4/aac_... File media/formats/mp4/aac_unittest.cc (right): https://codereview.chromium.org/1316273002/diff/140001/media/formats/mp4/aac_... media/formats/mp4/aac_unittest.cc:274: EXPECT_MEDIA_LOG(UnsupportedFrquencyIndexLog("e")); On 2015/09/15 23:53:17, wolenetz wrote: > nit: s/Frq/Freq/ Done. https://codereview.chromium.org/1316273002/diff/140001/media/formats/mp4/aac_... media/formats/mp4/aac_unittest.cc:291: EXPECT_MEDIA_LOG(UnsupportedExtensionFrquencyIndexLog("e")); On 2015/09/15 23:53:17, wolenetz wrote: > nit ditto Done.
Just small changes remaining. Almost there! https://codereview.chromium.org/1316273002/diff/140001/media/formats/mp4/aac.cc File media/formats/mp4/aac.cc (right): https://codereview.chromium.org/1316273002/diff/140001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:41: // The following code is written according to ISO 14496 Part 3 Table 1.13 - On 2015/09/16 08:16:23, msu.koo wrote: > On 2015/09/15 23:53:17, wolenetz wrote: > > nit: s/14496 Part 3 Table 1.13/14496-3-2009 Table 1.15/, though profile_ > doesn't > > seem to allow for audioObjectTypeExt in the stream. > > > > Given this and similar I saw on simple digging, I think we should keep this CL > > simpler (refer to 14496-3-2005, not ...2009 table references) and add a TODO > > (and file related crbug) to consider any necessary updates to comments/logic > to > > conform to the ...2009 spec. > > > > So, please leave this comment as-is, but adjust the new MEDIA_LOG table > > references to reference explicitly the 14496-3-2005 version and table numbers. > > And add the TODO referencing a newly filed crbug please :) > > Thank you for your comments. I tried to post a new issue for this, but maybe I > don't have enough permissions to raise all-platform type issue. > Can you raise it and let me know the url? It looks to me like you have successfully filed https://crbug.com/532281 (or do I misunderstand?) https://codereview.chromium.org/1316273002/diff/140001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:134: RCHECK_MEDIA_LOGGED(channel_layout_ != CHANNEL_LAYOUT_NONE, media_log, On 2015/09/16 08:16:23, msu.koo wrote: > On 2015/09/15 23:53:17, wolenetz wrote: > > nit: it looks like this log is also possible to test. > > This code block is dead. "|channel_config_| == 0" is filtered out during > "RCHECK(SkipDecoderGASpecificConfig(&reader));"(line 61), which tested at line > 263. Acknowledged. Since the helper method might change its implementation, let's protect our borders in debug builds at least and add a DCHECK(channel_layout_ != CHANNEL_LAYOUT_NONE) here. https://codereview.chromium.org/1316273002/diff/180001/media/formats/mp4/aac.cc File media/formats/mp4/aac.cc (right): https://codereview.chromium.org/1316273002/diff/180001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:44: // http://crbug.com/532281 nit: s/http:/https:/ https://codereview.chromium.org/1316273002/diff/180001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:105: << ") is not supported. Please see ISO 14496-3-2005 Table 1.18 " nit: in 2005 version, it's not 1.18. similar dittos, below. :)
PTAL :) https://codereview.chromium.org/1316273002/diff/140001/media/formats/mp4/aac.cc File media/formats/mp4/aac.cc (right): https://codereview.chromium.org/1316273002/diff/140001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:41: // The following code is written according to ISO 14496 Part 3 Table 1.13 - On 2015/09/16 19:39:19, wolenetz wrote: > On 2015/09/16 08:16:23, msu.koo wrote: > > On 2015/09/15 23:53:17, wolenetz wrote: > > > nit: s/14496 Part 3 Table 1.13/14496-3-2009 Table 1.15/, though profile_ > > doesn't > > > seem to allow for audioObjectTypeExt in the stream. > > > > > > Given this and similar I saw on simple digging, I think we should keep this > CL > > > simpler (refer to 14496-3-2005, not ...2009 table references) and add a TODO > > > (and file related crbug) to consider any necessary updates to comments/logic > > to > > > conform to the ...2009 spec. > > > > > > So, please leave this comment as-is, but adjust the new MEDIA_LOG table > > > references to reference explicitly the 14496-3-2005 version and table > numbers. > > > And add the TODO referencing a newly filed crbug please :) > > > > Thank you for your comments. I tried to post a new issue for this, but maybe I > > don't have enough permissions to raise all-platform type issue. > > Can you raise it and let me know the url? > > It looks to me like you have successfully filed https://crbug.com/532281 (or do > I misunderstand?) Oh, I forgot to update this comment :) I filed that bug as linux issue, and you already changed to all-platform. By the way, can you let me know how can I get the permission to edit issue? https://codereview.chromium.org/1316273002/diff/140001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:134: RCHECK_MEDIA_LOGGED(channel_layout_ != CHANNEL_LAYOUT_NONE, media_log, On 2015/09/16 19:39:19, wolenetz wrote: > On 2015/09/16 08:16:23, msu.koo wrote: > > On 2015/09/15 23:53:17, wolenetz wrote: > > > nit: it looks like this log is also possible to test. > > > > This code block is dead. "|channel_config_| == 0" is filtered out during > > "RCHECK(SkipDecoderGASpecificConfig(&reader));"(line 61), which tested at line > > 263. > > Acknowledged. Since the helper method might change its implementation, let's > protect our borders in debug builds at least and add a DCHECK(channel_layout_ != > CHANNEL_LAYOUT_NONE) here. Done. https://codereview.chromium.org/1316273002/diff/180001/media/formats/mp4/aac.cc File media/formats/mp4/aac.cc (right): https://codereview.chromium.org/1316273002/diff/180001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:44: // http://crbug.com/532281 On 2015/09/16 19:39:19, wolenetz wrote: > nit: s/http:/https:/ Done. https://codereview.chromium.org/1316273002/diff/180001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:105: << ") is not supported. Please see ISO 14496-3-2005 Table 1.18 " On 2015/09/16 19:39:19, wolenetz wrote: > nit: in 2005 version, it's not 1.18. similar dittos, below. :) Done.
Can you take a look this patchset?
Sorry for the short delay. I'll send this to CQ shortly; it LGTM! Thanks for contributing this change! https://codereview.chromium.org/1316273002/diff/140001/media/formats/mp4/aac.cc File media/formats/mp4/aac.cc (right): https://codereview.chromium.org/1316273002/diff/140001/media/formats/mp4/aac.... media/formats/mp4/aac.cc:41: // The following code is written according to ISO 14496 Part 3 Table 1.13 - On 2015/09/17 02:49:26, msu.koo_ooo_28_sep wrote: > On 2015/09/16 19:39:19, wolenetz wrote: > > On 2015/09/16 08:16:23, msu.koo wrote: > > > On 2015/09/15 23:53:17, wolenetz wrote: > > > > nit: s/14496 Part 3 Table 1.13/14496-3-2009 Table 1.15/, though profile_ > > > doesn't > > > > seem to allow for audioObjectTypeExt in the stream. > > > > > > > > Given this and similar I saw on simple digging, I think we should keep > this > > CL > > > > simpler (refer to 14496-3-2005, not ...2009 table references) and add a > TODO > > > > (and file related crbug) to consider any necessary updates to > comments/logic > > > to > > > > conform to the ...2009 spec. > > > > > > > > So, please leave this comment as-is, but adjust the new MEDIA_LOG table > > > > references to reference explicitly the 14496-3-2005 version and table > > numbers. > > > > And add the TODO referencing a newly filed crbug please :) > > > > > > Thank you for your comments. I tried to post a new issue for this, but maybe > I > > > don't have enough permissions to raise all-platform type issue. > > > Can you raise it and let me know the url? > > > > It looks to me like you have successfully filed https://crbug.com/532281 (or > do > > I misunderstand?) > > Oh, I forgot to update this comment :) I filed that bug as linux issue, and you > already changed to all-platform. > By the way, can you let me know how can I get the permission to edit issue? The process for obtaining such edit issue permissions is documented at https://www.chromium.org/getting-involved/get-bug-editing-privileges For short-term, you can always request another contributor with those permissions to make the necessary changes. For long-term, that documented process is the way to go :)
The CQ bit was checked by wolenetz@chromium.org
On 2015/09/24 19:05:39, wolenetz wrote: > Sorry for the short delay. I'll send this to CQ shortly; it LGTM! > > Thanks for contributing this change! > > https://codereview.chromium.org/1316273002/diff/140001/media/formats/mp4/aac.cc > File media/formats/mp4/aac.cc (right): > > https://codereview.chromium.org/1316273002/diff/140001/media/formats/mp4/aac.... > media/formats/mp4/aac.cc:41: // The following code is written according to ISO > 14496 Part 3 Table 1.13 - > On 2015/09/17 02:49:26, msu.koo_ooo_28_sep wrote: > > On 2015/09/16 19:39:19, wolenetz wrote: > > > On 2015/09/16 08:16:23, msu.koo wrote: > > > > On 2015/09/15 23:53:17, wolenetz wrote: > > > > > nit: s/14496 Part 3 Table 1.13/14496-3-2009 Table 1.15/, though profile_ > > > > doesn't > > > > > seem to allow for audioObjectTypeExt in the stream. > > > > > > > > > > Given this and similar I saw on simple digging, I think we should keep > > this > > > CL > > > > > simpler (refer to 14496-3-2005, not ...2009 table references) and add a > > TODO > > > > > (and file related crbug) to consider any necessary updates to > > comments/logic > > > > to > > > > > conform to the ...2009 spec. > > > > > > > > > > So, please leave this comment as-is, but adjust the new MEDIA_LOG table > > > > > references to reference explicitly the 14496-3-2005 version and table > > > numbers. > > > > > And add the TODO referencing a newly filed crbug please :) > > > > > > > > Thank you for your comments. I tried to post a new issue for this, but > maybe > > I > > > > don't have enough permissions to raise all-platform type issue. > > > > Can you raise it and let me know the url? > > > > > > It looks to me like you have successfully filed https://crbug.com/532281 (or > > do > > > I misunderstand?) > > > > Oh, I forgot to update this comment :) I filed that bug as linux issue, and > you > > already changed to all-platform. > > By the way, can you let me know how can I get the permission to edit issue? > > The process for obtaining such edit issue permissions is documented at > https://www.chromium.org/getting-involved/get-bug-editing-privileges > > For short-term, you can always request another contributor with those > permissions to make the necessary changes. For long-term, that documented > process is the way to go :) (Though that doc is focused on triage, if you frequently need to make changes to issues outside of triage, we can talk about your nomination.)
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1316273002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1316273002/200001
Message was sent while issue was closed.
Committed patchset #8 (id:200001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/c77e527bbd46edb3ea0fbe728500c266735d23d7 Cr-Commit-Position: refs/heads/master@{#350650} |
