vda unittest: delete GLRenderingVDAClient in correct thread

vda unittest: delete GLRenderingVDAClient in correct thread
    
In VideoDecodeAcceleratorParamTest.TestSimpleDecode there are many ASSERT_xx
while testing, which aborts test and doesn't delete GLRenderingVDAClient in
GetRenderingTaskRunner.
    
1. For thumbnail test, in fact it doesn't need ASSERT for MD5 missing, wrong
format, or comparison failure since it won't break any other things. Change to
EXPECT_xx and LOG(ERROR).
    
2. Delete GLRenderingVDAClient in VideoDecodeAcceleratorParamTest::TearDown().
When ASSERT happens it aborts test immediately and call TearDown(), so we can
make sure GLRenderingVDAClient is deleted.
    
BUG=654677
TEST=test debug version of video_decode_accelerator_unittest on Elm
1. For thumbnail test, try to not push md5 file and the test will fail and end
gracefully. Add an invalid entry in md5 and the test will pass with error
messages.
2. Try to make assertion in any place inside TEST_P. Test will fail and end
gracefully.
CQ_INCLUDE_TRYBOTS=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/2596193005
Cr-Commit-Position: refs/heads/master@{#442835}
Committed: https://chromium.googlesource.com/chromium/src/+/2ea32a808df771b5c5acb53a3819010cf054e021

[johnylin1, 2016-12-23 14:01:58]

Description was changed from

==========
vda unittest: delete GLRenderingVDAClient in correct thread

In VideoDecodeAcceleratorParamTest.TestSimpleDecode there are many ASSERT_xx
while testing, which aborts test and doesn't delete GLRenderingVDAClient in
GetRenderingTaskRunner.

1. For thumbnail test, in fact it doesn't need ASSERT for MD5 missing, wrong
format, or comparison failure since it won't break any other things. Change to
EXPECT_xx or LOG(ERROR), even LOG(WARNING).

2. Use a boolean terminate_early instead to indicate state check error while
decoder running. If terminate_early has been set then the for loop will break
and then delete GLRenderingVDAClient in correct thread and terminate the test.

BUG=654677
TEST=test debug version of video_decode_accelerator_unittest on Elm
1. For thumbnail test, try to not push md5 file and the test will fail and end
gracefully. Add an invalid entry in md5 and the test will pass with warning.
2. Try to force terminate_early = true everywhere in for loop, the test will
fail and end gracefully.
==========

to

==========
vda unittest: delete GLRenderingVDAClient in correct thread

In VideoDecodeAcceleratorParamTest.TestSimpleDecode there are many ASSERT_xx
while testing, which aborts test and doesn't delete GLRenderingVDAClient in
GetRenderingTaskRunner.

1. For thumbnail test, in fact it doesn't need ASSERT for MD5 missing, wrong
format, or comparison failure since it won't break any other things. Change to
EXPECT_xx or LOG(ERROR), even LOG(WARNING).

2. Use a boolean terminate_early instead to indicate state check error while
decoder running. If terminate_early has been set then the for loop will break
and then delete GLRenderingVDAClient in correct thread and terminate the test.

BUG=654677
TEST=test debug version of video_decode_accelerator_unittest on Elm
1. For thumbnail test, try to not push md5 file and the test will fail and end
gracefully. Add an invalid entry in md5 and the test will pass with warning.
2. Try to force terminate_early = true everywhere in for loop, the test will
fail and end gracefully.
CQ_INCLUDE_TRYBOTS=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
==========

[johnylin1, 2016-12-23 14:04:07]

johnylin@chromium.org changed reviewers:
+ owenlin@chromium.org, posciak@chromium.org, wuchengli@chromium.org

[Owen Lin, 2016-12-27 07:32:44]

https://codereview.chromium.org/2596193005/diff/20001/media/gpu/video_decode_...
File media/gpu/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2596193005/diff/20001/media/gpu/video_decode_...
media/gpu/video_decode_accelerator_unittest.cc:215: LOG_IF(ERROR,
md5_strings->size() < 1U) << "  MD5 checksum file ("
== 0
Why this one is error and others are just warning? Incorrect or missing file
doesn't sound so different to me.

https://codereview.chromium.org/2596193005/diff/20001/media/gpu/video_decode_...
media/gpu/video_decode_accelerator_unittest.cc:1413: bool terminate_early =
false;
Instead, can we have a helper function which returns true or false ?

bool VerifyClientState(
    num_concurrent_decoders, num_play_throughts, notes, clients) {

  for (size_t i = 0; i < num_.... ) {
     if (state_check_failed) return false;
 
  }


  return true;
}

https://codereview.chromium.org/2596193005/diff/20001/media/gpu/video_decode_...
media/gpu/video_decode_accelerator_unittest.cc:1463:
DeleteClientsAndNotes(std::move(clients), std::move(notes));
To me, it looks like the main issue here is to call DeleteClientsAndNotes when
we exist the function.

In that case, can we have a helper object to do this?

class ClientAndNotesDeleter {
public:
  ClientAndNotesDeleter(clients, notes);
  ~ClientAndNotesDeleter() {
    DeleteClientsAndNotes();
  }
}

[johnylin1, 2016-12-27 14:02:06]

https://codereview.chromium.org/2596193005/diff/20001/media/gpu/video_decode_...
File media/gpu/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2596193005/diff/20001/media/gpu/video_decode_...
media/gpu/video_decode_accelerator_unittest.cc:215: LOG_IF(ERROR,
md5_strings->size() < 1U) << "  MD5 checksum file ("
On 2016/12/27 07:32:44, Owen Lin wrote:
> == 0
> Why this one is error and others are just warning? Incorrect or missing file
> doesn't sound so different to me.

My opinion is "missing file" will definitely make test fail because there is
nothing to compare (failure will be raised in line 1538). So it deserves an
ERROR.

"Incorrect format" could be only found in one of md5_string in md5 file (that's
say if there is a mis-typed string in md5 file), but it may not affect the test
if we still get other md5_string matched. So I set them to WARNING and the user
may also get noticed there is incorrect entry in md5 file.

[johnylin1, 2016-12-30 12:38:38]

https://codereview.chromium.org/2596193005/diff/20001/media/gpu/video_decode_...
File media/gpu/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2596193005/diff/20001/media/gpu/video_decode_...
media/gpu/video_decode_accelerator_unittest.cc:1463:
DeleteClientsAndNotes(std::move(clients), std::move(notes));
On 2016/12/27 07:32:44, Owen Lin wrote:
> To me, it looks like the main issue here is to call DeleteClientsAndNotes when
> we exist the function.
> 
> In that case, can we have a helper object to do this?
> 
> class ClientAndNotesDeleter {
> public:
>   ClientAndNotesDeleter(clients, notes);
>   ~ClientAndNotesDeleter() {
>     DeleteClientsAndNotes();
>   }
> }
I got another idea is to delete clients and notes in the destructor of
VideoDecodeAcceleratorParamTest. Exiting the test also means
VideoDecodeAcceleratorParamTest gets released.

class VideoDecodeAcceleratorParamTest {
 public:
  ~VideoDecodeAcceleratorParamTest();
 protected:
  NotesVector notes;
  ClientsVector clients;
};

VideoDecodeAcceleratorParamTest::~VideoDecodeAcceleratorParamTest() {
  DeleteClientsAndNotes();
}

[wuchengli, 2017-01-03 09:10:12]

I'll be OOO the rest of this week. Leaving the review to Owen.

https://codereview.chromium.org/2596193005/diff/20001/media/gpu/video_decode_...
File media/gpu/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2596193005/diff/20001/media/gpu/video_decode_...
media/gpu/video_decode_accelerator_unittest.cc:215: LOG_IF(ERROR,
md5_strings->size() < 1U) << "  MD5 checksum file ("
On 2016/12/27 14:02:06, johnylin1 wrote:
> On 2016/12/27 07:32:44, Owen Lin wrote:
> > == 0
> > Why this one is error and others are just warning? Incorrect or missing file
> > doesn't sound so different to me.
> 
> My opinion is "missing file" will definitely make test fail because there is
> nothing to compare (failure will be raised in line 1538). So it deserves an
> ERROR.
> 
> "Incorrect format" could be only found in one of md5_string in md5 file
(that's
> say if there is a mis-typed string in md5 file), but it may not affect the
test
> if we still get other md5_string matched. So I set them to WARNING and the
user
> may also get noticed there is incorrect entry in md5 file.

+1. Use LOG_IF(ERROR, for consistency because it's indeed a bug.

https://codereview.chromium.org/2596193005/diff/20001/media/gpu/video_decode_...
media/gpu/video_decode_accelerator_unittest.cc:1293: void
VideoDecodeAcceleratorParamTest::DeleteClientsAndNotes(
This can be renamed to VideoDecodeAcceleratorParamTest::TearDown and override
VideoDecodeAcceleratorTest::TearDown, which does similar clean-ups. Most code in
TestSimpleDecode and AssertWaitForStateOrDeleted don't need to change.

[johnylin1, 2017-01-04 15:34:37]

https://codereview.chromium.org/2596193005/diff/20001/media/gpu/video_decode_...
File media/gpu/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2596193005/diff/20001/media/gpu/video_decode_...
media/gpu/video_decode_accelerator_unittest.cc:215: LOG_IF(ERROR,
md5_strings->size() < 1U) << "  MD5 checksum file ("
On 2017/01/03 09:10:11, wuchengli wrote:
> On 2016/12/27 14:02:06, johnylin1 wrote:
> > On 2016/12/27 07:32:44, Owen Lin wrote:
> > > == 0
> > > Why this one is error and others are just warning? Incorrect or missing
file
> > > doesn't sound so different to me.
> > 
> > My opinion is "missing file" will definitely make test fail because there is
> > nothing to compare (failure will be raised in line 1538). So it deserves an
> > ERROR.
> > 
> > "Incorrect format" could be only found in one of md5_string in md5 file
> (that's
> > say if there is a mis-typed string in md5 file), but it may not affect the
> test
> > if we still get other md5_string matched. So I set them to WARNING and the
> user
> > may also get noticed there is incorrect entry in md5 file.
> 
> +1. Use LOG_IF(ERROR, for consistency because it's indeed a bug.

Done.

https://codereview.chromium.org/2596193005/diff/20001/media/gpu/video_decode_...
media/gpu/video_decode_accelerator_unittest.cc:1293: void
VideoDecodeAcceleratorParamTest::DeleteClientsAndNotes(
On 2017/01/03 09:10:12, wuchengli wrote:
> This can be renamed to VideoDecodeAcceleratorParamTest::TearDown and override
> VideoDecodeAcceleratorTest::TearDown, which does similar clean-ups. Most code
in
> TestSimpleDecode and AssertWaitForStateOrDeleted don't need to change.

Sounds great. I override TearDown() in VideoDecodeAcceleratorParamTest ,and in
the end call VideoDecodeAcceleratorTest::TearDown() so everything will be
cleaned in the correct order.

https://codereview.chromium.org/2596193005/diff/20001/media/gpu/video_decode_...
media/gpu/video_decode_accelerator_unittest.cc:1413: bool terminate_early =
false;
On 2016/12/27 07:32:44, Owen Lin wrote:
> Instead, can we have a helper function which returns true or false ?
> 
> bool VerifyClientState(
>     num_concurrent_decoders, num_play_throughts, notes, clients) {
> 
>   for (size_t i = 0; i < num_.... ) {
>      if (state_check_failed) return false;
>  
>   }
> 
> 
>   return true;
> }

This is no longer needed since we gonna delete clients in TearDown(). When the
assertion happens, TearDown() will be called.

https://codereview.chromium.org/2596193005/diff/20001/media/gpu/video_decode_...
media/gpu/video_decode_accelerator_unittest.cc:1463:
DeleteClientsAndNotes(std::move(clients), std::move(notes));
On 2016/12/30 12:38:37, johnylin1 wrote:
> On 2016/12/27 07:32:44, Owen Lin wrote:
> > To me, it looks like the main issue here is to call DeleteClientsAndNotes
when
> > we exist the function.
> > 
> > In that case, can we have a helper object to do this?
> > 
> > class ClientAndNotesDeleter {
> > public:
> >   ClientAndNotesDeleter(clients, notes);
> >   ~ClientAndNotesDeleter() {
> >     DeleteClientsAndNotes();
> >   }
> > }
> I got another idea is to delete clients and notes in the destructor of
> VideoDecodeAcceleratorParamTest. Exiting the test also means
> VideoDecodeAcceleratorParamTest gets released.
> 
> class VideoDecodeAcceleratorParamTest {
>  public:
>   ~VideoDecodeAcceleratorParamTest();
>  protected:
>   NotesVector notes;
>   ClientsVector clients;
> };
> 
> VideoDecodeAcceleratorParamTest::~VideoDecodeAcceleratorParamTest() {
>   DeleteClientsAndNotes();
> }

Override TearDown() is even better and also makes everything cleaned in the
correct order.

[johnylin1, 2017-01-04 15:35:48]

Description was changed from

==========
vda unittest: delete GLRenderingVDAClient in correct thread

In VideoDecodeAcceleratorParamTest.TestSimpleDecode there are many ASSERT_xx
while testing, which aborts test and doesn't delete GLRenderingVDAClient in
GetRenderingTaskRunner.

1. For thumbnail test, in fact it doesn't need ASSERT for MD5 missing, wrong
format, or comparison failure since it won't break any other things. Change to
EXPECT_xx or LOG(ERROR), even LOG(WARNING).

2. Use a boolean terminate_early instead to indicate state check error while
decoder running. If terminate_early has been set then the for loop will break
and then delete GLRenderingVDAClient in correct thread and terminate the test.

BUG=654677
TEST=test debug version of video_decode_accelerator_unittest on Elm
1. For thumbnail test, try to not push md5 file and the test will fail and end
gracefully. Add an invalid entry in md5 and the test will pass with warning.
2. Try to force terminate_early = true everywhere in for loop, the test will
fail and end gracefully.
CQ_INCLUDE_TRYBOTS=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

==========
vda unittest: delete GLRenderingVDAClient in correct thread
    
In VideoDecodeAcceleratorParamTest.TestSimpleDecode there are many ASSERT_xx
while testing, which aborts test and doesn't delete GLRenderingVDAClient in
GetRenderingTaskRunner.
    
1. For thumbnail test, in fact it doesn't need ASSERT for MD5 missing, wrong
format, or comparison failure since it won't break any other things. Change to
EXPECT_xx and LOG(ERROR).
    
2. Delete GLRenderingVDAClient in VideoDecodeAcceleratorParamTest::TearDown().
When ASSERT happens it aborts test immediately and call TearDown(), so we can
make sure GLRenderingVDAClient is deleted.
    
BUG=654677
TEST=test debug version of video_decode_accelerator_unittest on Elm
1. For thumbnail test, try to not push md5 file and the test will fail and end
gracefully. Add an invalid entry in md5 and the test will pass with error
messages.
2. Try to make assertion in any place inside TEST_P. Test will fail and end
gracefully.
CQ_INCLUDE_TRYBOTS=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
==========

[Owen Lin, 2017-01-05 05:56:47]

https://codereview.chromium.org/2596193005/diff/40001/media/gpu/video_decode_...
File media/gpu/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2596193005/diff/40001/media/gpu/video_decode_...
media/gpu/video_decode_accelerator_unittest.cc:1296:
std::unique_ptr<ClientsVector> clients2(new ClientsVector);
This part of code looks redundant to me.

How about changing Delete to 

template <typename T>
static void Delete<T>(T item) {
  // item will be dropped in the end of the function scope.
}

So that we can pass it directly?

base::Bind(&Delete<ClientVector>, base::Passed(&client_);

I think this also applied to the only other usage where delete a
TestFilesVector.

[johnylin1, 2017-01-05 06:52:55]

https://codereview.chromium.org/2596193005/diff/40001/media/gpu/video_decode_...
File media/gpu/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2596193005/diff/40001/media/gpu/video_decode_...
media/gpu/video_decode_accelerator_unittest.cc:1296:
std::unique_ptr<ClientsVector> clients2(new ClientsVector);
On 2017/01/05 05:56:46, Owen Lin wrote:
> This part of code looks redundant to me.
> 
> How about changing Delete to 
> 
> template <typename T>
> static void Delete<T>(T item) {
>   // item will be dropped in the end of the function scope.
> }
> 
> So that we can pass it directly?
> 
> base::Bind(&Delete<ClientVector>, base::Passed(&client_);
> 
> I think this also applied to the only other usage where delete a
> TestFilesVector.

I'm not sure if it is intended to do this way before. Delete template was added
in this CL: 
https://codereview.chromium.org/2371783002/patch/140001/150019

And before this CL, it used base::STLDeleteElements() which is already
deprecated, but I guess it was also implemented by unique_ptr

[Owen Lin, 2017-01-06 02:52:27]

On 2017/01/05 06:52:55, johnylin1 wrote:
>
https://codereview.chromium.org/2596193005/diff/40001/media/gpu/video_decode_...
> File media/gpu/video_decode_accelerator_unittest.cc (right):
> 
>
https://codereview.chromium.org/2596193005/diff/40001/media/gpu/video_decode_...
> media/gpu/video_decode_accelerator_unittest.cc:1296:
> std::unique_ptr<ClientsVector> clients2(new ClientsVector);
> On 2017/01/05 05:56:46, Owen Lin wrote:
> > This part of code looks redundant to me.
> > 
> > How about changing Delete to 
> > 
> > template <typename T>
> > static void Delete<T>(T item) {
> >   // item will be dropped in the end of the function scope.
> > }
> > 
> > So that we can pass it directly?
> > 
> > base::Bind(&Delete<ClientVector>, base::Passed(&client_);
> > 
> > I think this also applied to the only other usage where delete a
> > TestFilesVector.

> 
> I'm not sure if it is intended to do this way before. Delete template was
added
> in this CL: 
> https://codereview.chromium.org/2371783002/patch/140001/150019
> 
> And before this CL, it used base::STLDeleteElements() which is already
> deprecated, but I guess it was also implemented by unique_ptr

I don't think there will be any noticeable behavior change for this
modification.
std::move (i.e., base::Passed) will do a similar thing as swap(), we don't need
to do it explicitly.

[johnylin1, 2017-01-09 07:11:05]

https://codereview.chromium.org/2596193005/diff/40001/media/gpu/video_decode_...
File media/gpu/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2596193005/diff/40001/media/gpu/video_decode_...
media/gpu/video_decode_accelerator_unittest.cc:1296:
std::unique_ptr<ClientsVector> clients2(new ClientsVector);
On 2017/01/05 06:52:55, johnylin1 wrote:
> On 2017/01/05 05:56:46, Owen Lin wrote:
> > This part of code looks redundant to me.
> > 
> > How about changing Delete to 
> > 
> > template <typename T>
> > static void Delete<T>(T item) {
> >   // item will be dropped in the end of the function scope.
> > }
> > 
> > So that we can pass it directly?
> > 
> > base::Bind(&Delete<ClientVector>, base::Passed(&client_);
> > 
> > I think this also applied to the only other usage where delete a
> > TestFilesVector.
> 
> I'm not sure if it is intended to do this way before. Delete template was
added
> in this CL: 
> https://codereview.chromium.org/2371783002/patch/140001/150019
> 
> And before this CL, it used base::STLDeleteElements() which is already
> deprecated, but I guess it was also implemented by unique_ptr

I think Owen is right. base::Passed() will do std::move so for std::vector we
don't need to wrap an additional unique_ptr then.

https://codereview.chromium.org/2596193005/diff/60001/media/gpu/video_decode_...
File media/gpu/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2596193005/diff/60001/media/gpu/video_decode_...
media/gpu/video_decode_accelerator_unittest.cc:892: !decoder_deleted()) {
Hi Pawel, we found that the deletion of current GLRenderingVDAClient might hit
the LOG_ASSERT above with the situation:

1. GLRenderingVDAClient decoder is decoding, ClientState is 4 for example.
2. Assertion raised, GLRenderingVDAClient destructor is called, then
DeleteDecoder() is called.
https://cs.chromium.org/chromium/src/media/gpu/video_decode_accelerator_unitt...
3. There is a ClientState cascade in the end of DeleteDecoder(), so SetState()
are called with states after 4 (5,6,7).
https://cs.chromium.org/chromium/src/media/gpu/video_decode_accelerator_unitt...
4. If delete_decoder_state_ is 5, SetState() will try to delete decoder if
SetState(5) is called, but in fact decoder is already deleted.
https://cs.chromium.org/chromium/src/media/gpu/video_decode_accelerator_unitt...

So we feel that LOG_ASSERT(!decoder_deleted()) should not be here, what do you
think?

Thanks

[wuchengli, 2017-01-10 10:14:02]

On 2017/01/09 07:11:05, johnylin1 wrote:
>
https://codereview.chromium.org/2596193005/diff/40001/media/gpu/video_decode_...
> File media/gpu/video_decode_accelerator_unittest.cc (right):
> 
>
https://codereview.chromium.org/2596193005/diff/40001/media/gpu/video_decode_...
> media/gpu/video_decode_accelerator_unittest.cc:1296:
> std::unique_ptr<ClientsVector> clients2(new ClientsVector);
> On 2017/01/05 06:52:55, johnylin1 wrote:
> > On 2017/01/05 05:56:46, Owen Lin wrote:
> > > This part of code looks redundant to me.
> > > 
> > > How about changing Delete to 
> > > 
> > > template <typename T>
> > > static void Delete<T>(T item) {
> > >   // item will be dropped in the end of the function scope.
> > > }
> > > 
> > > So that we can pass it directly?
> > > 
> > > base::Bind(&Delete<ClientVector>, base::Passed(&client_);
> > > 
> > > I think this also applied to the only other usage where delete a
> > > TestFilesVector.
> > 
> > I'm not sure if it is intended to do this way before. Delete template was
> added
> > in this CL: 
> > https://codereview.chromium.org/2371783002/patch/140001/150019
> > 
> > And before this CL, it used base::STLDeleteElements() which is already
> > deprecated, but I guess it was also implemented by unique_ptr
> 
> I think Owen is right. base::Passed() will do std::move so for std::vector we
> don't need to wrap an additional unique_ptr then.
> 
>
https://codereview.chromium.org/2596193005/diff/60001/media/gpu/video_decode_...
> File media/gpu/video_decode_accelerator_unittest.cc (right):
> 
>
https://codereview.chromium.org/2596193005/diff/60001/media/gpu/video_decode_...
> media/gpu/video_decode_accelerator_unittest.cc:892: !decoder_deleted()) {
> Hi Pawel, we found that the deletion of current GLRenderingVDAClient might hit
> the LOG_ASSERT above with the situation:
> 
> 1. GLRenderingVDAClient decoder is decoding, ClientState is 4 for example.
> 2. Assertion raised, GLRenderingVDAClient destructor is called, then
> DeleteDecoder() is called.
>
https://cs.chromium.org/chromium/src/media/gpu/video_decode_accelerator_unitt...
> 3. There is a ClientState cascade in the end of DeleteDecoder(), so SetState()
> are called with states after 4 (5,6,7).
>
https://cs.chromium.org/chromium/src/media/gpu/video_decode_accelerator_unitt...
> 4. If delete_decoder_state_ is 5, SetState() will try to delete decoder if
> SetState(5) is called, but in fact decoder is already deleted.
>
https://cs.chromium.org/chromium/src/media/gpu/video_decode_accelerator_unitt...
> 
> So we feel that LOG_ASSERT(!decoder_deleted()) should not be here, what do you
> think?
> 
> Thanks
I discussed with Johny. It's bad for two functions to call each other (SetState
and DeleteDecoder). We decided to make DeleteDecoder not call SetState.

[johnylin1, 2017-01-10 13:53:51]

On 2017/01/10 10:14:02, wuchengli wrote:
> On 2017/01/09 07:11:05, johnylin1 wrote:
> >
>
https://codereview.chromium.org/2596193005/diff/40001/media/gpu/video_decode_...
> > File media/gpu/video_decode_accelerator_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2596193005/diff/40001/media/gpu/video_decode_...
> > media/gpu/video_decode_accelerator_unittest.cc:1296:
> > std::unique_ptr<ClientsVector> clients2(new ClientsVector);
> > On 2017/01/05 06:52:55, johnylin1 wrote:
> > > On 2017/01/05 05:56:46, Owen Lin wrote:
> > > > This part of code looks redundant to me.
> > > > 
> > > > How about changing Delete to 
> > > > 
> > > > template <typename T>
> > > > static void Delete<T>(T item) {
> > > >   // item will be dropped in the end of the function scope.
> > > > }
> > > > 
> > > > So that we can pass it directly?
> > > > 
> > > > base::Bind(&Delete<ClientVector>, base::Passed(&client_);
> > > > 
> > > > I think this also applied to the only other usage where delete a
> > > > TestFilesVector.
> > > 
> > > I'm not sure if it is intended to do this way before. Delete template was
> > added
> > > in this CL: 
> > > https://codereview.chromium.org/2371783002/patch/140001/150019
> > > 
> > > And before this CL, it used base::STLDeleteElements() which is already
> > > deprecated, but I guess it was also implemented by unique_ptr
> > 
> > I think Owen is right. base::Passed() will do std::move so for std::vector
we
> > don't need to wrap an additional unique_ptr then.
> > 
> >
>
https://codereview.chromium.org/2596193005/diff/60001/media/gpu/video_decode_...
> > File media/gpu/video_decode_accelerator_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2596193005/diff/60001/media/gpu/video_decode_...
> > media/gpu/video_decode_accelerator_unittest.cc:892: !decoder_deleted()) {
> > Hi Pawel, we found that the deletion of current GLRenderingVDAClient might
hit
> > the LOG_ASSERT above with the situation:
> > 
> > 1. GLRenderingVDAClient decoder is decoding, ClientState is 4 for example.
> > 2. Assertion raised, GLRenderingVDAClient destructor is called, then
> > DeleteDecoder() is called.
> >
>
https://cs.chromium.org/chromium/src/media/gpu/video_decode_accelerator_unitt...
> > 3. There is a ClientState cascade in the end of DeleteDecoder(), so
SetState()
> > are called with states after 4 (5,6,7).
> >
>
https://cs.chromium.org/chromium/src/media/gpu/video_decode_accelerator_unitt...
> > 4. If delete_decoder_state_ is 5, SetState() will try to delete decoder if
> > SetState(5) is called, but in fact decoder is already deleted.
> >
>
https://cs.chromium.org/chromium/src/media/gpu/video_decode_accelerator_unitt...
> > 
> > So we feel that LOG_ASSERT(!decoder_deleted()) should not be here, what do
you
> > think?
> > 
> > Thanks
> I discussed with Johny. It's bad for two functions to call each other
(SetState
> and DeleteDecoder). We decided to make DeleteDecoder not call SetState.

Done. But one thing I feel kind of ugly after this change. Please see below.

[johnylin1, 2017-01-10 14:01:55]

https://codereview.chromium.org/2596193005/diff/80001/media/gpu/video_decode_...
File media/gpu/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2596193005/diff/80001/media/gpu/video_decode_...
media/gpu/video_decode_accelerator_unittest.cc:901: for (int i =
delete_decoder_state_ + 1; i < CS_MAX; ++i)
This change makes us need to call SetState() inside SetState(), although it can
be guaranteed there is no endless recursive possibility, it still looks not
pretty.

One thought is we could do this way:

void GLRenderingVDAClient::SetState(ClientState new_state) {
  note_->Notify(new_state);
  state_ = new_state;
  if (!remaining_play_throughs_ && new_state == delete_decoder_state_) {
    LOG_ASSERT(!decoder_deleted());
    DeleteDecoder();
    // Cascade through the rest of the states to simplify test code below.
    for (int i = delete_decoder_state_ + 1; i < CS_MAX; ++i) {
      note_->Notify(static_cast<ClientState>(i));
      state_ = static_cast<ClientState>(i);
    }
  }
}

We can avoid any recursion but spending more lines.

[wuchengli, 2017-01-11 03:34:03]

On 2017/01/10 14:01:55, johnylin1 wrote:
>
https://codereview.chromium.org/2596193005/diff/80001/media/gpu/video_decode_...
> File media/gpu/video_decode_accelerator_unittest.cc (right):
> 
>
https://codereview.chromium.org/2596193005/diff/80001/media/gpu/video_decode_...
> media/gpu/video_decode_accelerator_unittest.cc:901: for (int i =
> delete_decoder_state_ + 1; i < CS_MAX; ++i)
> This change makes us need to call SetState() inside SetState(), although it
can
> be guaranteed there is no endless recursive possibility, it still looks not
> pretty.
> 
> One thought is we could do this way:
> 
> void GLRenderingVDAClient::SetState(ClientState new_state) {
>   note_->Notify(new_state);
>   state_ = new_state;
>   if (!remaining_play_throughs_ && new_state == delete_decoder_state_) {
>     LOG_ASSERT(!decoder_deleted());
>     DeleteDecoder();
>     // Cascade through the rest of the states to simplify test code below.
>     for (int i = delete_decoder_state_ + 1; i < CS_MAX; ++i) {
>       note_->Notify(static_cast<ClientState>(i));
>       state_ = static_cast<ClientState>(i);
>     }
>   }
> }
> 
> We can avoid any recursion but spending more lines.
Can we removing all "cascading states"? If we return from
AssertWaitForStateOrDeleted if decoder is deleted, we don't need it.

[johnylin1, 2017-01-11 07:30:59]

On 2017/01/11 03:34:03, wuchengli wrote:
> On 2017/01/10 14:01:55, johnylin1 wrote:
> >
>
https://codereview.chromium.org/2596193005/diff/80001/media/gpu/video_decode_...
> > File media/gpu/video_decode_accelerator_unittest.cc (right):
> > 
> >
>
https://codereview.chromium.org/2596193005/diff/80001/media/gpu/video_decode_...
> > media/gpu/video_decode_accelerator_unittest.cc:901: for (int i =
> > delete_decoder_state_ + 1; i < CS_MAX; ++i)
> > This change makes us need to call SetState() inside SetState(), although it
> can
> > be guaranteed there is no endless recursive possibility, it still looks not
> > pretty.
> > 
> > One thought is we could do this way:
> > 
> > void GLRenderingVDAClient::SetState(ClientState new_state) {
> >   note_->Notify(new_state);
> >   state_ = new_state;
> >   if (!remaining_play_throughs_ && new_state == delete_decoder_state_) {
> >     LOG_ASSERT(!decoder_deleted());
> >     DeleteDecoder();
> >     // Cascade through the rest of the states to simplify test code below.
> >     for (int i = delete_decoder_state_ + 1; i < CS_MAX; ++i) {
> >       note_->Notify(static_cast<ClientState>(i));
> >       state_ = static_cast<ClientState>(i);
> >     }
> >   }
> > }
> > 
> > We can avoid any recursion but spending more lines.
> Can we removing all "cascading states"? If we return from
> AssertWaitForStateOrDeleted if decoder is deleted, we don't need it.
Done. We add a pre-check in AssertWaitForStateOrDeleted to skip if
decoder_deleted()

And set state to CS_DESTROYED in the end of DeleteDecoder().

[wuchengli, 2017-01-11 07:39:12]

https://codereview.chromium.org/2596193005/diff/100001/media/gpu/video_decode...
File media/gpu/video_decode_accelerator_unittest.cc (right):

https://codereview.chromium.org/2596193005/diff/100001/media/gpu/video_decode...
media/gpu/video_decode_accelerator_unittest.cc:1314: // Skip waiting state if
decoder is already deleted of |client|.
decoder of |client| is already deleted.

[wuchengli, 2017-01-11 07:39:19]

lgtm

[johnylin1, 2017-01-11 08:34:21]

The CQ bit was checked by johnylin@chromium.org

[johnylin1, 2017-01-11 08:34:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
wuchengli@chromium.org
Link to the patchset: https://codereview.chromium.org/2596193005/#ps140001
(title: "vda unittest: delete GLRenderingVDAClient in correct thread")

[commit-bot: I haz the power, 2017-01-11 08:34:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 10:06:34]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484123661974720,
"parent_rev": "fa7174c16c023fa43f13fcafe213952a471866b9", "commit_rev":
"2ea32a808df771b5c5acb53a3819010cf054e021"}

[commit-bot: I haz the power, 2017-01-11 10:07:03]

Description was changed from

==========
vda unittest: delete GLRenderingVDAClient in correct thread
    
In VideoDecodeAcceleratorParamTest.TestSimpleDecode there are many ASSERT_xx
while testing, which aborts test and doesn't delete GLRenderingVDAClient in
GetRenderingTaskRunner.
    
1. For thumbnail test, in fact it doesn't need ASSERT for MD5 missing, wrong
format, or comparison failure since it won't break any other things. Change to
EXPECT_xx and LOG(ERROR).
    
2. Delete GLRenderingVDAClient in VideoDecodeAcceleratorParamTest::TearDown().
When ASSERT happens it aborts test immediately and call TearDown(), so we can
make sure GLRenderingVDAClient is deleted.
    
BUG=654677
TEST=test debug version of video_decode_accelerator_unittest on Elm
1. For thumbnail test, try to not push md5 file and the test will fail and end
gracefully. Add an invalid entry in md5 and the test will pass with error
messages.
2. Try to make assertion in any place inside TEST_P. Test will fail and end
gracefully.
CQ_INCLUDE_TRYBOTS=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

==========
vda unittest: delete GLRenderingVDAClient in correct thread
    
In VideoDecodeAcceleratorParamTest.TestSimpleDecode there are many ASSERT_xx
while testing, which aborts test and doesn't delete GLRenderingVDAClient in
GetRenderingTaskRunner.
    
1. For thumbnail test, in fact it doesn't need ASSERT for MD5 missing, wrong
format, or comparison failure since it won't break any other things. Change to
EXPECT_xx and LOG(ERROR).
    
2. Delete GLRenderingVDAClient in VideoDecodeAcceleratorParamTest::TearDown().
When ASSERT happens it aborts test immediately and call TearDown(), so we can
make sure GLRenderingVDAClient is deleted.
    
BUG=654677
TEST=test debug version of video_decode_accelerator_unittest on Elm
1. For thumbnail test, try to not push md5 file and the test will fail and end
gracefully. Add an invalid entry in md5 and the test will pass with error
messages.
2. Try to make assertion in any place inside TEST_P. Test will fail and end
gracefully.
CQ_INCLUDE_TRYBOTS=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/2596193005
Cr-Commit-Position: refs/heads/master@{#442835}
Committed:
https://chromium.googlesource.com/chromium/src/+/2ea32a808df771b5c5acb53a3819...
==========

[commit-bot: I haz the power, 2017-01-11 10:07:04]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/2ea32a808df771b5c5acb53a3819...