Fix DCHECK crash in RtcVideoEncoder.

Fix DCHECK crash in RtcVideoEncoder.
    
webrtc::VideoEncoder is synchronized by webrtc::VideoSender.
The methods are called on different threads but that is OK.
SetRates and SetChannelParameters are called on ProcessThread
or libjingle worker thread. Encode is called on VieCaptureThread.
Others are called on libjingle worker thread.
    
BUG=chromium:350111
TESTS=webrtc call on CrOS.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279669

[wuchengli, 2014-06-23 15:01:41]

PTAL

[wuchengli, 2014-06-24 03:19:23]

scherkus@ OWNER review. Thanks!

[wuchengli, 2014-06-24 03:41:17]

SetRates and SetChannelParameters were called on 
ProcessThread since Mar 26 by
https://code.google.com/p/webrtc/source/detail?r=5794. I'll ask webrtc team if
it's bug or they want to keep this behavior.

[Pawel Osciak, 2014-06-24 03:55:58]

Can we instead of adding a lock, after freeing Impl post a task to the impl
thread and wait for it to finish before returning from Release()? Then would be
sure all tasks are finished before we finish Release().

But actually, are we sure this happens? Impl uses a weak pointer factory so
nothing posted by old Impl should execute after Release invalidates the weak
pointer given to the old Impl...

[Pawel Osciak, 2014-06-24 04:02:16]

On 2014/06/24 03:55:58, Pawel Osciak wrote:
> Can we instead of adding a lock, after freeing Impl post a task to the impl
> thread and wait for it to finish before returning from Release()? Then would
be
> sure all tasks are finished before we finish Release().
> 
> But actually, are we sure this happens? Impl uses a weak pointer factory so
> nothing posted by old Impl should execute after Release invalidates the weak
> pointer given to the old Impl...

Ah, I guess it's possible that a task is already in flight when we
InvalidateWeakPtrs() in Release(), so a task may start before we invalidate, but
not finish before we return from Release() and set impl_ = NULL. Looks like a
race. So I guess my initial suggestion to post something and wait in Release for
Impl tasks to be done is probably better.

[Pawel Osciak, 2014-06-24 04:11:54]

On 2014/06/24 04:02:16, Pawel Osciak wrote:
> On 2014/06/24 03:55:58, Pawel Osciak wrote:
> > Can we instead of adding a lock, after freeing Impl post a task to the impl
> > thread and wait for it to finish before returning from Release()? Then would
> be
> > sure all tasks are finished before we finish Release().
> > 
> > But actually, are we sure this happens? Impl uses a weak pointer factory so
> > nothing posted by old Impl should execute after Release invalidates the weak
> > pointer given to the old Impl...
> 
> Ah, I guess it's possible that a task is already in flight when we
> InvalidateWeakPtrs() in Release(), so a task may start before we invalidate,
but
> not finish before we return from Release() and set impl_ = NULL. Looks like a
> race. So I guess my initial suggestion to post something and wait in Release
for
> Impl tasks to be done is probably better.

Oh again. Scratch the above. The only callbacks coming from Impl to RTCVE are
either ReturnEncodedImage or NotifyError. Given that they run on the same thread
as Release() and we have DCHECKs for that, if you are not hitting them, I think
there is no way something posted from Impl may execute on RTCVE thread after
Release() returns. So I think what you assumed in the CL description should not
be possible, i.e. we can't have an outdated encoded frame in ReturnEncodeImage
after Release() returns...

[wuchengli, 2014-06-24 04:24:13]

On 2014/06/24 04:11:54, Pawel Osciak wrote:
> On 2014/06/24 04:02:16, Pawel Osciak wrote:
> > On 2014/06/24 03:55:58, Pawel Osciak wrote:
> > > Can we instead of adding a lock, after freeing Impl post a task to the
impl
> > > thread and wait for it to finish before returning from Release()? Then
would
> > be
> > > sure all tasks are finished before we finish Release().
> > > 
> > > But actually, are we sure this happens? Impl uses a weak pointer factory
so
> > > nothing posted by old Impl should execute after Release invalidates the
weak
> > > pointer given to the old Impl...
> > 
> > Ah, I guess it's possible that a task is already in flight when we
> > InvalidateWeakPtrs() in Release(), so a task may start before we invalidate,
> but
> > not finish before we return from Release() and set impl_ = NULL. Looks like
a
> > race. So I guess my initial suggestion to post something and wait in Release
> for
> > Impl tasks to be done is probably better.
> 
> Oh again. Scratch the above. The only callbacks coming from Impl to RTCVE are
> either ReturnEncodedImage or NotifyError. Given that they run on the same
thread
> as Release() and we have DCHECKs for that, if you are not hitting them, I
think
> there is no way something posted from Impl may execute on RTCVE thread after
> Release() returns. So I think what you assumed in the CL description should
not
> be possible, i.e. we can't have an outdated encoded frame in ReturnEncodeImage
> after Release() returns...

Agree with Pawel. I'll upload a new patchset.

[wuchengli, 2014-06-24 10:15:14]

PTAL

[kcwu, 2014-06-24 11:57:26]

On 2014/06/24 10:15:14, wuchengli wrote:
> PTAL

The BUG= and TESTS= in the commit msg are duplicated.

[kcwu, 2014-06-24 12:00:12]

lgtm

[scherkus (not reviewing), 2014-06-24 16:18:04]

lgtm

[wuchengli, 2014-06-25 03:52:00]

The CQ bit was checked by wuchengli@chromium.org

[commit-bot: I haz the power, 2014-06-25 03:52:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wuchengli@chromium.org/347073002/80001

[commit-bot: I haz the power, 2014-06-25 10:27:40]

Change committed as 279669