Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2556343002:
Add ffmpeg regression tests for multiple issues from M56 roll (Closed)
DescriptionAdd ffmpeg regression tests for multiple issues from M56 roll
Note: Neither I nor chcunningham@ were able to reproduce 666874 with
current msan tooling, though both CF and chcunningham@ confirmed the
fix previously. Perhaps toolchain or sanitizer changes in the interim
have impacted ability to repro this case.
Excepting above, all new tests repro prior to their fix, and no longer
repro on trunk.
For issue 666770, a seek to GetStartTime() was insufficient for repro,
so a _SEEKING version of the test macro was added to obtain
repro.
Added 8b80a219364dd4c4baaa9297005218f43dc5c49f to internal repo.
BUG=666794, 666874, 667063, 666770
R=dalecurtis@chromium.org,chcunningham@chromium.org
Committed: https://crrev.com/0bf26e16060899a224a208cfbc40549fc924f1c0
Cr-Commit-Position: refs/heads/master@{#437331}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Use GetStartTime() instead of base::TimeDelta() #Messages
Total messages: 24 (13 generated)
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Please take a look. Thanks!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Note, I'll have at least one more batch like this in a subsequent CL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM test for 666874 Skimmed the rest - no concerns.
Actually, one nit https://codereview.chromium.org/2556343002/diff/1/media/ffmpeg/ffmpeg_regress... File media/ffmpeg/ffmpeg_regression_tests.cc (right): https://codereview.chromium.org/2556343002/diff/1/media/ffmpeg/ffmpeg_regress... media/ffmpeg/ffmpeg_regression_tests.cc:73: FFMPEG_TEST_CASE_SEEKING(name, fn, init_status, end_status, base::TimeDelta()) I'm wondering about the base::TimeDelta() default. It may have been that GetStartTime() was non-zero for tests before this change, and I think that *might* be important for some regressions to repro? Maybe the safer thing is to default to kNoTimestamp() and then use GetStartTime() in the tests if no real seek time is provided.
https://codereview.chromium.org/2556343002/diff/1/media/ffmpeg/ffmpeg_regress... File media/ffmpeg/ffmpeg_regression_tests.cc (right): https://codereview.chromium.org/2556343002/diff/1/media/ffmpeg/ffmpeg_regress... media/ffmpeg/ffmpeg_regression_tests.cc:385: Seek(GetParam().seek_time); Doesn't this change all the existing tests? Seems like this should keep the old behavior?
https://codereview.chromium.org/2556343002/diff/1/media/ffmpeg/ffmpeg_regress... File media/ffmpeg/ffmpeg_regression_tests.cc (right): https://codereview.chromium.org/2556343002/diff/1/media/ffmpeg/ffmpeg_regress... media/ffmpeg/ffmpeg_regression_tests.cc:73: FFMPEG_TEST_CASE_SEEKING(name, fn, init_status, end_status, base::TimeDelta()) On 2016/12/08 18:34:24, chcunningham wrote: > I'm wondering about the base::TimeDelta() default. It may have been that > GetStartTime() was non-zero for tests before this change, and I think that > *might* be important for some regressions to repro? Maybe the safer thing is to > default to kNoTimestamp() and then use GetStartTime() in the tests if no real > seek time is provided. Good catch! I'll change to default of kNoTimeStamp as a signal for the test to use GetStartTime() at runtime, if it reaches that far in the test. And I'll add some comments about this. https://codereview.chromium.org/2556343002/diff/1/media/ffmpeg/ffmpeg_regress... media/ffmpeg/ffmpeg_regression_tests.cc:385: Seek(GetParam().seek_time); On 2016/12/08 19:07:47, DaleCurtis wrote: > Doesn't this change all the existing tests? Seems like this should keep the old > behavior? Good catch ditto of chcunningham@'s catch. Thank you! (see my reply above. fixed patch set is coming soon...)
Description was changed from ========== Add ffmpeg regression tests for multiple issues from M56 roll Note: Neither I nor chcunningham@ were able to reproduce 666874 with current msan tooling, though both CF and chcunningham@ confirmed the fix previously. Perhaps toolchain or sanitizer changes in the interim have impacted ability to repro this case. Excepting above, all new tests repro prior to their fix, and no longer repro on trunk. For issue 666770, a seek to base::TimeDelta() was insufficient for repro, so a _SEEKING version of the test macro was added to obtain repro. Added 8b80a219364dd4c4baaa9297005218f43dc5c49f to internal repo. BUG=666794,666874,667063,666770 R=dalecurtis@chromium.org,chcunningham@chromium.org ========== to ========== Add ffmpeg regression tests for multiple issues from M56 roll Note: Neither I nor chcunningham@ were able to reproduce 666874 with current msan tooling, though both CF and chcunningham@ confirmed the fix previously. Perhaps toolchain or sanitizer changes in the interim have impacted ability to repro this case. Excepting above, all new tests repro prior to their fix, and no longer repro on trunk. For issue 666770, a seek to GetStartTime() was insufficient for repro, so a _SEEKING version of the test macro was added to obtain repro. Added 8b80a219364dd4c4baaa9297005218f43dc5c49f to internal repo. BUG=666794,666874,667063,666770 R=dalecurtis@chromium.org,chcunningham@chromium.org ==========
The CQ bit was checked by wolenetz@chromium.org to run a CQ dry run
Patch set 2 addresses all comments so far. https://codereview.chromium.org/2556343002/diff/1/media/ffmpeg/ffmpeg_regress... File media/ffmpeg/ffmpeg_regression_tests.cc (right): https://codereview.chromium.org/2556343002/diff/1/media/ffmpeg/ffmpeg_regress... media/ffmpeg/ffmpeg_regression_tests.cc:73: FFMPEG_TEST_CASE_SEEKING(name, fn, init_status, end_status, base::TimeDelta()) On 2016/12/08 19:12:52, wolenetz wrote: > On 2016/12/08 18:34:24, chcunningham wrote: > > I'm wondering about the base::TimeDelta() default. It may have been that > > GetStartTime() was non-zero for tests before this change, and I think that > > *might* be important for some regressions to repro? Maybe the safer thing is > to > > default to kNoTimestamp() and then use GetStartTime() in the tests if no real > > seek time is provided. > > Good catch! I'll change to default of kNoTimeStamp as a signal for the test to > use GetStartTime() at runtime, if it reaches that far in the test. And I'll add > some comments about this. Done. https://codereview.chromium.org/2556343002/diff/1/media/ffmpeg/ffmpeg_regress... media/ffmpeg/ffmpeg_regression_tests.cc:385: Seek(GetParam().seek_time); On 2016/12/08 19:12:52, wolenetz wrote: > On 2016/12/08 19:07:47, DaleCurtis wrote: > > Doesn't this change all the existing tests? Seems like this should keep the > old > > behavior? > > Good catch ditto of chcunningham@'s catch. Thank you! (see my reply above. fixed > patch set is coming soon...) Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by wolenetz@chromium.org
The CQ bit was checked by wolenetz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from chcunningham@chromium.org Link to the patchset: https://codereview.chromium.org/2556343002/#ps20001 (title: "Use GetStartTime() instead of base::TimeDelta()")
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": 20001, "attempt_start_ts": 1481226919983480,
"parent_rev": "e16e24c21ca8598e0cfdb91c2e4558b64c0165f7", "commit_rev":
"4911dec5ee82191e9f31f730ef8c08021a510441"}
Message was sent while issue was closed.
Description was changed from ========== Add ffmpeg regression tests for multiple issues from M56 roll Note: Neither I nor chcunningham@ were able to reproduce 666874 with current msan tooling, though both CF and chcunningham@ confirmed the fix previously. Perhaps toolchain or sanitizer changes in the interim have impacted ability to repro this case. Excepting above, all new tests repro prior to their fix, and no longer repro on trunk. For issue 666770, a seek to GetStartTime() was insufficient for repro, so a _SEEKING version of the test macro was added to obtain repro. Added 8b80a219364dd4c4baaa9297005218f43dc5c49f to internal repo. BUG=666794,666874,667063,666770 R=dalecurtis@chromium.org,chcunningham@chromium.org ========== to ========== Add ffmpeg regression tests for multiple issues from M56 roll Note: Neither I nor chcunningham@ were able to reproduce 666874 with current msan tooling, though both CF and chcunningham@ confirmed the fix previously. Perhaps toolchain or sanitizer changes in the interim have impacted ability to repro this case. Excepting above, all new tests repro prior to their fix, and no longer repro on trunk. For issue 666770, a seek to GetStartTime() was insufficient for repro, so a _SEEKING version of the test macro was added to obtain repro. Added 8b80a219364dd4c4baaa9297005218f43dc5c49f to internal repo. BUG=666794,666874,667063,666770 R=dalecurtis@chromium.org,chcunningham@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Add ffmpeg regression tests for multiple issues from M56 roll Note: Neither I nor chcunningham@ were able to reproduce 666874 with current msan tooling, though both CF and chcunningham@ confirmed the fix previously. Perhaps toolchain or sanitizer changes in the interim have impacted ability to repro this case. Excepting above, all new tests repro prior to their fix, and no longer repro on trunk. For issue 666770, a seek to GetStartTime() was insufficient for repro, so a _SEEKING version of the test macro was added to obtain repro. Added 8b80a219364dd4c4baaa9297005218f43dc5c49f to internal repo. BUG=666794,666874,667063,666770 R=dalecurtis@chromium.org,chcunningham@chromium.org ========== to ========== Add ffmpeg regression tests for multiple issues from M56 roll Note: Neither I nor chcunningham@ were able to reproduce 666874 with current msan tooling, though both CF and chcunningham@ confirmed the fix previously. Perhaps toolchain or sanitizer changes in the interim have impacted ability to repro this case. Excepting above, all new tests repro prior to their fix, and no longer repro on trunk. For issue 666770, a seek to GetStartTime() was insufficient for repro, so a _SEEKING version of the test macro was added to obtain repro. Added 8b80a219364dd4c4baaa9297005218f43dc5c49f to internal repo. BUG=666794,666874,667063,666770 R=dalecurtis@chromium.org,chcunningham@chromium.org Committed: https://crrev.com/0bf26e16060899a224a208cfbc40549fc924f1c0 Cr-Commit-Position: refs/heads/master@{#437331} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/0bf26e16060899a224a208cfbc40549fc924f1c0 Cr-Commit-Position: refs/heads/master@{#437331} |
