Revert to recreating RTCVideoEncoder::Impl

Revert to recreating RTCVideoEncoder::Impl

This CL revert to recreating RTCVideoEncoder::Impl each time RTCVideoEncoder::Impl::Destroy is called to avoid issues like
https://bugs.chromium.org/p/chromium/issues/detail?id=595308#c17.

BUG=595308
Committed: https://crrev.com/50f037b918776a78e40dad72a5f0e777179a5424
Cr-Commit-Position: refs/heads/master@{#391840}

[emircan, 2016-04-28 01:57:48]

Description was changed from

==========
clears.

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

to

==========
Clear class members in RTCVideoEncoder::Impl::Destroy

This CL makes sure that class members are cleared after 
RTCVideoEncoder::Impl::Destroy  is called to avoid issues like
https://bugs.chromium.org/p/chromium/issues/detail?id=595308#c17.

BUG=595308
==========

[emircan, 2016-04-28 01:57:49]

emircan@chromium.org changed reviewers:
+ wuchengli@chromium.org

[emircan, 2016-04-28 01:58:46]

PTAL.

[Pawel Osciak, 2016-04-28 02:05:32]

posciak@chromium.org changed reviewers:
+ posciak@chromium.org

[Pawel Osciak, 2016-04-28 02:05:33]

https://codereview.chromium.org/1928873002/diff/1/content/renderer/media/rtc_...
File content/renderer/media/rtc_video_encoder.cc (right):

https://codereview.chromium.org/1928873002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_video_encoder.cc:389: input_buffers_.clear();
What would you think of having a Clear() (or a similar) method instead?

We could call it from here and the constructor. This is specifically for
picture_id_ as it's not obvious what it should be initialized to and it'd be
best to always init uniformly...

[emircan, 2016-04-28 02:42:35]

Description was changed from

==========
Clear class members in RTCVideoEncoder::Impl::Destroy

This CL makes sure that class members are cleared after 
RTCVideoEncoder::Impl::Destroy  is called to avoid issues like
https://bugs.chromium.org/p/chromium/issues/detail?id=595308#c17.

BUG=595308
==========

to

==========
Clear class members in RTCVideoEncoder::Impl::Destroy

This CL makes sure that class members are cleared after 
RTCVideoEncoder::Impl::Destroy is called to avoid issues like
https://bugs.chromium.org/p/chromium/issues/detail?id=595308#c17.

BUG=595308
==========

[emircan, 2016-04-28 02:44:09]

https://codereview.chromium.org/1928873002/diff/1/content/renderer/media/rtc_...
File content/renderer/media/rtc_video_encoder.cc (right):

https://codereview.chromium.org/1928873002/diff/1/content/renderer/media/rtc_...
content/renderer/media/rtc_video_encoder.cc:389: input_buffers_.clear();
On 2016/04/28 02:05:33, Pawel Osciak wrote:
> What would you think of having a Clear() (or a similar) method instead?
> 
> We could call it from here and the constructor. This is specifically for
> picture_id_ as it's not obvious what it should be initialized to and it'd be
> best to always init uniformly...

Done.

[wuchengli, 2016-04-29 13:58:24]

https://codereview.chromium.org/1928873002/diff/20001/content/renderer/media/...
File content/renderer/media/rtc_video_encoder.cc (right):

https://codereview.chromium.org/1928873002/diff/20001/content/renderer/media/...
content/renderer/media/rtc_video_encoder.cc:272: Reset();
This means RTCVideoEncoder is reused after RTCVideoEncoder::Release is called.
You can consider changing the code back before my patch (#1) -- new |impl_| in
RTCVideoEncoder::InitEncode and release it in RTCVideoEncoder::Release. Then we
don't need to reset anything.

If you prefer Reset(), we should also clear |input_next_frame_| and
|encoded_image_callback_| in Reset().

#1:
https://codereview.chromium.org/1845563006/diff/100001/content/renderer/media...

[emircan, 2016-04-29 19:37:28]

https://codereview.chromium.org/1928873002/diff/20001/content/renderer/media/...
File content/renderer/media/rtc_video_encoder.cc (right):

https://codereview.chromium.org/1928873002/diff/20001/content/renderer/media/...
content/renderer/media/rtc_video_encoder.cc:272: Reset();
On 2016/04/29 13:58:24, wuchengli wrote:
> This means RTCVideoEncoder is reused after RTCVideoEncoder::Release is called.
> You can consider changing the code back before my patch (#1) -- new |impl_| in
> RTCVideoEncoder::InitEncode and release it in RTCVideoEncoder::Release. Then
we
> don't need to reset anything.
> 
> If you prefer Reset(), we should also clear |input_next_frame_| and
> |encoded_image_callback_| in Reset().
> 
> #1:
>
https://codereview.chromium.org/1845563006/diff/100001/content/renderer/media...

Sure, reverting to earlier sounds better as the destructed class will clear all
the members. I thought the current re-use was an intended behavior, so I was
going for Reset().

[wuchengli, 2016-05-04 15:24:04]

On 2016/04/29 19:37:28, emircan wrote:
>
https://codereview.chromium.org/1928873002/diff/20001/content/renderer/media/...
> File content/renderer/media/rtc_video_encoder.cc (right):
> 
>
https://codereview.chromium.org/1928873002/diff/20001/content/renderer/media/...
> content/renderer/media/rtc_video_encoder.cc:272: Reset();
> On 2016/04/29 13:58:24, wuchengli wrote:
> > This means RTCVideoEncoder is reused after RTCVideoEncoder::Release is
called.
> > You can consider changing the code back before my patch (#1) -- new |impl_|
in
> > RTCVideoEncoder::InitEncode and release it in RTCVideoEncoder::Release. Then
> we
> > don't need to reset anything.
> > 
> > If you prefer Reset(), we should also clear |input_next_frame_| and
> > |encoded_image_callback_| in Reset().
> > 
> > #1:
> >
>
https://codereview.chromium.org/1845563006/diff/100001/content/renderer/media...
> 
> Sure, reverting to earlier sounds better as the destructed class will clear
all
> the members. I thought the current re-use was an intended behavior, so I was
> going for Reset().
I forgot webrtc could reuse RtcVideoEncoder. :)

[wuchengli, 2016-05-04 15:24:25]

LGTM. Please update the subject and the change description.

[emircan, 2016-05-05 07:00:00]

Description was changed from

==========
Clear class members in RTCVideoEncoder::Impl::Destroy

This CL makes sure that class members are cleared after 
RTCVideoEncoder::Impl::Destroy is called to avoid issues like
https://bugs.chromium.org/p/chromium/issues/detail?id=595308#c17.

BUG=595308
==========

to

==========
Revert to recreating RTCVideoEncoder::Impl

This CL revert to recreating RTCVideoEncoder::Impl each time
RTCVideoEncoder::Impl::Destroy is called to avoid issues like
https://bugs.chromium.org/p/chromium/issues/detail?id=595308#c17.

BUG=595308
==========

[emircan, 2016-05-05 07:02:03]

emircan@chromium.org changed reviewers:
+ mcasas@chromium.org

[emircan, 2016-05-05 07:02:04]

mcasas@ for RS review.

[mcasas, 2016-05-05 14:27:33]

On 2016/05/05 07:02:04, emircan wrote:
> mcasas@ for RS review.

RS LGTM

[emircan1, 2016-05-05 16:33:41]

The CQ bit was checked by emircan@google.com

[commit-bot: I haz the power, 2016-05-05 16:33:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1928873002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1928873002/40001

[commit-bot: I haz the power, 2016-05-05 17:56:28]

Description was changed from

==========
Revert to recreating RTCVideoEncoder::Impl

This CL revert to recreating RTCVideoEncoder::Impl each time
RTCVideoEncoder::Impl::Destroy is called to avoid issues like
https://bugs.chromium.org/p/chromium/issues/detail?id=595308#c17.

BUG=595308
==========

to

==========
Revert to recreating RTCVideoEncoder::Impl

This CL revert to recreating RTCVideoEncoder::Impl each time
RTCVideoEncoder::Impl::Destroy is called to avoid issues like
https://bugs.chromium.org/p/chromium/issues/detail?id=595308#c17.

BUG=595308
==========

[commit-bot: I haz the power, 2016-05-05 17:56:29]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-05 17:58:36]

Description was changed from

==========
Revert to recreating RTCVideoEncoder::Impl

This CL revert to recreating RTCVideoEncoder::Impl each time
RTCVideoEncoder::Impl::Destroy is called to avoid issues like
https://bugs.chromium.org/p/chromium/issues/detail?id=595308#c17.

BUG=595308
==========

to

==========
Revert to recreating RTCVideoEncoder::Impl

This CL revert to recreating RTCVideoEncoder::Impl each time
RTCVideoEncoder::Impl::Destroy is called to avoid issues like
https://bugs.chromium.org/p/chromium/issues/detail?id=595308#c17.

BUG=595308

Committed: https://crrev.com/50f037b918776a78e40dad72a5f0e777179a5424
Cr-Commit-Position: refs/heads/master@{#391840}
==========

[commit-bot: I haz the power, 2016-05-05 17:58:37]

Patchset 3 (id:??) landed as
https://crrev.com/50f037b918776a78e40dad72a5f0e777179a5424
Cr-Commit-Position: refs/heads/master@{#391840}