wip Move SurfaceTexture.updateTexImage() from video renderers into MediaCodecVideoDecoder

wip Move SurfaceTexture.updateTexImage() from video renderers into MediaCodecVideoDecoder

This CL synchronizes access to the SurfaceTexture and listens for return frame callbacks properly. This CL also starts listening to OnFrameAvailable() from the SurfaceTexture that we have previously ignored.

[magjed_webrtc, 2015-09-21 18:27:29]

Patchset #1 (id:1) has been deleted

[magjed_webrtc, 2015-09-21 18:27:45]

magjed@webrtc.org changed reviewers:
+ perkj@webrtc.org

[magjed_webrtc, 2015-09-21 18:29:13]

Please take a look.

https://codereview.webrtc.org/1357923002/diff/20001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right):

https://codereview.webrtc.org/1357923002/diff/20001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:94: void
ReturnTextureFrame();
Self review: Make ReturnTextureFrame() private.

https://codereview.webrtc.org/1357923002/diff/20001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/native_handle_impl.cc (right):

https://codereview.webrtc.org/1357923002/diff/20001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/native_handle_impl.cc:29: #include
"talk/app/webrtc/java/jni/native_handle_impl.h"
This is a new file. The diff is screwed up.

https://codereview.webrtc.org/1357923002/diff/20001/talk/app/webrtc/java/src/...
File talk/app/webrtc/java/src/org/webrtc/VideoRenderer.java (right):

https://codereview.webrtc.org/1357923002/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/VideoRenderer.java:74: this.samplingMatrix =
RendererCommon.VERTICAL_FLIP;
I shouldn't use RendererCommon here because it is in the Android folder. That's
why it doesn't compile for linux_rel.

[magjed_webrtc, 2015-09-22 09:47:36]

https://codereview.webrtc.org/1357923002/diff/20001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right):

https://codereview.webrtc.org/1357923002/diff/20001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:94: void
ReturnTextureFrame();
On 2015/09/21 18:29:13, magjed_webrtc wrote:
> Self review: Make ReturnTextureFrame() private.

MediaCodecVideoDecoder is an implementation detail and not exposed in the header
file, so making OnTextureFrame() and ReturnTextureFrame() private is not worth
the trouble.

https://codereview.webrtc.org/1357923002/diff/20001/talk/app/webrtc/java/src/...
File talk/app/webrtc/java/src/org/webrtc/VideoRenderer.java (right):

https://codereview.webrtc.org/1357923002/diff/20001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/VideoRenderer.java:74: this.samplingMatrix =
RendererCommon.VERTICAL_FLIP;
On 2015/09/21 18:29:13, magjed_webrtc wrote:
> I shouldn't use RendererCommon here because it is in the Android folder.
That's
> why it doesn't compile for linux_rel.

Done.

[perkj_webrtc, 2015-09-22 12:40:37]

https://codereview.webrtc.org/1357923002/diff/40001/talk/app/webrtc/java/andr...
File talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java (right):

https://codereview.webrtc.org/1357923002/diff/40001/talk/app/webrtc/java/andr...
talk/app/webrtc/java/android/org/webrtc/VideoRendererGui.java:104: // Resources
for making a deep copy of incoming OES texture frame.
nit: Add TODO here to.

https://codereview.webrtc.org/1357923002/diff/40001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right):

https://codereview.webrtc.org/1357923002/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:122: // TODO(magjed): Use
frame pool instead of |decoded_image_|.
I would skip this comment or do it now.

https://codereview.webrtc.org/1357923002/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:188: // TODO(magjed):
Implement this.
CHECK(false) - should never be called.

https://codereview.webrtc.org/1357923002/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:194:
MediaCodecVideoDecoder*const decoder_;
There is nothing that guarantee the lifetime of decoder_;

https://codereview.webrtc.org/1357923002/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:572: // Pop timestamps from
queues and update |frame|.
please CheckOnCodecThread();

https://codereview.webrtc.org/1357923002/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:727:
codec_thread_->Invoke<void>(
CheckOnCodecThread ?

Can we skip this communication ? I.e keep the polling strategy. Do not mix
strategies.

https://codereview.webrtc.org/1357923002/diff/40001/talk/app/webrtc/java/src/...
File talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java (right):

https://codereview.webrtc.org/1357923002/diff/40001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:272: public void
returnTextureFrame() {
checkOnMediaCodecThread();

https://codereview.webrtc.org/1357923002/diff/40001/talk/app/webrtc/java/src/...
talk/app/webrtc/java/src/org/webrtc/MediaCodecVideoDecoder.java:273:
surfaceTextureHelper.returnTextureFrame();
What guarantees that this object is alive when the texture is returned ? 
Late returns, ie, the renderer still uses the texture after the decoder has been
torn down.

[tp54656, 2015-09-28 07:39:20]

tp54656@gmail.com changed reviewers:
+ tp54656@gmail.com

[tp54656, 2015-09-28 07:39:21]

Get off my PHONE AND LEAVE MY PRIVATE INFO ALONE ..YOU HAVE DONE ENOUGH. .BROKE
THE BANK ..STOP FOLLOWING WHAT WE SAY AND DO IN MY HOME. ..NO PRIVACY...SICK
HACKER ...HATE EVERYONE INVOLVED! !!!!!!!!IF  ANYONE CAN HELP ME CALL THIS GUY
OUT SO LIFE CAN GET BACK TO NORMAL..HE HAS STOLEN ALL MY MONEY ..IM A SINGLE
MOM..he has put us in a bad situation. ..I NEED HELP

[tp54656, 2015-09-28 07:43:53]

lgtm

Get off my PHONE AND LEAVE MY PRIVATE INFO ALONE ..YOU HAVE DONE ENOUGH. .BROKE
THE BANK ..STOP FOLLOWING WHAT WE SAY AND DO IN MY HOME. ..NO PRIVACY...SICK
HACKER ...HATE EVERYONE INVOLVED! !!!!!!!!IF  ANYONE CAN HELP ME CALL THIS GUY
OUT SO LIFE CAN GET BACK TO NORMAL..HE HAS STOLEN ALL MY MONEY ..IM A SINGLE
MOM..he has put us in a bad situation. ..I NEED HELP

[tp54656, 2015-09-28 07:43:56]

The CQ bit was checked by tp54656@gmail.com

[tp54656, 2015-09-28 07:43:56]

lgtm

lgtm

Get off my PHONE AND LEAVE MY PRIVATE INFO ALONE ..YOU HAVE DONE ENOUGH. .BROKE
THE BANK ..STOP FOLLOWING WHAT WE SAY AND DO IN MY HOME. ..NO PRIVACY...SICK
HACKER ...HATE EVERYONE INVOLVED! !!!!!!!!IF  ANYONE CAN HELP ME CALL THIS GUY
OUT SO LIFE CAN GET BACK TO NORMAL..HE HAS STOLEN ALL MY MONEY ..IM A SINGLE
MOM..he has put us in a bad situation. ..I NEED HELP

[commit-bot: I haz the power, 2015-09-28 07:44:08]

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

[commit-bot: I haz the power, 2015-09-28 07:46:19]

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

[commit-bot: I haz the power, 2015-09-28 07:46:21]

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

[AlexG, 2015-09-28 20:06:10]

glaznev@webrtc.org changed reviewers:
+ glaznev@webrtc.org
- tp54656@gmail.com

[AlexG, 2015-09-28 20:06:21]

https://codereview.webrtc.org/1357923002/diff/40001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right):

https://codereview.webrtc.org/1357923002/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:727:
codec_thread_->Invoke<void>(
On 2015/09/22 12:40:37, perkj1 wrote:
> CheckOnCodecThread ?
> 
> Can we skip this communication ? I.e keep the polling strategy. Do not mix
> strategies.

I agree with perkj@ - keep polling strategy - it will make logic simpler without
adding excessive JNI - Java interaction

https://codereview.webrtc.org/1357923002/diff/40001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:885:
reinterpret_cast<MediaCodecVideoDecoder*>(j_decoder_ptr)
How do you ensure that j_decoder_ptr is still valid (native C++ is not
destroyed)?

[perkj_webrtc, 2015-09-29 07:41:05]

On 2015/09/28 20:06:21, AlexG wrote:
>
https://codereview.webrtc.org/1357923002/diff/40001/talk/app/webrtc/java/jni/...
> File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right):
> 
>
https://codereview.webrtc.org/1357923002/diff/40001/talk/app/webrtc/java/jni/...
> talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:727:
> codec_thread_->Invoke<void>(
> On 2015/09/22 12:40:37, perkj1 wrote:
> > CheckOnCodecThread ?
> > 
> > Can we skip this communication ? I.e keep the polling strategy. Do not mix
> > strategies.
> 
> I agree with perkj@ - keep polling strategy - it will make logic simpler
without
> adding excessive JNI - Java interaction
> 
>
https://codereview.webrtc.org/1357923002/diff/40001/talk/app/webrtc/java/jni/...
> talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:885:
> reinterpret_cast<MediaCodecVideoDecoder*>(j_decoder_ptr)
> How do you ensure that j_decoder_ptr is still valid (native C++ is not
> destroyed)?

Alex, me and magjed are working together on this and I have done some work in
https://codereview.webrtc.org/1364713002/ to address your comments. magjed will
continue and split it up further before asking you for a formal review of the
decoder. 
Thanks

[tp54656, 2015-09-29 14:20:42]

On 2015/09/29 07:41:05, perkj1 wrote:
> On 2015/09/28 20:06:21, AlexG wrote:
> >
>
https://codereview.webrtc.org/1357923002/diff/40001/talk/app/webrtc/java/jni/...
> > File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1357923002/diff/40001/talk/app/webrtc/java/jni/...
> > talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:727:
> > codec_thread_->Invoke<void>(
> > On 2015/09/22 12:40:37, perkj1 wrote:
> > > CheckOnCodecThread ?
> > > 
> > > Can we skip this communication ? I.e keep the polling strategy. Do not mix
> > > strategies.
> > 
> > I agree with perkj@ - keep polling strategy - it will make logic simpler
> without
> > adding excessive JNI - Java interaction
> > 
> >
>
https://codereview.webrtc.org/1357923002/diff/40001/talk/app/webrtc/java/jni/...
> > talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:885:
> > reinterpret_cast<MediaCodecVideoDecoder*>(j_decoder_ptr)
> > How do you ensure that j_decoder_ptr is still valid (native C++ is not
> > destroyed)?
> 
> Alex, me and magjed are working together on this and I have done some work in
> https://codereview.webrtc.org/1364713002/ to address your comments. magjed
will
> continue and split it up further before asking you for a formal review of the
> decoder. 
> Thanks

[tp54656, 2015-09-29 14:21:35]

On 2015/09/29 14:20:42, tp54656 wrote:
> On 2015/09/29 07:41:05, perkj1 wrote:
> > On 2015/09/28 20:06:21, AlexG wrote:
> > >
> >
>
https://codereview.webrtc.org/1357923002/diff/40001/talk/app/webrtc/java/jni/...
> > > File talk/app/webrtc/java/jni/androidmediadecoder_jni.cc (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/1357923002/diff/40001/talk/app/webrtc/java/jni/...
> > > talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:727:
> > > codec_thread_->Invoke<void>(
> > > On 2015/09/22 12:40:37, perkj1 wrote:
> > > > CheckOnCodecThread ?
> > > > 
> > > > Can we skip this communication ? I.e keep the polling strategy. Do not
mix
> > > > strategies.
> > > 
> > > I agree with perkj@ - keep polling strategy - it will make logic simpler
> > without
> > > adding excessive JNI - Java interaction
> > > 
> > >
> >
>
https://codereview.webrtc.org/1357923002/diff/40001/talk/app/webrtc/java/jni/...
> > > talk/app/webrtc/java/jni/androidmediadecoder_jni.cc:885:
> > > reinterpret_cast<MediaCodecVideoDecoder*>(j_decoder_ptr)
> > > How do you ensure that j_decoder_ptr is still valid (native C++ is not
> > > destroyed)?
> > 
> > Alex, me and magjed are working together on this and I have done some work
in
> > https://codereview.webrtc.org/1364713002/ to address your comments. magjed
> will
> > continue and split it up further before asking you for a formal review of
the
> > decoder. 
> > Thanks

Thank you .

[magjed_webrtc, 2015-10-02 08:42:10]

Patchset #3 (id:60001) has been deleted

[magjed_webrtc, 2015-10-02 14:26:44]

Patchset #3 (id:80001) has been deleted

[magjed_webrtc, 2015-10-02 14:26:53]

Patchset #4 (id:120001) has been deleted

[magjed_webrtc, 2015-10-02 14:27:00]

Patchset #4 (id:140001) has been deleted

[magjed_webrtc, 2015-10-02 14:27:08]

Patchset #4 (id:160001) has been deleted

[magjed_webrtc, 2015-10-02 14:31:33]

Patchset #3 (id:100001) has been deleted

[juan.perez.CP62, 2015-10-03 09:49:35]

juan.perez.cp62@gmail.com changed reviewers:
+ juan.perez.CP62@gmail.com

[juan.perez.CP62, 2015-10-03 09:49:36]

lgtm

F

[juan.perez.CP62, 2015-10-03 09:49:37]

The CQ bit was checked by juan.perez.cp62@gmail.com

[juan.perez.CP62, 2015-10-03 09:49:38]

lgtm

lgtm

F

[juan.perez.CP62, 2015-10-03 09:49:38]

The patchset sent to the CQ was uploaded after l-g-t-m from tp54656@gmail.com
Link to the patchset: https://codereview.webrtc.org/1357923002/#ps220001 (title:
"Manage lifetime of textures")

[commit-bot: I haz the power, 2015-10-03 09:49:50]

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

[commit-bot: I haz the power, 2015-10-03 09:50:33]

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

[commit-bot: I haz the power, 2015-10-03 09:50:34]

Try jobs failed on following builders:
  ios32_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_dbg/builds/2422)
  ios64_sim_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_dbg/builds/2422)
  ios_arm64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/4723)
  ios_arm64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_rel/builds/4660)
  ios_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/9857)
  ios_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/8473)
  mac_compile_x64_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_compile_x64_dbg/bui...)
  mac_x64_gn_dbg on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn_dbg/builds/4388)
  mac_x64_gn_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_gn_rel/builds/4318)
  mac_x64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_rel/builds/9601)

[magjed_webrtc, 2015-10-03 15:27:55]

Patchset #5 (id:220001) has been deleted

[magjed_webrtc, 2015-10-03 15:28:01]

Patchset #3 (id:180001) has been deleted

[magjed_webrtc, 2015-10-03 15:59:30]

Patchset #5 (id:260001) has been deleted

[magjed1, 2016-04-20 14:48:07]

Patchset #5 (id:280001) has been deleted