Updates to VCMDecodedFrameCallback, VideoReceiver and a few related classes/tests.

Updates to VCMDecodedFrameCallback, VideoReceiver and a few related classes/tests.

* The _receiveCallback member of VCMDecodedFrameCallback does actually not require locking now that the threading model is slightly clearer. Documentation and checks have been added.
* UserReceiveCallback() never returns null and must always be called on the decoder thread. Checks have been added and the two test suites that were failing to set this callback, have been fixed and a new mock class added.  (looks like sakal@ may have hit some issues with flaky tests there).
* Changed VcmPayloadSink to use move semantics which I suspect was the intention at the time the code was written (when we didn't have move semantics).
* Added thread checker to a couple of classes and started adding thread checks for known behavior.  There's more to be  done there.
* Remove the |_decoder| member variable in VideoReceiver. It is not needed and as it could be used, left us open to a race.
* TODOs added for places where we can reduce locking. I suspect that we can get away with not needing a lock around _codecDataBase in VideoReceiver once we've got a clear picture of the threading model and ensured that all adhere to it.

BUG=webrtc:7328
Review-Url: https://codereview.webrtc.org/2744013002
Cr-Commit-Position: refs/heads/master@{#17226}
Committed: https://chromium.googlesource.com/external/webrtc/+/d0a71ba1ae69f73025e7381fa7e5593b3a0f7833

[tommi, 2017-03-10 19:52:41]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 19:52:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-10 19:57:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 19:57:17]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/14724)

[tommi, 2017-03-10 20:34:16]

More comments checks, remove _decoder

[tommi, 2017-03-10 20:34:24]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 20:34:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-10 20:38:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 20:38:59]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/14728)

[tommi, 2017-03-10 20:45:28]

Update checks and docs

[tommi, 2017-03-10 20:45:32]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 20:45:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-10 20:50:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 20:50:10]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/14729)

[tommi, 2017-03-10 23:13:37]

Add mock for receive callback and set it for a few tests that didn't

[tommi, 2017-03-10 23:13:43]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 23:13:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-10 23:18:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 23:18:25]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/14735)

[tommi, 2017-03-10 23:32:48]

Format + assert->DCHECK and require callback in decode

[tommi, 2017-03-10 23:33:02]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 23:33:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-10 23:37:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 23:37:58]

Dry run: Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/14739)

[tommi, 2017-03-11 10:23:47]

Description was changed from

==========
Add thread checks and remove lock for _receiveCallback

BUG=
==========

to

==========
Add thread checks and remove lock for _receiveCallback.

* The _receiveCallback member of VCMDecodedFrameCallback does actually not
require locking now that the threading model is slightly clearer. Documentation
and checks have been added.
* UserReceiveCallback() never returns null and must always be called on the
decoder thread. Checks have been added and the two test suites that were failing
to set this callback, have been fixed and a new mock class added.  (looks like
sakal@ may have hit some issues with flaky tests there).
* Changed VcmPayloadSink to use move semantics which I suspect was the intention
at the time the code was written (when we didn't have move semantics).
* Added thread checker to a couple of classes and started adding thread checks
for known behavior.  There's more to be  done there.
* Remove the |_decoder| member variable in VideoReceiver. It is not needed and
as it could be used, left us open to a race.
* TODOs added for places where we can reduce locking. I suspect that we can get
away with not needing a lock around _codecDataBase in VideoReceiver once we've
got a clear picture of the threading model and ensured that all adhere to it.

BUG=
==========

[tommi, 2017-03-11 10:24:18]

Description was changed from

==========
Add thread checks and remove lock for _receiveCallback.

* The _receiveCallback member of VCMDecodedFrameCallback does actually not
require locking now that the threading model is slightly clearer. Documentation
and checks have been added.
* UserReceiveCallback() never returns null and must always be called on the
decoder thread. Checks have been added and the two test suites that were failing
to set this callback, have been fixed and a new mock class added.  (looks like
sakal@ may have hit some issues with flaky tests there).
* Changed VcmPayloadSink to use move semantics which I suspect was the intention
at the time the code was written (when we didn't have move semantics).
* Added thread checker to a couple of classes and started adding thread checks
for known behavior.  There's more to be  done there.
* Remove the |_decoder| member variable in VideoReceiver. It is not needed and
as it could be used, left us open to a race.
* TODOs added for places where we can reduce locking. I suspect that we can get
away with not needing a lock around _codecDataBase in VideoReceiver once we've
got a clear picture of the threading model and ensured that all adhere to it.

BUG=
==========

to

==========
Updates to VCMDecodedFrameCallback, VideoReceiver and a few related
classes/tests.

* The _receiveCallback member of VCMDecodedFrameCallback does actually not
require locking now that the threading model is slightly clearer. Documentation
and checks have been added.
* UserReceiveCallback() never returns null and must always be called on the
decoder thread. Checks have been added and the two test suites that were failing
to set this callback, have been fixed and a new mock class added.  (looks like
sakal@ may have hit some issues with flaky tests there).
* Changed VcmPayloadSink to use move semantics which I suspect was the intention
at the time the code was written (when we didn't have move semantics).
* Added thread checker to a couple of classes and started adding thread checks
for known behavior.  There's more to be  done there.
* Remove the |_decoder| member variable in VideoReceiver. It is not needed and
as it could be used, left us open to a race.
* TODOs added for places where we can reduce locking. I suspect that we can get
away with not needing a lock around _codecDataBase in VideoReceiver once we've
got a clear picture of the threading model and ensured that all adhere to it.

BUG=
==========

[tommi, 2017-03-11 10:26:22]

Description was changed from

==========
Updates to VCMDecodedFrameCallback, VideoReceiver and a few related
classes/tests.

* The _receiveCallback member of VCMDecodedFrameCallback does actually not
require locking now that the threading model is slightly clearer. Documentation
and checks have been added.
* UserReceiveCallback() never returns null and must always be called on the
decoder thread. Checks have been added and the two test suites that were failing
to set this callback, have been fixed and a new mock class added.  (looks like
sakal@ may have hit some issues with flaky tests there).
* Changed VcmPayloadSink to use move semantics which I suspect was the intention
at the time the code was written (when we didn't have move semantics).
* Added thread checker to a couple of classes and started adding thread checks
for known behavior.  There's more to be  done there.
* Remove the |_decoder| member variable in VideoReceiver. It is not needed and
as it could be used, left us open to a race.
* TODOs added for places where we can reduce locking. I suspect that we can get
away with not needing a lock around _codecDataBase in VideoReceiver once we've
got a clear picture of the threading model and ensured that all adhere to it.

BUG=
==========

to

==========
Updates to VCMDecodedFrameCallback, VideoReceiver and a few related
classes/tests.

* The _receiveCallback member of VCMDecodedFrameCallback does actually not
require locking now that the threading model is slightly clearer. Documentation
and checks have been added.
* UserReceiveCallback() never returns null and must always be called on the
decoder thread. Checks have been added and the two test suites that were failing
to set this callback, have been fixed and a new mock class added.  (looks like
sakal@ may have hit some issues with flaky tests there).
* Changed VcmPayloadSink to use move semantics which I suspect was the intention
at the time the code was written (when we didn't have move semantics).
* Added thread checker to a couple of classes and started adding thread checks
for known behavior.  There's more to be  done there.
* Remove the |_decoder| member variable in VideoReceiver. It is not needed and
as it could be used, left us open to a race.
* TODOs added for places where we can reduce locking. I suspect that we can get
away with not needing a lock around _codecDataBase in VideoReceiver once we've
got a clear picture of the threading model and ensured that all adhere to it.

BUG=7328
==========

[tommi, 2017-03-11 10:26:22]

tommi@webrtc.org changed reviewers:
+ stefan@webrtc.org

[tommi, 2017-03-11 10:26:33]

Description was changed from

==========
Updates to VCMDecodedFrameCallback, VideoReceiver and a few related
classes/tests.

* The _receiveCallback member of VCMDecodedFrameCallback does actually not
require locking now that the threading model is slightly clearer. Documentation
and checks have been added.
* UserReceiveCallback() never returns null and must always be called on the
decoder thread. Checks have been added and the two test suites that were failing
to set this callback, have been fixed and a new mock class added.  (looks like
sakal@ may have hit some issues with flaky tests there).
* Changed VcmPayloadSink to use move semantics which I suspect was the intention
at the time the code was written (when we didn't have move semantics).
* Added thread checker to a couple of classes and started adding thread checks
for known behavior.  There's more to be  done there.
* Remove the |_decoder| member variable in VideoReceiver. It is not needed and
as it could be used, left us open to a race.
* TODOs added for places where we can reduce locking. I suspect that we can get
away with not needing a lock around _codecDataBase in VideoReceiver once we've
got a clear picture of the threading model and ensured that all adhere to it.

BUG=7328
==========

to

==========
Updates to VCMDecodedFrameCallback, VideoReceiver and a few related
classes/tests.

* The _receiveCallback member of VCMDecodedFrameCallback does actually not
require locking now that the threading model is slightly clearer. Documentation
and checks have been added.
* UserReceiveCallback() never returns null and must always be called on the
decoder thread. Checks have been added and the two test suites that were failing
to set this callback, have been fixed and a new mock class added.  (looks like
sakal@ may have hit some issues with flaky tests there).
* Changed VcmPayloadSink to use move semantics which I suspect was the intention
at the time the code was written (when we didn't have move semantics).
* Added thread checker to a couple of classes and started adding thread checks
for known behavior.  There's more to be  done there.
* Remove the |_decoder| member variable in VideoReceiver. It is not needed and
as it could be used, left us open to a race.
* TODOs added for places where we can reduce locking. I suspect that we can get
away with not needing a lock around _codecDataBase in VideoReceiver once we've
got a clear picture of the threading model and ensured that all adhere to it.

BUG=webrtc:7328
==========

[stefan-webrtc, 2017-03-11 12:57:37]

https://codereview.webrtc.org/2744013002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/generic_decoder.h (right):

https://codereview.webrtc.org/2744013002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/generic_decoder.h:59: rtc::CriticalSection lock_;
Move this down to the member it protects.

https://codereview.webrtc.org/2744013002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_receiver.cc (right):

https://codereview.webrtc.org/2744013002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_receiver.cc:172: // been stopped.
Clarify what is to be done to remove this TODO

https://codereview.webrtc.org/2744013002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_receiver.cc:200: // running.  Do we need a
lock in that case?
I think the answer is no as _codecDataBase only seems to be accessed on the
decoder thread and from these Register/Deregister methods. Maybe that's what you
already also have concluded... :)

https://codereview.webrtc.org/2744013002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_receiver.cc:395: // If not, it will make
threading simpler.
No calls that I can find.

[tommi, 2017-03-11 15:11:00]

Rebase

[tommi, 2017-03-11 15:23:51]

Address comments and replace a few assert calls with RTC_DCHECK

[tommi, 2017-03-11 15:24:03]

https://codereview.webrtc.org/2744013002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/generic_decoder.h (right):

https://codereview.webrtc.org/2744013002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/generic_decoder.h:59: rtc::CriticalSection lock_;
On 2017/03/11 12:57:37, stefan-webrtc wrote:
> Move this down to the member it protects.

Done.

https://codereview.webrtc.org/2744013002/diff/80001/webrtc/modules/video_codi...
File webrtc/modules/video_coding/video_receiver.cc (right):

https://codereview.webrtc.org/2744013002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_receiver.cc:172: // been stopped.
On 2017/03/11 12:57:37, stefan-webrtc wrote:
> Clarify what is to be done to remove this TODO

Done.

https://codereview.webrtc.org/2744013002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_receiver.cc:200: // running.  Do we need a
lock in that case?
On 2017/03/11 12:57:37, stefan-webrtc wrote:
> I think the answer is no as _codecDataBase only seems to be accessed on the
> decoder thread and from these Register/Deregister methods. Maybe that's what
you
> already also have concluded... :)

Yes, this is more of a rhetorical question/TODO from myself to me.  A followup
change would be to remove locking, which I think would be nice since as is, all
methods that grab the lock, contend with a decode operation.

https://codereview.webrtc.org/2744013002/diff/80001/webrtc/modules/video_codi...
webrtc/modules/video_coding/video_receiver.cc:395: // If not, it will make
threading simpler.
On 2017/03/11 12:57:37, stefan-webrtc wrote:
> No calls that I can find.

Thanks for looking. I didn't find any either.  I'll note that neither one of us
found a caller.

[tommi, 2017-03-11 18:47:42]

The CQ bit was checked by tommi@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-11 18:47:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-03-11 19:08:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-11 19:08:52]

Dry run: Try jobs failed on following builders:
  win_x64_clang_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_clang_dbg/build...)

[tommi, 2017-03-13 22:05:54]

ping .. btw I've already prepared a CL to remove ReceiveCodec:
https://codereview.webrtc.org/2741853008/

[stefan-webrtc, 2017-03-14 09:58:17]

lgtm

https://codereview.webrtc.org/2744013002/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/video_coding_impl.cc (right):

https://codereview.webrtc.org/2744013002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_coding_impl.cc:103: RTC_DCHECK(receiver_time
>= 0);
GE

[tommi, 2017-03-14 10:52:01]

Update DCHECKs

[tommi, 2017-03-14 10:52:11]

https://codereview.webrtc.org/2744013002/diff/120001/webrtc/modules/video_cod...
File webrtc/modules/video_coding/video_coding_impl.cc (right):

https://codereview.webrtc.org/2744013002/diff/120001/webrtc/modules/video_cod...
webrtc/modules/video_coding/video_coding_impl.cc:103: RTC_DCHECK(receiver_time
>= 0);
On 2017/03/14 09:58:17, stefan-webrtc wrote:
> GE

Done.

[tommi, 2017-03-14 10:52:19]

The CQ bit was checked by tommi@webrtc.org

[tommi, 2017-03-14 10:52:19]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2744013002/#ps140001 (title:
"Update DCHECKs")

[commit-bot: I haz the power, 2017-03-14 10:52:22]

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

[commit-bot: I haz the power, 2017-03-14 11:16:21]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1489488739471800,
"parent_rev": "4262f1f892ea4fed87cc3eb940954039a2aa261d", "commit_rev":
"d0a71ba1ae69f73025e7381fa7e5593b3a0f7833"}

[commit-bot: I haz the power, 2017-03-14 11:16:24]

Description was changed from

==========
Updates to VCMDecodedFrameCallback, VideoReceiver and a few related
classes/tests.

* The _receiveCallback member of VCMDecodedFrameCallback does actually not
require locking now that the threading model is slightly clearer. Documentation
and checks have been added.
* UserReceiveCallback() never returns null and must always be called on the
decoder thread. Checks have been added and the two test suites that were failing
to set this callback, have been fixed and a new mock class added.  (looks like
sakal@ may have hit some issues with flaky tests there).
* Changed VcmPayloadSink to use move semantics which I suspect was the intention
at the time the code was written (when we didn't have move semantics).
* Added thread checker to a couple of classes and started adding thread checks
for known behavior.  There's more to be  done there.
* Remove the |_decoder| member variable in VideoReceiver. It is not needed and
as it could be used, left us open to a race.
* TODOs added for places where we can reduce locking. I suspect that we can get
away with not needing a lock around _codecDataBase in VideoReceiver once we've
got a clear picture of the threading model and ensured that all adhere to it.

BUG=webrtc:7328
==========

to

==========
Updates to VCMDecodedFrameCallback, VideoReceiver and a few related
classes/tests.

* The _receiveCallback member of VCMDecodedFrameCallback does actually not
require locking now that the threading model is slightly clearer. Documentation
and checks have been added.
* UserReceiveCallback() never returns null and must always be called on the
decoder thread. Checks have been added and the two test suites that were failing
to set this callback, have been fixed and a new mock class added.  (looks like
sakal@ may have hit some issues with flaky tests there).
* Changed VcmPayloadSink to use move semantics which I suspect was the intention
at the time the code was written (when we didn't have move semantics).
* Added thread checker to a couple of classes and started adding thread checks
for known behavior.  There's more to be  done there.
* Remove the |_decoder| member variable in VideoReceiver. It is not needed and
as it could be used, left us open to a race.
* TODOs added for places where we can reduce locking. I suspect that we can get
away with not needing a lock around _codecDataBase in VideoReceiver once we've
got a clear picture of the threading model and ensured that all adhere to it.

BUG=webrtc:7328

Review-Url: https://codereview.webrtc.org/2744013002
Cr-Commit-Position: refs/heads/master@{#17226}
Committed:
https://chromium.googlesource.com/external/webrtc/+/d0a71ba1ae69f73025e7381fa...
==========

[commit-bot: I haz the power, 2017-03-14 11:16:26]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/external/webrtc/+/d0a71ba1ae69f73025e7381fa...