Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2753773002:
Fix failed to write thumbnail png issue (Closed)
DescriptionFix failed to write thumbnail png issue
In https://codereview.chromium.org/2594243002/, filepath of png output is
wrapped to GetTestDataFile(), which will use base::MakeAbsoluteFilePath() to
get absolute path. However, since the output file doesn't exist (yet), this
function will return an empty FilePath instead and makes write file failed.
BUG=701232
TEST=Run Thumbnail/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0 of VDA
unittest and make the test matches no md5sum. The test will generate
bad_thumbnails.png successfully.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2753773002
Cr-Commit-Position: refs/heads/master@{#458706}
Committed: https://chromium.googlesource.com/chromium/src/+/58af2e5f92c3c5706d7aba46169001c618684169
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fix failed to write thumbnail png issue #
Total comments: 2
Patch Set 3 : nit #Patch Set 4 : Fix failed to write thumbnail png issue #
Total comments: 2
Patch Set 5 : lgtm nits #Messages
Total messages: 25 (10 generated)
Description was changed from ========== Fix failed to write thumbnail png issue In https://codereview.chromium.org/2594243002/, filepath of png output is wrapped to GetTestDataFile(), which will use base::MakeAbsoluteFilePath() to get absolute path. However, since the output file doesn't exist (yet), this function will return an empty FilePath instead and makes write file failed. BUG=701232 TEST=Run Thumbnail/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0 of VDA unittest and make the test matches no md5sum. The test will generate bad_thumbnails.png successfully. ========== to ========== Fix failed to write thumbnail png issue In https://codereview.chromium.org/2594243002/, filepath of png output is wrapped to GetTestDataFile(), which will use base::MakeAbsoluteFilePath() to get absolute path. However, since the output file doesn't exist (yet), this function will return an empty FilePath instead and makes write file failed. BUG=701232 TEST=Run Thumbnail/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0 of VDA unittest and make the test matches no md5sum. The test will generate bad_thumbnails.png successfully. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
johnylin@chromium.org changed reviewers: + posciak@chromium.org, wuchengli@chromium.org
wuchengli@chromium.org changed reviewers: + owenlin@chromium.org
Owen. Please help review this.
https://codereview.chromium.org/2753773002/diff/1/media/gpu/video_decode_acce... File media/gpu/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2753773002/diff/1/media/gpu/video_decode_acce... media/gpu/video_decode_accelerator_unittest.cc:203: // return an empty base::FilePath. Sounds like we should fail the test and LOG(FATAL) the information. https://codereview.chromium.org/2753773002/diff/1/media/gpu/video_decode_acce... media/gpu/video_decode_accelerator_unittest.cc:1545: filepath = GetTestDataFile(filepath); I would suggest to allow user to configure the output directory. So that we can save the output png files in autotest's result dir.
https://codereview.chromium.org/2753773002/diff/1/media/gpu/video_decode_acce... File media/gpu/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2753773002/diff/1/media/gpu/video_decode_acce... media/gpu/video_decode_accelerator_unittest.cc:203: // return an empty base::FilePath. On 2017/03/15 08:41:43, Owen Lin wrote: > Sounds like we should fail the test and LOG(FATAL) the information. Done. https://codereview.chromium.org/2753773002/diff/1/media/gpu/video_decode_acce... media/gpu/video_decode_accelerator_unittest.cc:1545: filepath = GetTestDataFile(filepath); On 2017/03/15 08:41:43, Owen Lin wrote: > I would suggest to allow user to configure the output directory. So that we can > save the output png files in autotest's result dir. Done.
lgtm % nit https://codereview.chromium.org/2753773002/diff/20001/media/gpu/video_decode_... File media/gpu/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2753773002/diff/20001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:148: base::FilePath g_test_output_dir; nit: How about naming it g_thumbnail_output_dir?
https://codereview.chromium.org/2753773002/diff/20001/media/gpu/video_decode_... File media/gpu/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2753773002/diff/20001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:148: base::FilePath g_test_output_dir; On 2017/03/16 09:26:58, Owen Lin wrote: > nit: How about naming it g_thumbnail_output_dir? Done.
The CQ bit was checked by johnylin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from owenlin@chromium.org Link to the patchset: https://codereview.chromium.org/2753773002/#ps40001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
After more investigation, I found there are still issues of writing thumbnail png for autotest. 1. The thumbnail write path for autotest is always the same directory of test data, which is /usr/local/autotest/deps/chrome_test/test_src/media/test/data/ (before and after CL:https://codereview.chromium.org/2594243002/). So I believe png file never been packed to test results. 2. Write file still fail for autotest even with this patch. base::WriteFile() seems unable to write under /usr/local/autotest/, but ok to write under /tmp/. Maybe it's because /tmp/ is mod 777, /usr/local/ is 755? But it has no issue for locally running on DUT (Manually run video_decode_accelerator_unittest)
On 2017/03/20 14:22:50, johnylin1 wrote: > After more investigation, I found there are still issues of writing thumbnail > png for autotest. > > 1. The thumbnail write path for autotest is always the same directory of test > data, which is /usr/local/autotest/deps/chrome_test/test_src/media/test/data/ > (before and after CL:https://codereview.chromium.org/2594243002/). So I believe > png file never been packed to test results. > > 2. Write file still fail for autotest even with this patch. base::WriteFile() > seems unable to write under /usr/local/autotest/, but ok to write under /tmp/. > Maybe it's because /tmp/ is mod 777, /usr/local/ is 755? > But it has no issue for locally running on DUT (Manually run > video_decode_accelerator_unittest) As discussed to Owen it's a known bug of autotest, and there is "results" dir for vda_unittest to write file if needed. And that dir will also be fetched by host. (/usr/local/autotest/results/default/<test>/results/) For this reason, in order to write thumbnail file on autotest we need the new parameter, otherwise it is no way to write on autotest. Wu-cheng, how do you think?
On 2017/03/21 07:25:13, johnylin1 wrote: > On 2017/03/20 14:22:50, johnylin1 wrote: > > After more investigation, I found there are still issues of writing thumbnail > > png for autotest. > > > > 1. The thumbnail write path for autotest is always the same directory of test > > data, which is /usr/local/autotest/deps/chrome_test/test_src/media/test/data/ > > (before and after CL:https://codereview.chromium.org/2594243002/). So I > believe > > png file never been packed to test results. > > > > 2. Write file still fail for autotest even with this patch. base::WriteFile() > > seems unable to write under /usr/local/autotest/, but ok to write under /tmp/. > > Maybe it's because /tmp/ is mod 777, /usr/local/ is 755? > > But it has no issue for locally running on DUT (Manually run > > video_decode_accelerator_unittest) > > As discussed to Owen it's a known bug of autotest, and there is "results" dir > for vda_unittest to write file if needed. And that dir will also be fetched by > host. > (/usr/local/autotest/results/default/<test>/results/) > > For this reason, in order to write thumbnail file on autotest we need the new > parameter, otherwise it is no way to write on autotest. > > Wu-cheng, how do you think? That's a separate issue. Please make it another CL. I'd prefer not adding more parameters. Can we always write the thumbnail file to results folder? Or can we always try to write to two folders? Ping me to discuss.
On 2017/03/22 05:03:49, wuchengli wrote: > On 2017/03/21 07:25:13, johnylin1 wrote: > > On 2017/03/20 14:22:50, johnylin1 wrote: > > > After more investigation, I found there are still issues of writing > thumbnail > > > png for autotest. > > > > > > 1. The thumbnail write path for autotest is always the same directory of > test > > > data, which is > /usr/local/autotest/deps/chrome_test/test_src/media/test/data/ > > > (before and after CL:https://codereview.chromium.org/2594243002/). So I > > believe > > > png file never been packed to test results. > > > > > > 2. Write file still fail for autotest even with this patch. > base::WriteFile() > > > seems unable to write under /usr/local/autotest/, but ok to write under > /tmp/. > > > Maybe it's because /tmp/ is mod 777, /usr/local/ is 755? > > > But it has no issue for locally running on DUT (Manually run > > > video_decode_accelerator_unittest) > > > > As discussed to Owen it's a known bug of autotest, and there is "results" dir To be correct, it is not bug but intended behavior. > > for vda_unittest to write file if needed. And that dir will also be fetched by > > host. > > (/usr/local/autotest/results/default/<test>/results/) > > > > For this reason, in order to write thumbnail file on autotest we need the new > > parameter, otherwise it is no way to write on autotest. > > > > Wu-cheng, how do you think? > That's a separate issue. Please make it another CL. The reason I don't think it's separate issue is, if we only fix crbug:2594243002, autotest still fail to write png file. So this patch should fix all cases. > I'd prefer not adding more parameters. Can we always > write the thumbnail file to results folder? Or can > we always try to write to two folders? Ping me to discuss. The question is vda_unittest won't know resultsdir, which is established by autotest.
lgtm https://codereview.chromium.org/2753773002/diff/60001/media/gpu/video_decode_... File media/gpu/video_decode_accelerator_unittest.cc (right): https://codereview.chromium.org/2753773002/diff/60001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:210: LOG_IF(FATAL, abs_path.empty()) s/FATAL/ERROR/. Let's not crash the test. It's better to see the failure result and continue the test. https://codereview.chromium.org/2753773002/diff/60001/media/gpu/video_decode_... media/gpu/video_decode_accelerator_unittest.cc:1555: base::DirectoryExists(g_thumbnail_output_dir)) This is multiple lines. Please use braces if () { } else { }
The CQ bit was checked by johnylin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from owenlin@chromium.org, wuchengli@chromium.org Link to the patchset: https://codereview.chromium.org/2753773002/#ps80001 (title: "lgtm nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1490173853201780,
"parent_rev": "0e1ab062c1452c0baa9c9adf3ad9671c268e06fe", "commit_rev":
"58af2e5f92c3c5706d7aba46169001c618684169"}
Message was sent while issue was closed.
Description was changed from ========== Fix failed to write thumbnail png issue In https://codereview.chromium.org/2594243002/, filepath of png output is wrapped to GetTestDataFile(), which will use base::MakeAbsoluteFilePath() to get absolute path. However, since the output file doesn't exist (yet), this function will return an empty FilePath instead and makes write file failed. BUG=701232 TEST=Run Thumbnail/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0 of VDA unittest and make the test matches no md5sum. The test will generate bad_thumbnails.png successfully. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Fix failed to write thumbnail png issue In https://codereview.chromium.org/2594243002/, filepath of png output is wrapped to GetTestDataFile(), which will use base::MakeAbsoluteFilePath() to get absolute path. However, since the output file doesn't exist (yet), this function will return an empty FilePath instead and makes write file failed. BUG=701232 TEST=Run Thumbnail/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0 of VDA unittest and make the test matches no md5sum. The test will generate bad_thumbnails.png successfully. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2753773002 Cr-Commit-Position: refs/heads/master@{#458706} Committed: https://chromium.googlesource.com/chromium/src/+/58af2e5f92c3c5706d7aba461690... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/58af2e5f92c3c5706d7aba461690... |
