It is worth noting that h264 is failed but vp8 is still passed.
[H264]
$ ./video_decode_accelerator_unittest --test_video_data=test-25fps.h264:320:240:250:258:35:150:1 --ozone-platform=gbm --gtest_filter=Thumbnail/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0
[VP8]
$ ./video_decode_accelerator_unittest --test_video_data=test-25fps.vp8:320:240:250:250:35:150:11 --ozone-platform=gbm --gtest_filter=Thumbnail/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0

hiroh@ thanks for the images. 

I see that the series labeled "bad" has frame IDs:

..., 240, 241, 142, 143, ... 150,...

whereas the "good" ones has frame IDs:

..., 241, 249, 150, 151,...

This is another symptom of the change mentioned in the unittests ([1]),
namely, that on TOT (i.e. with the CL) we discard Pictures that the Client
has not been notified upon Reset() ([2] and [3]), whereas before the CL we 
still went ahead and decoded them. 

So, we should update the thumbnails.



[1] https://cs.chromium.org/chromium/src/media/gpu/video_decode_accelerator_unittest.cc?sq=package:chromium&dr=C&l=1478
[2] https://cs.chromium.org/chromium/src/media/gpu/vaapi/vaapi_video_decode_accelerator.cc?sq=package:chromium&dr=C&l=720
[3] https://cs.chromium.org/chromium/src/media/gpu/vaapi/vaapi_video_decode_accelerator.cc?sq=package:chromium&dr=C&l=242

Just for completion I downloaded the images from the Issue description
and I'm uploading them here.

I figured that VP9_2 MD5 had not been updated in the recent past, probably
lost in a sea of red tests. Will do at the same time.

Deleted: test-25fps.h264.good_thumbnails.png 212 KB

	test-25fps.h264.good_thumbnails.png 212 KB View Download

Deleted: test-25fps.h264.bad_thumbnails.png 217 KB

	test-25fps.h264.bad_thumbnails.png 217 KB View Download

Deleted: difference.png 239 KB

	difference.png 239 KB View Download

@3: Thanks for looking into this! Could you clarify what do you suggest should be done please?

#5: there's a CL ready updating the MD5 values [1] following local repro,
SG ?

VP9_2 is in an even easier situation since it just needed MD5 update
but there's nothing fancy on the series numbering etc.

[1] https://crrev.com/c/1018020

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...

#7: it's a bit more complicated than that: the Reset() comes at the
end of the test (Thumbnail/VideoDecodeAcceleratorParamTest.TestSimpleDecode/0)
which makes us drop a number of frames (again because [1] and [2]).
E.g. on H264, we drop 9 frames upon Reset(), because they are being
blitted or waiting to be sent to the Client via PictureReady().

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).


[1] https://cs.chromium.org/chromium/src/media/gpu/vaapi/vaapi_video_decode_accelerator.cc?sq=package:chromium&dr=C&l=720
[2] https://cs.chromium.org/chromium/src/media/gpu/vaapi/vaapi_video_decode_accelerator.cc?sq=package:chromium&dr=C&l=242
[3] https://chromium.googlesource.com/chromium/src/+/2d48653d11bd3856561d73532218fbcad4ce267a

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?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ae42eaef5241e661d5abbb86caf6de01852b0896

commit ae42eaef5241e661d5abbb86caf6de01852b0896
Author: Miguel Casas <mcasas@chromium.org>
Date: Thu Apr 19 03:38:51 2018

vda unittests: correct MD5SUM values

This CL corrects the MD5 values for two video_decode_accelerator_unittest
cases:

- H264: as discussed in the bug  https://crbug.com/834156 #3, after moving the
 Blit to a background thread, Reset() becomes more responsive and the produced
 frame sequence jumps faster to the new values.

- VP9 Profile 2: the last round of MD5 updates missed this one (causing the
 autotest runs to be red; this was not obvious due to other test issues). This
 CL simply updates this value.

Bug:  834156 
Change-Id: I86aee496c3f8e4f2cffc9fe0c5028dff658e2289
Reviewed-on: https://chromium-review.googlesource.com/1018020
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551937}
[modify] https://crrev.com/ae42eaef5241e661d5abbb86caf6de01852b0896/media/test/data/test-25fps.h264.md5
[modify] https://crrev.com/ae42eaef5241e661d5abbb86caf6de01852b0896/media/test/data/test-25fps.vp9_2.md5

Marking as Fixed - leaving a NextAction to verify in ~7days

Issue 834946 tracks fixing the test as requested in 
https://chromium-review.googlesource.com/c/chromium/src/+/1018020#message-56fa32bec144e85b9113d9d49a7f812e90a33a9c

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cc53eec9d209f0235a6b119bfd011c22e71fbb98

commit cc53eec9d209f0235a6b119bfd011c22e71fbb98
Author: Miguel Casas <mcasas@chromium.org>
Date: Thu Apr 19 19:44:02 2018

Revert "vda unittests: correct MD5SUM values"

This reverts commit ae42eaef5241e661d5abbb86caf6de01852b0896.

Reason for revert: a regression tracked in  https://crbug.com/834146 
moved the ToT a few CLs back, so the changes to h264.md5 should be
undone. (I'll land those to VP9_2 in a different CL now).

Original change's description:
> vda unittests: correct MD5SUM values
> 
> This CL corrects the MD5 values for two video_decode_accelerator_unittest
> cases:
> 
> - H264: as discussed in the bug  https://crbug.com/834156 #3, after moving the
>  Blit to a background thread, Reset() becomes more responsive and the produced
>  frame sequence jumps faster to the new values.
> 
> - VP9 Profile 2: the last round of MD5 updates missed this one (causing the
>  autotest runs to be red; this was not obvious due to other test issues). This
>  CL simply updates this value.
> 
> Bug:  834156 
> Change-Id: I86aee496c3f8e4f2cffc9fe0c5028dff658e2289
> Reviewed-on: https://chromium-review.googlesource.com/1018020
> Reviewed-by: Dan Sanders <sandersd@chromium.org>
> Reviewed-by: Kristian H. Kristensen <hoegsberg@chromium.org>
> Commit-Queue: Miguel Casas <mcasas@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#551937}

TBR=mcasas@chromium.org,sandersd@chromium.org,hoegsberg@chromium.org

Change-Id: I3552ab0150f8b766dae4141078a43578b8ef6fcb
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  834156 
Reviewed-on: https://chromium-review.googlesource.com/1020006
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552123}
[modify] https://crrev.com/cc53eec9d209f0235a6b119bfd011c22e71fbb98/media/test/data/test-25fps.h264.md5
[modify] https://crrev.com/cc53eec9d209f0235a6b119bfd011c22e71fbb98/media/test/data/test-25fps.vp9_2.md5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5e239d3f067aa46512615434149b22d954061e92

commit 5e239d3f067aa46512615434149b22d954061e92
Author: Miguel Casas <mcasas@chromium.org>
Date: Thu Apr 19 22:15:16 2018

vda unittests: correct MD5SUM value for VP9 Profile 2

This CL is a refry of crrev.com/c/1018020, but including only the VP9.2 MD5SUM.

"Original" CL description ----------------------------------------------
This CL corrects the MD5 values for two video_decode_accelerator_unittest
cases:
[ ... ]
- VP9 Profile 2: the last round of MD5 updates missed this one (causing the
 autotest runs to be red; this was not obvious due to other test issues). This
 CL simply updates this value.
Bug:  834156 


TBR=hoegsberg@chromium.org, since it's a trivial part of an already landed CL.

Bug:  834954 
Change-Id: I19c7c59de437a99c6e29d42020bc0993619f804b
Reviewed-on: https://chromium-review.googlesource.com/1020024
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552181}
[modify] https://crrev.com/5e239d3f067aa46512615434149b22d954061e92/media/test/data/test-25fps.vp9_2.md5

Latest eve run I see is happy re. video_VideoDecodeAccelerator:

https://stainless.corp.google.com/search?exclude_retried=true&first_date=2018-04-16&master_builder_name=&builder_name_number=&shard=&exclude_acts=true&builder_name=&master_builder_name_number=&owner=&retry=&exclude_cts=false&exclude_non_production=true&hostname=&board=%5Eeve%24&test=video_VideoDecodeAccelerator&exclude_not_run=false&build=%5ER68%5C-10602%5C.0%5C.0%24&status=GOOD&reason=&waterfall=&suite=&last_date=2018-04-22&exclude_non_release=false&exclude_au=true&model=&view=list

crrev.com/c/947341 change seems to break video_VDAPerf due to the lack of the number of decoded frames, although it was gone thanks to reverting the change. 
https://cros-goldeneye.corp.google.com/chromeos/healthmonitoring/testDetails?testName=video_VDAPerf

I will update again md5sum for h264 on ivybridge and baytrail. It was changed due to  crbug.com/827032 .
https://cros-goldeneye.corp.google.com/chromeos/healthmonitoring/testDetails?testName=video_VideoDecodeAccelerator&suite&daysBack=7&version&milestone

Reopen because merging #16 to M67 is required.

Hi, TPM, may I ask to approve merging #16 to M67?

Thanks

This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Create cherry-picked CL.
https://chromium-review.googlesource.com/c/chromium/src/+/1027350

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/487066c750c13fcf8ba0554a70cab86008989bb8

commit 487066c750c13fcf8ba0554a70cab86008989bb8
Author: Miguel Casas <mcasas@chromium.org>
Date: Wed Apr 25 12:55:18 2018

vda unittests: correct MD5SUM value for VP9 Profile 2

This CL is a refry of crrev.com/c/1018020, but including only the VP9.2 MD5SUM.

"Original" CL description ----------------------------------------------
This CL corrects the MD5 values for two video_decode_accelerator_unittest
cases:
[ ... ]
- VP9 Profile 2: the last round of MD5 updates missed this one (causing the
 autotest runs to be red; this was not obvious due to other test issues). This
 CL simply updates this value.
Bug:  834156 


TBR=hoegsberg@chromium.org, since it's a trivial part of an already landed CL.

Bug:  834954 
Change-Id: I19c7c59de437a99c6e29d42020bc0993619f804b
Reviewed-on: https://chromium-review.googlesource.com/1020024
Reviewed-by: Miguel Casas <mcasas@chromium.org>
Commit-Queue: Miguel Casas <mcasas@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#552181}(cherry picked from commit 5e239d3f067aa46512615434149b22d954061e92)
Reviewed-on: https://chromium-review.googlesource.com/1027350
Cr-Commit-Position: refs/branch-heads/3396@{#285}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[modify] https://crrev.com/487066c750c13fcf8ba0554a70cab86008989bb8/media/test/data/test-25fps.vp9_2.md5

TPM: merge-CL requested in #20 is a trivial test data correction, 
merged already.

Confused on #20 and #24.  Did this already get merged?  Which CL is the merge request targeting?

#16 is the CL landing on ToT, and #23 is the same CL landed on the M67 branch
(refs/branch-heads/3396).

Yes. I asked to approved and created to cherry-picked CL.
mcasas merged it before approval, since the CL is obvious.