This is branch off of Issue 834156 , based on a posciak@s comment/dialogue,
https://crbug.com/834156#c7
> Personally I think we should remove Reset() call from thumbnail tests instead.
>
> Reset()'s behavior is (by design) subject to timings, depending on how fast we can react
> to it and by definition the VDA/codec is free to drop any number of frames at any time
> and by definition there is no one correct order/number of returned frames when Reset()
> is called. So we wouldn't be able to provide a golden reference.
>
> On the other hand, the thumbnail test's goal is to test decoded image correctness, not
> Reset/etc. behavior (other tests do so).
>
> So I think we should preferably remove Reset() calls from thumbnail test to avoid similar
> issues to this in the future instead...
Not trivial because (https://crbug.com834156#c8)
> The way the test harness is written is that it paints the received
> frames and then calculates the MD5 of that, hence the assumption of _all_
> frames being delivered to the client from the VDA is baked in. I'm hoping
> that the fix in the CL makes the bots happy, but I agree that it needs a
> longer term refactoring. CC owenlin@ that I see has worked on the Rendering
> helper (e.g. [3]) code drawing the thumbnails for his opinion, but this
> code seems to come from sabercrombie@ (who left Cr, but I'm cc'ing in the
> hope that they kept their Chromium profile alive).
Finally (https://crbug.com834156#c9)
> We don't have to Reset() for thumbnail test at all. Could we add a new value
> to the ResetPoint enum (e.g. NO_RESET) and pass something else than
> END_OF_STREAM_RESET to the thumbnail test case please (we may transition
> automatically to CS_RESET if needed in that case)? This should eliminate
> the issue with minimal change to the test?
Essentially, the baked in assumptions make this test potentially flaky. This
test "race" can be caused e.g. by running
./video_decode_accelerator_unittest --test_video_data=test-25fps.h264:320:240:250:258:35:150:1 --gtest_filter=Thumbnail
and then
/video_decode_accelerator_unittest --test_video_data=test-25fps.h264:320:240:250:258:35:150:1 --gtest_filter=Thumbnail/* -vmodule=*vaapi*=4
the extra -vmodule is enough to change the timing and produce a different MD5sum
This is a branch off of Issue 834156 , based on a posciak@s comment/dialogue,
https://crbug.com/834156#c7
> Personally I think we should remove Reset() call from thumbnail tests instead.
>
> Reset()'s behavior is (by design) subject to timings, depending on how fast we can react
> to it and by definition the VDA/codec is free to drop any number of frames at any time
> and by definition there is no one correct order/number of returned frames when Reset()
> is called. So we wouldn't be able to provide a golden reference.
>
> On the other hand, the thumbnail test's goal is to test decoded image correctness, not
> Reset/etc. behavior (other tests do so).
>
> So I think we should preferably remove Reset() calls from thumbnail test to avoid similar
> issues to this in the future instead...
Not trivial because (https://crbug.com834156#c8)
> The way the test harness is written is that it paints the received
> frames and then calculates the MD5 of that, hence the assumption of _all_
> frames being delivered to the client from the VDA is baked in. I'm hoping
> that the fix in the CL makes the bots happy, but I agree that it needs a
> longer term refactoring. CC owenlin@ that I see has worked on the Rendering
> helper (e.g. [3]) code drawing the thumbnails for his opinion, but this
> code seems to come from sabercrombie@ (who left Cr, but I'm cc'ing in the
> hope that they kept their Chromium profile alive).
Finally (https://crbug.com834156#c9)
> We don't have to Reset() for thumbnail test at all. Could we add a new value
> to the ResetPoint enum (e.g. NO_RESET) and pass something else than
> END_OF_STREAM_RESET to the thumbnail test case please (we may transition
> automatically to CS_RESET if needed in that case)? This should eliminate
> the issue with minimal change to the test?
Essentially, the baked in assumptions make this test potentially flaky. This
test "race" can be caused e.g. by running
./video_decode_accelerator_unittest --test_video_data=test-25fps.h264:320:240:250:258:35:150:1 --gtest_filter=Thumbnail
and then
/video_decode_accelerator_unittest --test_video_data=test-25fps.h264:320:240:250:258:35:150:1 --gtest_filter=Thumbnail/* -vmodule=*vaapi*=4
the extra -vmodule is enough to change the timing and produce a different MD5sum
This is a branch off of Issue 834156 , based on a posciak@s comment/dialogue,
https://crbug.com/834156#c7
> Personally I think we should remove Reset() call from thumbnail tests instead.
>
> Reset()'s behavior is (by design) subject to timings, depending on how fast we can react
> to it and by definition the VDA/codec is free to drop any number of frames at any time
> and by definition there is no one correct order/number of returned frames when Reset()
> is called. So we wouldn't be able to provide a golden reference.
>
> On the other hand, the thumbnail test's goal is to test decoded image correctness, not
> Reset/etc. behavior (other tests do so).
>
> So I think we should preferably remove Reset() calls from thumbnail test to avoid similar
> issues to this in the future instead...
Not trivial because ( https://crbug.com/834156#c8 )
> The way the test harness is written is that it paints the received
> frames and then calculates the MD5 of that, hence the assumption of _all_
> frames being delivered to the client from the VDA is baked in. I'm hoping
> that the fix in the CL makes the bots happy, but I agree that it needs a
> longer term refactoring. CC owenlin@ that I see has worked on the Rendering
> helper (e.g. [3]) code drawing the thumbnails for his opinion, but this
> code seems to come from sabercrombie@ (who left Cr, but I'm cc'ing in the
> hope that they kept their Chromium profile alive).
Finally ( https://crbug.com/834156#c9 )
> We don't have to Reset() for thumbnail test at all. Could we add a new value
> to the ResetPoint enum (e.g. NO_RESET) and pass something else than
> END_OF_STREAM_RESET to the thumbnail test case please (we may transition
> automatically to CS_RESET if needed in that case)? This should eliminate
> the issue with minimal change to the test?
Essentially, the baked in assumptions make this test potentially flaky. This
test "race" can be caused e.g. by running
./video_decode_accelerator_unittest --test_video_data=test-25fps.h264:320:240:250:258:35:150:1 --gtest_filter=Thumbnail
and then
/video_decode_accelerator_unittest --test_video_data=test-25fps.h264:320:240:250:258:35:150:1 --gtest_filter=Thumbnail/* -vmodule=*vaapi*=4
the extra -vmodule is enough to change the timing and produce a different MD5sum
This is a branch off of Issue 834156 , based on a posciak@s comment/dialogue,
https://crbug.com/834156#c7
> Personally I think we should remove Reset() call from thumbnail tests instead.
>
> Reset()'s behavior is (by design) subject to timings, depending on how fast we can react
> to it and by definition the VDA/codec is free to drop any number of frames at any time
> and by definition there is no one correct order/number of returned frames when Reset()
> is called. So we wouldn't be able to provide a golden reference.
>
> On the other hand, the thumbnail test's goal is to test decoded image correctness, not
> Reset/etc. behavior (other tests do so).
>
> So I think we should preferably remove Reset() calls from thumbnail test to avoid similar
> issues to this in the future instead...
Not trivial because ( https://crbug.com/834156#c8 )
> The way the test harness is written is that it paints the received
> frames and then calculates the MD5 of that, hence the assumption of _all_
> frames being delivered to the client from the VDA is baked in. I'm hoping
> that the fix in the CL makes the bots happy, but I agree that it needs a
> longer term refactoring. CC owenlin@ that I see has worked on the Rendering
> helper (e.g. [3]) code drawing the thumbnails for his opinion, but this
> code seems to come from sabercrombie@ (who left Cr, but I'm cc'ing in the
> hope that they kept their Chromium profile alive).
Finally ( https://crbug.com/834156#c9 )
> We don't have to Reset() for thumbnail test at all. Could we add a new value
> to the ResetPoint enum (e.g. NO_RESET) and pass something else than
> END_OF_STREAM_RESET to the thumbnail test case please (we may transition
> automatically to CS_RESET if needed in that case)? This should eliminate
> the issue with minimal change to the test?
Essentially, the baked in assumptions make this test potentially flaky. This
test "race" can be caused e.g. by running
./video_decode_accelerator_unittest --test_video_data=test-25fps.h264:320:240:250:258:35:150:1 --gtest_filter=Thumbnail/*
and then
/video_decode_accelerator_unittest --test_video_data=test-25fps.h264:320:240:250:258:35:150:1 --gtest_filter=Thumbnail/* -vmodule=*vaapi*=4
the extra -vmodule is enough to change the timing and produce a different MD5sum
Comment 1 by mcasas@chromium.org
, Apr 19 2018