Issue 2178973004: DialogSurfaceManager implementation.

liberato (no reviews please)

Description was changed from ========== DialogSurfaceManager implementation. Do-nothing implementation of DialogSurfaceManager. This CL introduces the ...

4 years, 4 months ago (2016-07-26 16:34:23 UTC) #1

liberato (no reviews please)

Description was changed from ========== DialogSurfaceManager implementation. Do-nothing implementation of DialogSurfaceManager. This CL introduces the ...

4 years, 4 months ago (2016-07-26 16:40:40 UTC) #2

liberato (no reviews please)

liberato@chromium.org changed reviewers: + dalecurtis@chromium.org, sievers@chromium.org, timav@chromium.org, watk@chromium.org

4 years, 4 months ago (2016-07-26 16:40:41 UTC) #3

liberato (no reviews please)

hey folks! after some additional effort, i think that it's time to start landing DialogSurface. ...

4 years, 4 months ago (2016-07-26 16:42:05 UTC) #4

liberato (no reviews please)

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-07-26 16:49:13 UTC) #5

commit-bot: I haz the power

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

4 years, 4 months ago (2016-07-26 16:49:24 UTC) #6

commit-bot: I haz the power

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

4 years, 4 months ago (2016-07-26 16:50:54 UTC) #7

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/41560) ios-device-gn on ...

4 years, 4 months ago (2016-07-26 16:50:55 UTC) #8

liberato (no reviews please)

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-07-26 17:15:48 UTC) #9

commit-bot: I haz the power

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

4 years, 4 months ago (2016-07-26 17:16:07 UTC) #10

commit-bot: I haz the power

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

4 years, 4 months ago (2016-07-26 17:19:25 UTC) #11

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/102155) android_compile_dbg on ...

4 years, 4 months ago (2016-07-26 17:19:26 UTC) #12

liberato (no reviews please)

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-07-26 18:30:16 UTC) #13

commit-bot: I haz the power

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

4 years, 4 months ago (2016-07-26 18:30:42 UTC) #14

commit-bot: I haz the power

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

4 years, 4 months ago (2016-07-26 19:23:53 UTC) #15

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clang_dbg_recipe/builds/102464)

4 years, 4 months ago (2016-07-26 19:23:54 UTC) #16

watk

just nits https://codereview.chromium.org/2178973004/diff/60001/content/browser/media/android/dialog_surface_activity_mapper.cc File content/browser/media/android/dialog_surface_activity_mapper.cc (right): https://codereview.chromium.org/2178973004/diff/60001/content/browser/media/android/dialog_surface_activity_mapper.cc#newcode19 content/browser/media/android/dialog_surface_activity_mapper.cc:19: jint renderHandle, Java side calls this renderer_pid ...

4 years, 4 months ago (2016-07-26 21:34:31 UTC) #17

liberato (no reviews please)

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-07-26 22:08:00 UTC) #18

commit-bot: I haz the power

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

4 years, 4 months ago (2016-07-26 22:08:53 UTC) #19

liberato (no reviews please)

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-07-26 22:49:00 UTC) #20

liberato (no reviews please)

https://codereview.chromium.org/2178973004/diff/60001/content/browser/media/android/dialog_surface_activity_mapper.cc File content/browser/media/android/dialog_surface_activity_mapper.cc (right): https://codereview.chromium.org/2178973004/diff/60001/content/browser/media/android/dialog_surface_activity_mapper.cc#newcode19 content/browser/media/android/dialog_surface_activity_mapper.cc:19: jint renderHandle, On 2016/07/26 21:34:30, watk wrote: > Java ...

4 years, 4 months ago (2016-07-26 22:49:16 UTC) #21

commit-bot: I haz the power

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

4 years, 4 months ago (2016-07-26 22:49:35 UTC) #22

commit-bot: I haz the power

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

4 years, 4 months ago (2016-07-26 23:50:44 UTC) #23

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gyp_rel/builds/6582)

4 years, 4 months ago (2016-07-26 23:50:45 UTC) #24

liberato (no reviews please)

The CQ bit was checked by liberato@chromium.org to run a CQ dry run

4 years, 4 months ago (2016-07-27 00:09:30 UTC) #25

commit-bot: I haz the power

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

4 years, 4 months ago (2016-07-27 00:10:00 UTC) #26

commit-bot: I haz the power

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

4 years, 4 months ago (2016-07-27 02:38:58 UTC) #27

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

4 years, 4 months ago (2016-07-27 02:38:59 UTC) #28

DaleCurtis

https://codereview.chromium.org/2178973004/diff/140001/content/browser/media/android/dialog_surface_activity_mapper.cc File content/browser/media/android/dialog_surface_activity_mapper.cc (right): https://codereview.chromium.org/2178973004/diff/140001/content/browser/media/android/dialog_surface_activity_mapper.cc#newcode14 content/browser/media/android/dialog_surface_activity_mapper.cc:14: using namespace content; We don't typically use "using" https://codereview.chromium.org/2178973004/diff/140001/content/browser/media/android/dialog_surface_activity_mapper.cc#newcode16 ...

4 years, 4 months ago (2016-07-27 18:14:20 UTC) #29

liberato (no reviews please)

https://codereview.chromium.org/2178973004/diff/140001/content/browser/media/android/dialog_surface_activity_mapper.cc File content/browser/media/android/dialog_surface_activity_mapper.cc (right): https://codereview.chromium.org/2178973004/diff/140001/content/browser/media/android/dialog_surface_activity_mapper.cc#newcode14 content/browser/media/android/dialog_surface_activity_mapper.cc:14: using namespace content; On 2016/07/27 18:14:19, DaleCurtis wrote: > ...

4 years, 4 months ago (2016-07-28 15:33:31 UTC) #30

Tima Vaisburd

I cannot comment on the essence, just pedantic nits so far. https://codereview.chromium.org/2178973004/diff/160001/content/browser/media/android/dialog_surface_activity_mapper.cc File content/browser/media/android/dialog_surface_activity_mapper.cc (right): ...

4 years, 4 months ago (2016-08-03 18:05:13 UTC) #31

Tima Vaisburd

https://codereview.chromium.org/2178973004/diff/160001/media/base/android/dialog_surface_manager.h File media/base/android/dialog_surface_manager.h (right): https://codereview.chromium.org/2178973004/diff/160001/media/base/android/dialog_surface_manager.h#newcode33 media/base/android/dialog_surface_manager.h:33: // surface->GetSurface() On 2016/08/03 18:05:13, Tima Vaisburd wrote: > ...

4 years, 4 months ago (2016-08-03 18:28:23 UTC) #32

liberato (no reviews please)

thanks for the feedback. -fl https://codereview.chromium.org/2178973004/diff/160001/content/browser/media/android/dialog_surface_activity_mapper.cc File content/browser/media/android/dialog_surface_activity_mapper.cc (right): https://codereview.chromium.org/2178973004/diff/160001/content/browser/media/android/dialog_surface_activity_mapper.cc#newcode40 content/browser/media/android/dialog_surface_activity_mapper.cc:40: if (!frame) On 2016/08/03 ...

4 years, 4 months ago (2016-08-03 23:11:55 UTC) #33

thanks for the feedback.

-fl

https://codereview.chromium.org/2178973004/diff/160001/content/browser/media/...
File content/browser/media/android/dialog_surface_activity_mapper.cc (right):

https://codereview.chromium.org/2178973004/diff/160001/content/browser/media/...
content/browser/media/android/dialog_surface_activity_mapper.cc:40: if (!frame)
On 2016/08/03 18:05:13, Tima Vaisburd wrote:
> Similar DVLOG(1) as l.33 (and below)? Or not having a frame, web_contents, cvc
> would be a normal condition? 

Done.

https://codereview.chromium.org/2178973004/diff/160001/media/base/android/dia...
File media/base/android/dialog_surface.h (right):

https://codereview.chromium.org/2178973004/diff/160001/media/base/android/dia...
media/base/android/dialog_surface.h:32: class MEDIA_EXPORT DialogSurface {
On 2016/08/03 18:05:13, Tima Vaisburd wrote:
> A general comment: I like stub CLs because they make it easier to concentrate
on
> the interface and documentation, but then I'd expect complete and documented
> interfaces. Are there going to be more public methods? Why did you choose to
> omit them?
> 
> In particular, it seems a comment in DialogSurfaceManager suggests
> void GetSurface() and void ScheduleLayout(rect) ?
> 
> As well, the headers seem not to be used yet. I'd say it would be better to
> remove unused headers everywhere and add them in later CLs when they are
needed.

they're included because some of the internal structs are used. 
DialogSurface::Config, for example, is used by dialog manager.

i agree that it's a little confusing as it is.  i re-added some of the methods
to dialog_surface.h, commented out since there's no impl file yet.

https://codereview.chromium.org/2178973004/diff/160001/media/base/android/dia...
media/base/android/dialog_surface.h:34: // These must match DialogSurface.java
On 2016/08/03 18:05:13, Tima Vaisburd wrote:
> I could not find DialogSurface.java in this CL. Where is it?

intentionally omitted for scope.  i wanted to remove all of dialog_surface.h,
but couldn't do it.

https://codereview.chromium.org/2178973004/diff/160001/media/base/android/dia...
File media/base/android/dialog_surface_manager.h (right):

https://codereview.chromium.org/2178973004/diff/160001/media/base/android/dia...
media/base/android/dialog_surface_manager.h:31: // surface =
DialogSurfaceManager::GetPointer()->CreateSurface(callback);
On 2016/08/03 18:05:13, Tima Vaisburd wrote:
> What is the type of |surface|? DialogSurface?

DialogSurface*, updated comment.

https://codereview.chromium.org/2178973004/diff/160001/media/base/android/dia...
media/base/android/dialog_surface_manager.h:33: // surface->GetSurface()
On 2016/08/03 18:28:22, Tima Vaisburd wrote:
> On 2016/08/03 18:05:13, Tima Vaisburd wrote:
> > void Surface::GetSurface() on the surface object that already exists is
> > confusing to me. What does it really do?
> 
> After rereading the doc: I think in the doc CreateSurface(callback) was an
async
> call, did it become a sync (i.e. blocking) call? In the doc it was
>   manager->RequestToCreateSurface(callback);
>   <wait>
>   Surface = manager->GetSurface();
> which I understand.
> 
> Please note that the doc is neither referenced in this CL nor in the
> corresponding bug, and it seems that we try to maintain the code to be
> self-documented, so I guess it won't be referenced.

not sure what you mean -- it's async here too.  in both cases, one must wait for
the callback to signal that the surface is created before expecting that
GetSurface will return non-null.

https://codereview.chromium.org/2178973004/diff/160001/media/base/android/dia...
media/base/android/dialog_surface_manager.h:33: // surface->GetSurface()
On 2016/08/03 18:05:13, Tima Vaisburd wrote:
> void Surface::GetSurface() on the surface object that already exists is
> confusing to me. What does it really do?

it returns the android surface.  probably clearer if i hadn't removed the
methods.  i put them back, commented out.

also open to other names -- i agree that GetSurface isn't descriptive.

https://codereview.chromium.org/2178973004/diff/160001/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/2178973004/diff/160001/media/media.gyp#newcod...
media/media.gyp:1825:
'base/android/java/src/org/chromium/media/DialogSurfaceManagerWrapper.java',
On 2016/08/03 18:05:13, Tima Vaisburd wrote:
> As far as I got GYP support has been abandoned. I think we should follow the
> recommendation not to touch these files, they have become a distraction.

Done.

DaleCurtis

lgtm % nits. https://codereview.chromium.org/2178973004/diff/200001/content/browser/media/android/dialog_surface_activity_mapper.cc File content/browser/media/android/dialog_surface_activity_mapper.cc (right): https://codereview.chromium.org/2178973004/diff/200001/content/browser/media/android/dialog_surface_activity_mapper.cc#newcode14 content/browser/media/android/dialog_surface_activity_mapper.cc:14: static ScopedJavaLocalRef<jobject> GetContentViewCore( This is still ...

4 years, 4 months ago (2016-08-04 00:20:36 UTC) #34

Tima Vaisburd

https://codereview.chromium.org/2178973004/diff/160001/media/base/android/dialog_surface_manager.h File media/base/android/dialog_surface_manager.h (right): https://codereview.chromium.org/2178973004/diff/160001/media/base/android/dialog_surface_manager.h#newcode33 media/base/android/dialog_surface_manager.h:33: // surface->GetSurface() On 2016/08/03 23:11:55, liberato wrote: > On ...

4 years, 4 months ago (2016-08-04 00:21:09 UTC) #35

https://codereview.chromium.org/2178973004/diff/160001/media/base/android/dia...
File media/base/android/dialog_surface_manager.h (right):

https://codereview.chromium.org/2178973004/diff/160001/media/base/android/dia...
media/base/android/dialog_surface_manager.h:33: // surface->GetSurface()
On 2016/08/03 23:11:55, liberato wrote:
> On 2016/08/03 18:28:22, Tima Vaisburd wrote:
> > On 2016/08/03 18:05:13, Tima Vaisburd wrote:
> > > void Surface::GetSurface() on the surface object that already exists is
> > > confusing to me. What does it really do?
> > 
> > After rereading the doc: I think in the doc CreateSurface(callback) was an
> async
> > call, did it become a sync (i.e. blocking) call? In the doc it was
> >   manager->RequestToCreateSurface(callback);
> >   <wait>
> >   Surface = manager->GetSurface();
> > which I understand.
> > 
> > Please note that the doc is neither referenced in this CL nor in the
> > corresponding bug, and it seems that we try to maintain the code to be
> > self-documented, so I guess it won't be referenced.
> 
> not sure what you mean -- it's async here too.  in both cases, one must wait
for
> the callback to signal that the surface is created before expecting that
> GetSurface will return non-null.

Ah, the DialogSurface is not a Surface...
So, as far as I get, it is a proxy object, like token, that you get at once, but
in invalid state; it later becomes valid, and then you can use it to get the
surface.

Can you send the token with the callback instead? E.g. DialogSurface::Callback
might take two parameters: Operation and DialogSurface* (I would rename to
something like SurfaceToken*). Then when you get it, it is already valid?

Alternatively, would a more traditional interface where DialogSurfaceManager
does all the job make sense in this situation? Its callback might return the
surface directly and layout can be changed with
   DialogSurfaceManager::ScheduleLayout(config, surface) ?
I do not know whether it would be better or not, it is just a question.

liberato (no reviews please)

On 2016/08/04 00:21:09, Tima Vaisburd wrote: > https://codereview.chromium.org/2178973004/diff/160001/media/base/android/dialog_surface_manager.h > File media/base/android/dialog_surface_manager.h (right): > > https://codereview.chromium.org/2178973004/diff/160001/media/base/android/dialog_surface_manager.h#newcode33 ...

4 years, 4 months ago (2016-08-04 01:05:42 UTC) #36

On 2016/08/04 00:21:09, Tima Vaisburd wrote:
>
https://codereview.chromium.org/2178973004/diff/160001/media/base/android/dia...
> File media/base/android/dialog_surface_manager.h (right):
> 
>
https://codereview.chromium.org/2178973004/diff/160001/media/base/android/dia...
> media/base/android/dialog_surface_manager.h:33: // surface->GetSurface()
> On 2016/08/03 23:11:55, liberato wrote:
> > On 2016/08/03 18:28:22, Tima Vaisburd wrote:
> > > On 2016/08/03 18:05:13, Tima Vaisburd wrote:
> > > > void Surface::GetSurface() on the surface object that already exists is
> > > > confusing to me. What does it really do?
> > > 
> > > After rereading the doc: I think in the doc CreateSurface(callback) was an
> > async
> > > call, did it become a sync (i.e. blocking) call? In the doc it was
> > >   manager->RequestToCreateSurface(callback);
> > >   <wait>
> > >   Surface = manager->GetSurface();
> > > which I understand.
> > > 
> > > Please note that the doc is neither referenced in this CL nor in the
> > > corresponding bug, and it seems that we try to maintain the code to be
> > > self-documented, so I guess it won't be referenced.
> > 
> > not sure what you mean -- it's async here too.  in both cases, one must wait
> for
> > the callback to signal that the surface is created before expecting that
> > GetSurface will return non-null.
> 
> Ah, the DialogSurface is not a Surface...
> So, as far as I get, it is a proxy object, like token, that you get at once,
but
> in invalid state; it later becomes valid, and then you can use it to get the
> surface.

Yes, exactly.  I'd add that it might become invalid later.  It does correspond
to, more or less, a SurfaceHolder in java.  I didn't want to call it that, since
it does do things that SurfaceHolder doesn't (e.g., layout).

> Can you send the token with the callback instead? E.g. DialogSurface::Callback
> might take two parameters: Operation and DialogSurface* (I would rename to
> something like SurfaceToken*). Then when you get it, it is already valid?

The app still needs to be able to interact with it even when there's no android
surface.  for example, it needs some way to position and size it, and possibly
to destroy it.  so, it needs some way to talk about "the surface that will be
created", independently of whether that surface currently exists or not.

Hrm, "token" is a cool name, but i'm worried that it's a little more than a
token.  I'll think some more along these lines.  i agree with your general point
-- DialogSurface should be a, well, Surface of some sort :)

> Alternatively, would a more traditional interface where DialogSurfaceManager
> does all the job make sense in this situation? Its callback might return the
> surface directly and layout can be changed with
>    DialogSurfaceManager::ScheduleLayout(config, surface) ?
> I do not know whether it would be better or not, it is just a question.

i went back and forth on this while developing this CL.  in the end, i opted for
a placeholder object like DialogSurface for the nice RAII pattern.

maybe move all the methods onto DialogSurfaceManager, as you suggest, with the
token as an argument.  then it really is a token, and i get my RAII.  i'll think
about what client code would end up looking like if we did that.

liberato (no reviews please)

https://codereview.chromium.org/2178973004/diff/200001/content/browser/media/android/dialog_surface_activity_mapper.cc File content/browser/media/android/dialog_surface_activity_mapper.cc (right): https://codereview.chromium.org/2178973004/diff/200001/content/browser/media/android/dialog_surface_activity_mapper.cc#newcode14 content/browser/media/android/dialog_surface_activity_mapper.cc:14: static ScopedJavaLocalRef<jobject> GetContentViewCore( On 2016/08/04 00:20:36, DaleCurtis wrote: > ...

4 years, 4 months ago (2016-08-04 01:06:20 UTC) #37

https://codereview.chromium.org/2178973004/diff/200001/content/browser/media/...
File content/browser/media/android/dialog_surface_activity_mapper.cc (right):

https://codereview.chromium.org/2178973004/diff/200001/content/browser/media/...
content/browser/media/android/dialog_surface_activity_mapper.cc:14: static
ScopedJavaLocalRef<jobject> GetContentViewCore(
On 2016/08/04 00:20:36, DaleCurtis wrote:
> This is still pretty crummy to have to do, but if it's what's done elsewhere I
> don't have a better suggestion without digging into the process more.

agree, it's ugly.  it's quite similar to what StreamTexture (or something that
uses it, don't remember) does.

https://codereview.chromium.org/2178973004/diff/200001/media/base/android/dia...
File media/base/android/dialog_surface.h (right):

https://codereview.chromium.org/2178973004/diff/200001/media/base/android/dia...
media/base/android/dialog_surface.h:42: // Configuration for the surface.
On 2016/08/04 00:20:36, DaleCurtis wrote:
> Is it worth keeping this as a struct? I.e., how soon do you expect there to be
> more than one element? 

is_secure will be showing up before too long, i think.  format will be needed if
we ever use these outside of mediacodec.

https://codereview.chromium.org/2178973004/diff/200001/media/base/android/dia...
File media/base/android/dialog_surface_manager.h (right):

https://codereview.chromium.org/2178973004/diff/200001/media/base/android/dia...
media/base/android/dialog_surface_manager.h:40: static DialogSurfaceManager&
Get();
On 2016/08/04 00:20:36, DaleCurtis wrote:
> I think it's fine to have this named Get() but return a pointer, that's what
we
> do almost everywhere else. It's rare to return non-const refs though.

Done.

https://codereview.chromium.org/2178973004/diff/200001/media/base/android/dia...
media/base/android/dialog_surface_manager.h:52: static bool
RegisterDialogSurfaceManager(JNIEnv* env);
On 2016/08/04 00:20:36, DaleCurtis wrote:
> sievers@ removed a lot of these recently, are they necessary for your class?

thanks!  i didn't realize that.  they're now only needed if java calls into
native, not the other way around.  DialogSurfaceManager doesn't need it anymore,
and DialogSurface might not either.  Wrapper and ActivityMapper still do,
however.

liberato (no reviews please)

i renamed DialogSurface* to AndroidSurface*, and renamed AndroidSurface to AndroidSurfaceToken. here's my reasoning: - "Dialog" ...

4 years, 4 months ago (2016-08-04 17:17:19 UTC) #38

watk

On 2016/08/04 17:17:19, liberato wrote: > i renamed DialogSurface* to AndroidSurface*, and renamed AndroidSurface to ...

4 years, 4 months ago (2016-08-04 18:52:58 UTC) #39

Tima Vaisburd

Would you think I'd be good to get back to PS12 and then rename DialogSurface ...

4 years, 4 months ago (2016-08-04 19:08:35 UTC) #40

liberato (no reviews please)

On 2016/08/04 19:08:35, Tima Vaisburd wrote: > Would you think I'd be good to get ...

4 years, 4 months ago (2016-08-04 19:15:42 UTC) #41

liberato (no reviews please)

liberato@chromium.org changed reviewers: + kenrb@chromium.org

4 years, 4 months ago (2016-08-04 20:52:17 UTC) #42

liberato (no reviews please)

kenrb: PTAL @ gpu/ipc for security review, plus anything else you'd care to. thanks -fl

4 years, 4 months ago (2016-08-04 20:57:00 UTC) #43

liberato (no reviews please)

i realized yesterday that threading is broken since i moved out of gpu -- random ...

4 years, 4 months ago (2016-08-11 17:26:33 UTC) #45

liberato (no reviews please)

this causes ActivtyMapper to post to the browser ui thread in native, which i think ...

4 years, 4 months ago (2016-08-15 18:40:32 UTC) #46

liberato (no reviews please)

liberato@chromium.org changed reviewers: + aelias@chromium.org, boliu@chromium.org

4 years, 3 months ago (2016-09-19 21:02:56 UTC) #47

liberato (no reviews please)

aelias, boliu: sievers suggested that one (or both) of you two might be appropriate to ...

4 years, 3 months ago (2016-09-19 21:04:36 UTC) #48

boliu

so uhh, what stage is this project? seems like it's past the design phase where ...

4 years, 3 months ago (2016-09-20 15:56:16 UTC) #49

boliu

On 2016/09/20 15:56:16, boliu wrote: > so uhh, what stage is this project? seems like ...

4 years, 3 months ago (2016-09-20 15:57:06 UTC) #50

liberato (no reviews please)

On 2016/09/20 15:57:06, boliu wrote: > On 2016/09/20 15:56:16, boliu wrote: > > so uhh, ...

4 years, 3 months ago (2016-09-20 16:44:48 UTC) #51

aelias_OOO_until_Jul13

At a high level, the idea seems good and I support this. We don't get ...

4 years, 3 months ago (2016-09-21 08:06:33 UTC) #52

no sievers

sievers@chromium.org changed reviewers: - sievers@chromium.org

4 years, 3 months ago (2016-09-21 22:04:39 UTC) #53

liberato (no reviews please)

sorry for the delay. boliu: this version renames DialogSurfaceManager.java => DialogSurfaceManagerImpl.java . I didn't do ...

4 years, 1 month ago (2016-11-02 17:49:50 UTC) #54

boliu

definitely did not finish looking at everything (wasn't looking linearly) but some comments so far ...

4 years, 1 month ago (2016-11-04 00:07:20 UTC) #55

liberato (no reviews please)

boliu: now that UnguessableToken's java binding has landed, i went ahead and removed (pid, frame ...

4 years, 1 month ago (2016-11-11 21:52:36 UTC) #56

boliu: now that UnguessableToken's java binding has landed, i went ahead and
removed (pid, frame id) in favor of it.

render frames don't actually have unguessable tokens yet, but that's out of
scope for this CL.  i'm working on adding them.

also, i realized that i can't remove the dependency on ContentViewCore yet.  in
a later CL, there's code to watch for changes to the WindowAndroid, and it
relies on functionality in ContentViewCore.  So, i un-did those changes.  my
in-line replies to your comments on that subject sound like it's done, but
they're now reverted.

thanks
-fl

https://codereview.chromium.org/2178973004/diff/320001/content/app/android/ap...
File content/app/android/app_jni_registrar.cc (right):

https://codereview.chromium.org/2178973004/diff/320001/content/app/android/ap...
content/app/android/app_jni_registrar.cc:16: {"ContentMain",
content::RegisterContentMain},
On 2016/11/04 00:07:19, boliu wrote:
> err, avoid white space changes in gigantic CL..?

weird -- i can imagine that i indented accidentally, but removing the white
space near the braces... gotta guess cl format.

https://codereview.chromium.org/2178973004/diff/320001/content/browser/media/...
File content/browser/media/android/dialog_surface_activity_mapper.cc (right):

https://codereview.chromium.org/2178973004/diff/320001/content/browser/media/...
content/browser/media/android/dialog_surface_activity_mapper.cc:17:
GetContentViewCore(JNIEnv* env, jint renderer_pid, jint render_frame_id) {
On 2016/11/04 00:07:19, boliu wrote:
> we'd like to remove ContentViewCore, and then following that, removing the
> concept of android Views from src/content
> 
> ideally the window token should come from WindowAndroid, which wraps a Context
> (which may or may not be an Activity). looks like in the wonderful world of
> android apis, neither is good enough to get a window token...
> 
> in which case I think the content embedder needs to supply WindowAndroid with
> the token
> 
> also don't need env here
> 
> and this and PostContentViewCore should be in anonymous namespace (apparently
> the preferred style, but more to distinguish from actual implementation of jni
> functions)

CVC: once CVC goes away, i'm assuming that a RenderFrameHostImpl will directly
know about its current WindowAndroid.  is that what you mean?

per off-line discussion, i'll expose getWindowToken on WindowAndroid.  might
also rename this class since it no longer has anything to with an activity.

namespace: done.
env: done.

https://codereview.chromium.org/2178973004/diff/320001/content/browser/media/...
content/browser/media/android/dialog_surface_activity_mapper.cc:22:
content::RenderProcessHost::AllHostsIterator();
On 2016/11/04 00:07:19, boliu wrote:
> DCHECK this is on UI thread?

Done.

https://codereview.chromium.org/2178973004/diff/320001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/DialogSurfaceActivityMapper.java
(right):

https://codereview.chromium.org/2178973004/diff/320001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/DialogSurfaceActivityMapper.java:20:
public class DialogSurfaceActivityMapper extends
IDialogSurfaceActivityMapper.Stub {
On 2016/11/04 00:07:19, boliu wrote:
> @JNINamespace("content") so native side can live in content namespace

Done.

https://codereview.chromium.org/2178973004/diff/320001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/DialogSurfaceActivityMapper.java:24:
public void postWindowToken(int rendererPid, int renderFrameId,
IDialogSurfaceHolder holder) {
On 2016/11/04 00:07:19, boliu wrote:
> I didn't find the caller of this binder call? wanted to ensure pid can't be
> faked

i split this cl in half, and the caller isn't here.

pid can be faked.  i'm in the process of replacing with UnguessableToken.  at
the time this was written, UnguessableToken wasn't ready yet.  i've got java
bindings for it out in a separate CL.

https://codereview.chromium.org/2178973004/diff/320001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/DialogSurfaceActivityMapper.java:45:
Log.e(TAG, "Unable to post window token to DialogSurfaceHolder", e);
On 2016/11/04 00:07:19, boliu wrote:
> Log.d (to avoid bloating release binary)

Done.

boliu

using CVC to listen to WindowAndroid changes is fine. Although that code is not in ...

4 years, 1 month ago (2016-11-14 16:39:25 UTC) #57

liberato (no reviews please)

i merged the CLs together, per offline discussion. i also simplified DialogSurface a bit -- ...

4 years ago (2016-12-20 17:16:38 UTC) #59

i merged the CLs together, per offline discussion.

i also simplified DialogSurface a bit -- realized late last week that it's much
simpler if it doesn't try to re-create the surface on a token loss / switch. 
instead, notifies the client app and lets it decide what to do.

summary of where to start:

DialogSurfaceImpl and DialogSurfaceCore do the work to handle a single
DialogSurface.  application controls a DS instance via dialog_surface.cc, which
is the native GPU part =JNI=> DialogSurfaceWrapper =Binder to browser=>
DialogSurfaceImpl =messages posted to correct thread=> DialogSurfaceCore.

DialogSurfaceWindowTokenProvider(Impl) - registers with CVC and sends window
tokens to DialogSurfaceImpl (=> Core).

feedback to the application follows this path:

DialogSurfaceCore =Binder to GPU=> DialogSurfaceCallbackWrapepr =JNI=>
dialog_surface_callback.cc => client-provided chrome callback

manual testing with a modified version of AVDA works pretty well.  tab transfer
from a chrome custom tabs demo app seems okay too -- closes up the DS instance
cleanly, and future DS instances use the new token.  hard to test every case
until it's fully integrated into AVDA, but it's at least believably close.

thanks
-fl

https://codereview.chromium.org/2178973004/diff/360001/content/app/android/ch...
File content/app/android/child_process_service_impl.cc (right):

https://codereview.chromium.org/2178973004/diff/360001/content/app/android/ch...
content/app/android/child_process_service_impl.cc:59:
gpu::DialogSurfaceLookup::InitInstance(NULL);
On 2016/11/14 16:39:24, boliu wrote:
> nit: new code still prefers nullptr

Done.

https://codereview.chromium.org/2178973004/diff/360001/content/app/android/ch...
content/app/android/child_process_service_impl.cc:155:
content::Java_ChildProcessServiceImpl_getDialogSurfaceManager(
On 2016/11/14 16:39:24, boliu wrote:
> nit: content:: part not ncessary, also I think explicit ScopedJavaLocalRef
> declaration isn't necessary either, although I didn't build and check exactly
> what the generated jni method returns..

Done.

https://codereview.chromium.org/2178973004/diff/360001/content/browser/media/...
File content/browser/media/android/dialog_surface_activity_mapper.cc (right):

https://codereview.chromium.org/2178973004/diff/360001/content/browser/media/...
content/browser/media/android/dialog_surface_activity_mapper.cc:18: static
base::android::ScopedJavaGlobalRef<jobject> GetContentViewCore(
On 2016/11/14 16:39:24, boliu wrote:
> anonymous namespace over static

Done.

https://codereview.chromium.org/2178973004/diff/360001/content/browser/media/...
content/browser/media/android/dialog_surface_activity_mapper.cc:22:
base::android::ScopedJavaGlobalRef<jobject> nullref;
On 2016/11/14 16:39:24, boliu wrote:
> nit: return nullptr works I think

didn't know that.  thanks!

https://codereview.chromium.org/2178973004/diff/360001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/DialogSurfaceActivityMapper.java
(right):

https://codereview.chromium.org/2178973004/diff/360001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/DialogSurfaceActivityMapper.java:40:
ViewGroup viewGroup = cvc.getContainerView();
On 2016/11/14 16:39:24, boliu wrote:
> not needed

Done.

https://codereview.chromium.org/2178973004/diff/360001/content/public/android...
File
content/public/android/java/src/org/chromium/content/common/IChildProcessCallback.aidl
(right):

https://codereview.chromium.org/2178973004/diff/360001/content/public/android...
content/public/android/java/src/org/chromium/content/common/IChildProcessCallback.aidl:29:
IBinder getDialogSurfaceManager();
On 2016/11/14 16:39:25, boliu wrote:
> hmm, surprising that aidl security review isn't enforced by owner files, these
> are supposed to be IPCs. applies to new aidls in media as well
> 
> This is the only instance I found where security team (well, only one member
of
> security team) is marked no parent owner of aidls:
> https://cs.chromium.org/chromium/src/content/public/android/OWNERS?rcl=0&l=8

good catch.  i'll make sure that these get reviewed.

https://codereview.chromium.org/2178973004/diff/360001/media/base/android/dia...
File media/base/android/dialog_surface_manager.cc (right):

https://codereview.chromium.org/2178973004/diff/360001/media/base/android/dia...
media/base/android/dialog_surface_manager.cc:74: return
std::unique_ptr<DialogSurfaceHolder>();
On 2016/11/14 16:39:25, boliu wrote:
> nit: return nullptr

code was removed entirely.

https://codereview.chromium.org/2178973004/diff/360001/media/base/android/jav...
File
media/base/android/java/src/org/chromium/media/DialogSurfaceManagerImpl.java
(right):

https://codereview.chromium.org/2178973004/diff/360001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceManagerImpl.java:52:
private static final int sMaxSurfaces = 1;
On 2016/11/14 16:39:25, boliu wrote:
> stype: MAX_SURFACES

Done.

https://codereview.chromium.org/2178973004/diff/360001/ui/android/java/src/or...
File ui/android/java/src/org/chromium/ui/base/WindowAndroid.java (right):

https://codereview.chromium.org/2178973004/diff/360001/ui/android/java/src/or...
ui/android/java/src/org/chromium/ui/base/WindowAndroid.java:676: return
activity.getWindow().getDecorView().getRootView().getWindowToken();
On 2016/11/14 16:39:25, boliu wrote:
> peekDecorView might be safer, but need a null check
> 
> is getRootView required?

peekDecorView: done.
getRootView: now that you mention it, i don't think so.  i'll check and remove
if not.

boliu

read the media stuff, which means I don't actually have the full picture in my ...

3 years, 11 months ago (2017-01-04 01:48:45 UTC) #60

boliu

https://codereview.chromium.org/2178973004/diff/420001/content/app/android/child_process_service_impl.cc File content/app/android/child_process_service_impl.cc (right): https://codereview.chromium.org/2178973004/diff/420001/content/app/android/child_process_service_impl.cc#newcode111 content/app/android/child_process_service_impl.cc:111: return // base::android::ScopedJavaLocalRef<jobject>( commented out code https://codereview.chromium.org/2178973004/diff/420001/content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java File content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java ...

3 years, 11 months ago (2017-01-04 23:14:39 UTC) #61

liberato (no reviews please)

thanks for the feedback. i'll do the class renaming as a separate PS. this CL ...

3 years, 11 months ago (2017-01-11 22:17:58 UTC) #62

thanks for the feedback.  i'll do the class renaming as a separate PS.  this CL
also still uses CVC, since i wasn't able to figure out how to avoid it.  i'll
probably have to chat with you offline about how to do it.

thanks
-fl

https://codereview.chromium.org/2178973004/diff/420001/content/app/android/ch...
File content/app/android/child_process_service_impl.cc (right):

https://codereview.chromium.org/2178973004/diff/420001/content/app/android/ch...
content/app/android/child_process_service_impl.cc:111: return  //
base::android::ScopedJavaLocalRef<jobject>(
On 2017/01/04 23:14:39, boliu wrote:
> commented out code

Done.

https://codereview.chromium.org/2178973004/diff/420001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java
(right):

https://codereview.chromium.org/2178973004/diff/420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ChildProcessLauncher.java:838:
return DialogSurfaceManagerImpl.instance(
On 2017/01/04 23:14:39, boliu wrote:
> getInstance a bit more idiomatic I think

Done.

https://codereview.chromium.org/2178973004/diff/420001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
(right):

https://codereview.chromium.org/2178973004/diff/420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2849:
if (mObserver != null) mObserver.onAttachedToWindowAndroid(this);
On 2017/01/04 23:14:39, boliu wrote:
> sigh.. I should really unify these two different "attachToWindow" code paths,
> there is no reason they are different
> 
> also, taking a step back, we are trying to get rid of ContentViewCore, so
adding
> more things here is not ideal. the attach/detach event observer probably
should
> live on ViewAndroid (which only exists in c++ code, though there is no reason
it
> can't have java observers..)

i didn't see an obvious way to do this.  i'll re-check, and probably chat
offline to see how to proceed.

https://codereview.chromium.org/2178973004/diff/420001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/DialogSurfaceWindowTokenProviderImpl.java
(right):

https://codereview.chromium.org/2178973004/diff/420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/DialogSurfaceWindowTokenProviderImpl.java:23:
public class DialogSurfaceWindowTokenProviderImpl
On 2017/01/04 23:14:39, boliu wrote:
> I can see why the Manager is singleton, but I don't see why this one is also
> (roughly) a global singleton as well
> 
> feels like should have one of these for every WebContents/ContentViewCore?

most of the code is to look up WebContents given (renderer pid, render frame
id), which has to be a singleton.  it could be moved into Mgr, i suppose, but
then mgr would have to move into content/browser.  i was trying to keep it out
of there, so that it was very limited in what deps it can have.

or maybe, i've misunderstood what you're suggesting. :)

https://codereview.chromium.org/2178973004/diff/420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/DialogSurfaceWindowTokenProviderImpl.java:27:
private class ClientSet extends HashSet<DialogSurfaceWindowTokenProvider.Client>
{}
On 2017/01/04 01:48:43, boliu wrote:
> these classes should be static

Done.

https://codereview.chromium.org/2178973004/diff/420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/DialogSurfaceWindowTokenProviderImpl.java:139:
Activity activity = cvc.getWindowAndroid().getActivity().get();
On 2017/01/04 23:14:39, boliu wrote:
> call getWindowToken?

erm, good point.

https://codereview.chromium.org/2178973004/diff/420001/content/public/android...
File
content/public/android/java/src/org/chromium/content/common/IChildProcessCallback.aidl
(right):

https://codereview.chromium.org/2178973004/diff/420001/content/public/android...
content/public/android/java/src/org/chromium/content/common/IChildProcessCallback.aidl:22:
IBinder getDialogSurfaceManager();
On 2017/01/04 23:14:39, boliu wrote:
> Manager only has a single method. Is it going to get more? Maybe it should
just
> be folded into this interface?

i expect that it will get more, based on my discussions with the VR folks.  it
may need to be configured to talk to GVR.

https://codereview.chromium.org/2178973004/diff/420001/gpu/ipc/common/android...
File gpu/ipc/common/android/dialog_surface_lookup.h (right):

https://codereview.chromium.org/2178973004/diff/420001/gpu/ipc/common/android...
gpu/ipc/common/android/dialog_surface_lookup.h:16: class GPU_EXPORT
DialogSurfaceLookup {
On 2017/01/04 23:14:39, boliu wrote:
> DialogSurface*Manager*Lookup?

good point.  Done.

https://codereview.chromium.org/2178973004/diff/420001/gpu/ipc/common/android...
gpu/ipc/common/android/dialog_surface_lookup.h:24: virtual
base::android::ScopedJavaLocalRef<jobject>
On 2017/01/04 23:14:39, boliu wrote:
> should document what this object actually is

Done.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
File media/base/android/dialog_surface.cc (right):

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
media/base/android/dialog_surface.cc:23: #include
"jni/DialogSurfaceWrapper_jni.h"
On 2017/01/04 01:48:43, boliu wrote:
> there is somewhat of a tradition to call the native thing that implements java
> jni the same name as the java thing (I'm sure you can find exceptions
though..)
> so how about rename the native side DialogSurfaceWrapper?

i'll put out a separate PS with the renaming per our offline discussion.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
media/base/android/dialog_surface.cc:41: CHECK(env);
On 2017/01/04 01:48:43, boliu wrote:
> there aren't a lot of CHECK(env), or even DCHECKs in existing code, I guess
> everything just assumes it works, and avoids the code bloat

Done, and elsewhere.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
media/base/android/dialog_surface.cc:48: env,
Java_DialogSurfaceWrapper_wrap(env, unwrapped_holder.obj()).obj());
On 2017/01/04 01:48:43, boliu wrote:
> are either .obj() here necessary? I'd hope everything is just take care of
> behind the scenes, hmm maybe the inner one is required..

turns out that the outer one is required, but the inner one is not.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
media/base/android/dialog_surface.cc:78: android_surface_ =
On 2017/01/04 01:48:43, boliu wrote:
> thread check here?

Done.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
File media/base/android/dialog_surface.h (right):

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
media/base/android/dialog_surface.h:39: // These must match DialogSurface.java
On 2017/01/04 01:48:44, boliu wrote:
> fwiw there is an automated way to generate enums that match in java and
native,
> eg net_errors_java. Though might be a bit overkill here..

Acknowledged.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
File media/base/android/dialog_surface_callback.cc (right):

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
media/base/android/dialog_surface_callback.cc:40: map_[id] = thiz;
On 2017/01/04 01:48:44, boliu wrote:
> check nothing is being overridden here?

Done.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
media/base/android/dialog_surface_callback.cc:64: task_runner =
thiz->GetTaskRunner();
On 2017/01/04 01:48:44, boliu wrote:
> is it overkill to support callbacks from multiple arbitrary threads?
> 
> maybe should make task_runner_ const to avoid threading issues

there's a replacement for AVDA that's in the works which will likely use a
private thread to do most work, including process callbacks.  i didn't want this
to memorize the task runner of the first thing that uses it.

whether the private thread is "private to all decoders" or "private to one
instance of a decoder" is up in the air.  either, though, "not the gpu main
thread" seems flaky if we ever want to use this for something other than video
decoding.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
media/base/android/dialog_surface_callback.cc:72: DialogSurfaceCallback* thiz =
nullptr;
On 2017/01/04 01:48:44, boliu wrote:
> lock_.AssertAcquired()

Done.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
media/base/android/dialog_surface_callback.cc:129: void
DialogSurfaceCallback::OnCallbackProperThread(
On 2017/01/04 01:48:44, boliu wrote:
> hmm, if we could have passed a WeakPtr opaquely through java and back, how
much
> of this stuff becomes unnecessary?

good question.  never thought of it.

i think that if we still need to remember a task runner, then not too much is
saved.  if we could use a single task runner for all, then yeah, pretty much all
of it can be taken out.

but, see above for why i think that we can't get around remembering a task
runner.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
media/base/android/dialog_surface_callback.cc:200: event.Wait();
On 2017/01/04 01:48:44, boliu wrote:
> why does this need to be synchronous?
> 
> I think blocking a binder thread is probably like blocking the IO thread in
> chrome, it's bad bad bad

it has to be synchronous because android's surfaceDestroyed() callback requires
it.

as long as one doesn't run out of threads in the binder pool, i think that this
should be okay.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
File media/base/android/dialog_surface_callback.h (right):

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
media/base/android/dialog_surface_callback.h:43: DialogSurfaceCallback(const
DialogSurface::Callback& callback);
On 2017/01/04 01:48:44, boliu wrote:
> explicit

Done.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
media/base/android/dialog_surface_callback.h:56: jobject obj() const { return
j_surface_callback_.obj(); }
On 2017/01/04 01:48:44, boliu wrote:
> hmm, having an obj() method is generally a red flag that it's a class is
> unnecessarily exposing internals
> 
> I don't have any good ideas for avoiding this one though, but something to
think
> about..

Acknowledged.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
File media/base/android/dialog_surface_manager.cc (right):

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
media/base/android/dialog_surface_manager.cc:26: using
base::android::ConvertJavaStringToUTF8;
On 2017/01/04 01:48:44, boliu wrote:
> some of these are not needed, which probably means some includes are not
needed
> as well, I didn't check all the files though..

thought i cleaned these up.  sorry about that.

done, and elsewhere.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
media/base/android/dialog_surface_manager.cc:78: if (result.obj() != nullptr) {
On 2017/01/04 01:48:44, boliu wrote:
> nit: !result.is_null() reads better..

Done.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
File media/base/android/dialog_surface_manager.h (right):

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
media/base/android/dialog_surface_manager.h:52: protected:
On 2017/01/04 01:48:44, boliu wrote:
> why protected? is there a subclass? (in which case destructor needs to be
> virtual)

no subclass.  private, done.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/DialogSurfaceCore.java
(right):

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceCore.java:26:
private static final String TAG = "cr_media";
On 2017/01/04 01:48:44, boliu wrote:
> generally, this is just the name of the class, the "cr_" art is added
> automatically by base.Log I think

not sure.  i copied the pattern from MediaCodecBridge.

personally, i prefer the class name too, so i've changed it.  shortened, since i
think there's a low length limit before it truncates.  and elsewhere.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceCore.java:114:
WindowManager.LayoutParams layoutParams = new WindowManager.LayoutParams();
On 2017/01/04 01:48:44, boliu wrote:
> ditto about allocating java objects in calls that could happen every frame

Done.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceCore.java:150:
(Integer) layoutParams.getClass().getField("privateFlags").get(layoutParams);
On 2017/01/04 01:48:44, boliu wrote:
> err, reflection? hard coded values?

PRIVATE_FLAG_NO_MOVE_ANIMATION -- there's no other way to do this,
unfortunately.  otherwise, the thing slides around the screen when it's resized.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceCore.java:209:
Log.e(TAG, "enterDestroyedState: callback failed: " + e);
On 2017/01/04 01:48:44, boliu wrote:
> all logs probably should be .d so they get stripped out of release builds (I
> don't think there's the equivalent of DLOG/VLOG for java yet)

Done.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceCore.java:233: //
Same token, do nothing.
On 2017/01/04 01:48:44, boliu wrote:
> haven't read the window token stuff yet.. but is this expected to happen?

i don't think that it should, but it seemed harmless.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceCore.java:239:
layoutSurface(mInitialX, mInitialY, mInitialWidth, mInitialHeight);
On 2017/01/04 01:48:44, boliu wrote:
> what if an layout happens before window token is received? right now that
layout
> is just dropped, is that correct?

it's okay, since the application shouldn't be positioning the surface before it
gets notification that it's been created.  however, now that you mention it,
it's just as easy to make it stick around.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/DialogSurfaceImpl.java
(right):

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceImpl.java:32: // the
client from deadlocking on mLock.
On 2017/01/04 01:48:44, boliu wrote:
> fwiw, synchronized blocks in java is re-entrant :)

not sure that they'd arrive on the same thread.  the client will thread hop away
from the binder thread, and binder will grab a new thread from the pool on our
side if it calls back in as a result.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceImpl.java:74:
protected void finalize() throws Throwable {
On 2017/01/04 01:48:44, boliu wrote:
> I gave a talk once about never using finalizers..
> 
> in this case, is asBinder().linkToDeath ok?

not sure.  the problem is that there's only the callback on the remote side, and
we have no guarantee that it ever dies.  i.e., a malicious client could drop our
IBinder stub, we'd become unreferenced, and nobody would call release.

methinks that we need this one, in case the client misbehaves.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceImpl.java:121:
Runnable r = new Runnable() {
On 2017/01/04 01:48:44, boliu wrote:
> if this could happen every frame, can consider saving this runnable to avoid
gc
> churn

it can, in principle, if we ever try to use this for embedded content.  for full
screen, i'd expect one or two calls and that's it.

if we ever try embedded, then i think we'll need to improve this.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
File
media/base/android/java/src/org/chromium/media/DialogSurfaceManagerImpl.java
(right):

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceManagerImpl.java:45:
// loop.  Including a looper on it is not a good idea.
On 2017/01/04 23:14:39, boliu wrote:
> the browser UI thread does have a java looper, so if this is only ever runs in
> the browser, can just use the UI thread

i want to avoid that so that synchronous callbacks for surface destruction don't
block the ui thread.  i'll update the comment to make that clearer.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceManagerImpl.java:77:
@CalledByNative
On 2017/01/04 01:48:44, boliu wrote:
> this class doesn't generate jni code?

whoops.  thanks.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceManagerImpl.java:100:
if (mNumSurfaces > 0) mNumSurfaces--;
On 2017/01/04 01:48:44, boliu wrote:
> this should be an assert? (which works as DCHECKs now, apparently)

Done.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
File
media/base/android/java/src/org/chromium/media/DialogSurfaceManagerWrapper.java
(right):

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceManagerWrapper.java:31:
class DialogSurfaceManagerWrapper implements IDialogSurfaceManager {
On 2017/01/04 01:48:44, boliu wrote:
> ditto about not needing to inherit from IDialogSurfaceManager here

will be removed along with the renaming in a separate PS.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/DialogSurfaceWrapper.java
(right):

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceWrapper.java:24:
public DialogSurfaceWrapper(IDialogSurface surface) {
On 2017/01/04 01:48:45, boliu wrote:
> private?

good point, done.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceWrapper.java:35:
public void release() throws RemoteException {
On 2017/01/04 01:48:44, boliu wrote:
> throwing exception doesn't really work for CalledByNative methods. any
exception
> will just immediately crash in native code
> 
> it's probably ok to swallow these exceptions I imagine? so can do that in java
> code, or need to return some kind of error code to native side if that's
> required

yeah, they can be ignored.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceWrapper.java:47:
public IBinder asBinder() {
On 2017/01/04 01:48:45, boliu wrote:
> does returning null here work? won't that break... something? I think you are
> supposed to extend IDialogSurface.Stub, then you don't need to worry about
this
> 
> otoh, does the wrapper need to inherit from aidl interface at all? is anything
> calling this object through aidl? because it looks very confusing that a
method
> is both overriding IInterface (ie it's being called through aidl) and have
> @CalledByNative

i don't think that returning null here will break anything, since binder never
finds out about this object anyway.

it's not needed to inherit from the aidl interface.  however, i wanted to
enforce that it implements IDialogSurface, and this was the only way to do so
that i could see.  i.e., if this ever diverges from IDialogSurface, then
something's broken.

WDYT?

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/med...
File media/base/android/media_jni_registrar.cc (right):

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/med...
media/base/android/media_jni_registrar.cc:14: #include
"media/base/android/media_codec_util.h"
On 2017/01/04 01:48:45, boliu wrote:
> random new include?

Done.

boliu

Didn't look at new code, just replying to comments. I assume there's enough work there ...

3 years, 11 months ago (2017-01-12 20:24:19 UTC) #63

Didn't look at new code, just replying to comments. I assume there's enough work
there :p

Another high level question.. why use binder? Sure the window token needs to be
passed through binder, but nothing else needs to go through binder? Why not IPC
or mojo for all the other things? (Thought about this because
DialogSurfaceWrapper exists purely to make binder calls from native, and mojo
already does that)

https://codereview.chromium.org/2178973004/diff/420001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/DialogSurfaceWindowTokenProviderImpl.java
(right):

https://codereview.chromium.org/2178973004/diff/420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/DialogSurfaceWindowTokenProviderImpl.java:23:
public class DialogSurfaceWindowTokenProviderImpl
On 2017/01/11 22:17:56, liberato wrote:
> On 2017/01/04 23:14:39, boliu wrote:
> > I can see why the Manager is singleton, but I don't see why this one is also
> > (roughly) a global singleton as well
> > 
> > feels like should have one of these for every WebContents/ContentViewCore?
> 
> most of the code is to look up WebContents given (renderer pid, render frame
> id), which has to be a singleton.  it could be moved into Mgr, i suppose, but
> then mgr would have to move into content/browser.

You can put it in a static method or something.

> i was trying to keep it out
> of there, so that it was very limited in what deps it can have.
> 
> or maybe, i've misunderstood what you're suggesting. :)

You didn't misunderstand.

I generally dislike introducing more global state that need to be cleaned up
when individual WebContents goes away. Ideally, this object would be owned by
WebContents; I don't think java WebContents has a WebContentsUserData
equivalent, but ideally should use that if it existed, then don't have to ever
worry about register/unregister client, and cleaning up these global maps/sets
etc.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
File media/base/android/dialog_surface_callback.cc (right):

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
media/base/android/dialog_surface_callback.cc:64: task_runner =
thiz->GetTaskRunner();
On 2017/01/11 22:17:56, liberato wrote:
> On 2017/01/04 01:48:44, boliu wrote:
> > is it overkill to support callbacks from multiple arbitrary threads?
> > 
> > maybe should make task_runner_ const to avoid threading issues
> 
> there's a replacement for AVDA that's in the works which will likely use a
> private thread to do most work, including process callbacks.  i didn't want
this
> to memorize the task runner of the first thing that uses it.

But this CL does exactly that right now..

> 
> whether the private thread is "private to all decoders" or "private to one
> instance of a decoder" is up in the air.

One thread per encoder? seems excessive to me..

> either, though, "not the gpu main
> thread" seems flaky if we ever want to use this for something other than video
> decoding.

Can we add the complexity when it's needed, and not before?

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
media/base/android/dialog_surface_callback.cc:129: void
DialogSurfaceCallback::OnCallbackProperThread(
On 2017/01/11 22:17:56, liberato wrote:
> On 2017/01/04 01:48:44, boliu wrote:
> > hmm, if we could have passed a WeakPtr opaquely through java and back, how
> much
> > of this stuff becomes unnecessary?
> 
> good question.  never thought of it.
> 
> i think that if we still need to remember a task runner, then not too much is
> saved.  if we could use a single task runner for all, then yeah, pretty much
all
> of it can be taken out.
> 
> but, see above for why i think that we can't get around remembering a task
> runner.

What if DialogSurfaceCallback is refcoutnedthreadsafe, and java takes a single
reference? Java has an explicit onDestroy to drop the reference?

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
media/base/android/dialog_surface_callback.cc:200: event.Wait();
On 2017/01/11 22:17:56, liberato wrote:
> On 2017/01/04 01:48:44, boliu wrote:
> > why does this need to be synchronous?
> > 
> > I think blocking a binder thread is probably like blocking the IO thread in
> > chrome, it's bad bad bad
> 
> it has to be synchronous because android's surfaceDestroyed() callback
requires
> it.
> 
> as long as one doesn't run out of threads in the binder pool, i think that
this
> should be okay.

But what if that happens?

Can emulate chrome's sync IPC which does not block the IO thread. Make the
caller block for an async reply back from here.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/DialogSurfaceCore.java
(right):

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceCore.java:26:
private static final String TAG = "cr_media";
On 2017/01/11 22:17:57, liberato wrote:
> On 2017/01/04 01:48:44, boliu wrote:
> > generally, this is just the name of the class, the "cr_" art is added
> > automatically by base.Log I think
> 
> not sure.  i copied the pattern from MediaCodecBridge.

fwiw, it's dynamic:
https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/...

> 
> personally, i prefer the class name too, so i've changed it.  shortened, since
i
> think there's a low length limit before it truncates.  and elsewhere.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceCore.java:150:
(Integer) layoutParams.getClass().getField("privateFlags").get(layoutParams);
On 2017/01/11 22:17:57, liberato wrote:
> On 2017/01/04 01:48:44, boliu wrote:
> > err, reflection? hard coded values?
> 
> PRIVATE_FLAG_NO_MOVE_ANIMATION -- there's no other way to do this,
> unfortunately.  otherwise, the thing slides around the screen when it's
resized.

The bar for using private APIs is very high, ie it's almost never allowed.

It doesn't affect fullscreen, right? Also is it that bad? You can never fully
synchronize surface updates or placements for inline video, so how much worse is
it?

If you really want to do this, then I want to see communication with the android
team to expose these things in the next version of android.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceCore.java:233: //
Same token, do nothing.
On 2017/01/11 22:17:57, liberato wrote:
> On 2017/01/04 01:48:44, boliu wrote:
> > haven't read the window token stuff yet.. but is this expected to happen?
> 
> i don't think that it should, but it seemed harmless.

but this is silencing a potential problem in the future. If this should be an
assert, then assert. Java asserts work like DCHECks now

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/DialogSurfaceImpl.java
(right):

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceImpl.java:32: // the
client from deadlocking on mLock.
On 2017/01/11 22:17:57, liberato wrote:
> On 2017/01/04 01:48:44, boliu wrote:
> > fwiw, synchronized blocks in java is re-entrant :)
> 
> not sure that they'd arrive on the same thread.  the client will thread hop
away
> from the binder thread, and binder will grab a new thread from the pool on our
> side if it calls back in as a result.

Oh you meant re-entrant binder calls.

This comment doesn't seem to actually hold. release does a lot more than just
posting a task.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceImpl.java:74:
protected void finalize() throws Throwable {
On 2017/01/11 22:17:57, liberato wrote:
> On 2017/01/04 01:48:44, boliu wrote:
> > I gave a talk once about never using finalizers..
> > 
> > in this case, is asBinder().linkToDeath ok?
> 
> not sure.  the problem is that there's only the callback on the remote side,
and
> we have no guarantee that it ever dies.  i.e., a malicious client could drop
our
> IBinder stub, we'd become unreferenced, and nobody would call release.
> 
> methinks that we need this one, in case the client misbehaves.

According to a comment on an internal thread, an android engineer said
linkToDeath should work for this case. But can you test it? I tried looking at
the code, but it goes into the kernel, and I'm not brave enough to go further..

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
File
media/base/android/java/src/org/chromium/media/DialogSurfaceManagerImpl.java
(right):

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceManagerImpl.java:45:
// loop.  Including a looper on it is not a good idea.
On 2017/01/11 22:17:57, liberato wrote:
> On 2017/01/04 23:14:39, boliu wrote:
> > the browser UI thread does have a java looper, so if this is only ever runs
in
> > the browser, can just use the UI thread
> 
> i want to avoid that so that synchronous callbacks for surface destruction
don't
> block the ui thread.  i'll update the comment to make that clearer.

Ahh, I was curious what surface view implement, but it looks like it doesn't
bother waiting for gpu to clean up at all? So is blocking in destroy really
necessary? perhaps the gpu code is already robust enough to handle this?

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/DialogSurfaceWrapper.java
(right):

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceWrapper.java:47:
public IBinder asBinder() {
On 2017/01/11 22:17:58, liberato wrote:
> On 2017/01/04 01:48:45, boliu wrote:
> > does returning null here work? won't that break... something? I think you
are
> > supposed to extend IDialogSurface.Stub, then you don't need to worry about
> this
> > 
> > otoh, does the wrapper need to inherit from aidl interface at all? is
anything
> > calling this object through aidl? because it looks very confusing that a
> method
> > is both overriding IInterface (ie it's being called through aidl) and have
> > @CalledByNative
> 
> i don't think that returning null here will break anything, since binder never
> finds out about this object anyway.
> 
> it's not needed to inherit from the aidl interface.  however, i wanted to
> enforce that it implements IDialogSurface, and this was the only way to do so
> that i could see.  i.e., if this ever diverges from IDialogSurface, then
> something's broken.
> 
> WDYT?

I feel is still leads to too much confusion.

liberato (no reviews please)

boliu: thanks for the discussion yesterday. i've verified basic functionality of this PS to know ...

3 years, 10 months ago (2017-02-03 21:28:32 UTC) #64

boliu: thanks for the discussion yesterday.  i've verified basic functionality
of this PS to know that it's a workable design (i.e., it can play video in an
overlay).  PTAL at a high level to see if you agree with the direction.

it's not ready for detailed review yet, but i want to verify that it's aiming at
the right target.

it includes the class renaming that we talked about earlier, too.  to recap:

- IDialogSurfaceManager => IAndroidOverlayProvider
- IDialogSurface => IAndroidOverlay
- dialog_surface.cc => dialog_surface_proxy.cc
- DialogSurfaceWrapper.java => DialogSurfaceProxy.java
- (similar for Provider).
- *Proxy no longer implement the binder interface.

of interest for the CVC discussion:

- ContentViewCore is no longer in the PS.
- dialog_android_overlay.cc has the ContentViewCoreImplObserver.  Instances of
this class are created by DialogAndroidOverlay.java (was DialogSurface).
- cleanup of the callback happens when the CVC is destroyed, or when the surface
is destroyed (either by android or when the AndroidOverlay client closes it)
- IAndroidOverlayProvider is still not one way, but the others are.  this will
be addressed soon.
- IAndroidOverlayCompletion is the (one way) callback that closes the loop to
make onSurfaceDestroyed callback synchronous.

thanks
-fl

https://codereview.chromium.org/2178973004/diff/420001/content/public/android...
File
content/public/android/java/src/org/chromium/content/browser/DialogSurfaceWindowTokenProviderImpl.java
(right):

https://codereview.chromium.org/2178973004/diff/420001/content/public/android...
content/public/android/java/src/org/chromium/content/browser/DialogSurfaceWindowTokenProviderImpl.java:23:
public class DialogSurfaceWindowTokenProviderImpl
On 2017/01/12 20:24:18, boliu wrote:
> On 2017/01/11 22:17:56, liberato wrote:
> > On 2017/01/04 23:14:39, boliu wrote:
> > > I can see why the Manager is singleton, but I don't see why this one is
also
> > > (roughly) a global singleton as well
> > > 
> > > feels like should have one of these for every WebContents/ContentViewCore?
> > 
> > most of the code is to look up WebContents given (renderer pid, render frame
> > id), which has to be a singleton.  it could be moved into Mgr, i suppose,
but
> > then mgr would have to move into content/browser.
> 
> You can put it in a static method or something.
> 
> > i was trying to keep it out
> > of there, so that it was very limited in what deps it can have.
> > 
> > or maybe, i've misunderstood what you're suggesting. :)
> 
> You didn't misunderstand.
> 
> I generally dislike introducing more global state that need to be cleaned up
> when individual WebContents goes away. Ideally, this object would be owned by
> WebContents; I don't think java WebContents has a WebContentsUserData
> equivalent, but ideally should use that if it existed, then don't have to ever
> worry about register/unregister client, and cleaning up these global maps/sets
> etc.

per our offline discussion, this class is not in PS23.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
File media/base/android/dialog_surface_callback.cc (right):

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
media/base/android/dialog_surface_callback.cc:64: task_runner =
thiz->GetTaskRunner();
On 2017/01/12 20:24:18, boliu wrote:
> On 2017/01/11 22:17:56, liberato wrote:
> > On 2017/01/04 01:48:44, boliu wrote:
> > > is it overkill to support callbacks from multiple arbitrary threads?
> > > 
> > > maybe should make task_runner_ const to avoid threading issues
> > 
> > there's a replacement for AVDA that's in the works which will likely use a
> > private thread to do most work, including process callbacks.  i didn't want
> this
> > to memorize the task runner of the first thing that uses it.
> 
> But this CL does exactly that right now..
> 
> > 
> > whether the private thread is "private to all decoders" or "private to one
> > instance of a decoder" is up in the air.
> 
> One thread per encoder? seems excessive to me..
> 
> > either, though, "not the gpu main
> > thread" seems flaky if we ever want to use this for something other than
video
> > decoding.
> 
> Can we add the complexity when it's needed, and not before?

Done.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
media/base/android/dialog_surface_callback.cc:129: void
DialogSurfaceCallback::OnCallbackProperThread(
On 2017/01/12 20:24:18, boliu wrote:
> On 2017/01/11 22:17:56, liberato wrote:
> > On 2017/01/04 01:48:44, boliu wrote:
> > > hmm, if we could have passed a WeakPtr opaquely through java and back, how
> > much
> > > of this stuff becomes unnecessary?
> > 
> > good question.  never thought of it.
> > 
> > i think that if we still need to remember a task runner, then not too much
is
> > saved.  if we could use a single task runner for all, then yeah, pretty much
> all
> > of it can be taken out.
> > 
> > but, see above for why i think that we can't get around remembering a task
> > runner.
> 
> What if DialogSurfaceCallback is refcoutnedthreadsafe, and java takes a single
> reference? Java has an explicit onDestroy to drop the reference?

it can work.

there are lots of thread hopping issues, though.  random thread calls into java
to start the callback => jni posts to native thread, returns or waits.  native
thread processes the callback which might onDestroy on a different thread (gpu
main thread).

we need to make sure that onDestroy doesn't happen while the native post is
still pending.  that can be done by duplicating the ref in the jni call, and
passing it along with the post.

we also need to make sure that onDestroy is synchronized with the random threads
that come into the callback on java.  that would be easy, but we have to make
sure that one can still cause onDestroy from a native callback on a native
thread, which might happen while the (sync) callback is still waiting on a
random jni thread.  could deadlock, i'd suspect, unless we're careful.  probably
onDestroy has to post in java, to prevent a deadlock.

i'll give it a try, and see if it ends up being more or less code.

(later)

ownership gets to be very messy.  i had the callback native object hand a newly
allocated ref to java.  somebody (DialogSurface) has to call a method on the
callback to destroy the extra ref, else it'll live on forever.  i don't see
anything on the java side that we could use to do it automatically.

plus, there are the thread hops mentioned above.

unless you feel strongly, i'd rather keep it this way.  ownership is clearer and
it's hard to leak.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
media/base/android/dialog_surface_callback.cc:200: event.Wait();
On 2017/01/12 20:24:18, boliu wrote:
> On 2017/01/11 22:17:56, liberato wrote:
> > On 2017/01/04 01:48:44, boliu wrote:
> > > why does this need to be synchronous?
> > > 
> > > I think blocking a binder thread is probably like blocking the IO thread
in
> > > chrome, it's bad bad bad
> > 
> > it has to be synchronous because android's surfaceDestroyed() callback
> requires
> > it.
> > 
> > as long as one doesn't run out of threads in the binder pool, i think that
> this
> > should be okay.
> 
> But what if that happens?
> 
> Can emulate chrome's sync IPC which does not block the IO thread. Make the
> caller block for an async reply back from here.

Done.  added IAndroidOverlayCompletion which gets sent to the callback for sync
destroy.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/DialogSurfaceCore.java
(right):

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceCore.java:26:
private static final String TAG = "cr_media";
On 2017/01/12 20:24:19, boliu wrote:
> On 2017/01/11 22:17:57, liberato wrote:
> > On 2017/01/04 01:48:44, boliu wrote:
> > > generally, this is just the name of the class, the "cr_" art is added
> > > automatically by base.Log I think
> > 
> > not sure.  i copied the pattern from MediaCodecBridge.
> 
> fwiw, it's dynamic:
>
https://cs.chromium.org/chromium/src/base/android/java/src/org/chromium/base/...
> 
> > 
> > personally, i prefer the class name too, so i've changed it.  shortened,
since
> i
> > think there's a low length limit before it truncates.  and elsewhere.
> 

Acknowledged.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceCore.java:150:
(Integer) layoutParams.getClass().getField("privateFlags").get(layoutParams);
On 2017/01/12 20:24:19, boliu wrote:
> On 2017/01/11 22:17:57, liberato wrote:
> > On 2017/01/04 01:48:44, boliu wrote:
> > > err, reflection? hard coded values?
> > 
> > PRIVATE_FLAG_NO_MOVE_ANIMATION -- there's no other way to do this,
> > unfortunately.  otherwise, the thing slides around the screen when it's
> resized.
> 
> The bar for using private APIs is very high, ie it's almost never allowed.
> 
> It doesn't affect fullscreen, right? Also is it that bad? You can never fully
> synchronize surface updates or placements for inline video, so how much worse
is
> it?
> 
> If you really want to do this, then I want to see communication with the
android
> team to expose these things in the next version of android.

i'd really like to do this.

we can chat offline about how it will work in O and later.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/DialogSurfaceImpl.java
(right):

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceImpl.java:74:
protected void finalize() throws Throwable {
On 2017/01/12 20:24:19, boliu wrote:
> On 2017/01/11 22:17:57, liberato wrote:
> > On 2017/01/04 01:48:44, boliu wrote:
> > > I gave a talk once about never using finalizers..
> > > 
> > > in this case, is asBinder().linkToDeath ok?
> > 
> > not sure.  the problem is that there's only the callback on the remote side,
> and
> > we have no guarantee that it ever dies.  i.e., a malicious client could drop
> our
> > IBinder stub, we'd become unreferenced, and nobody would call release.
> > 
> > methinks that we need this one, in case the client misbehaves.
> 
> According to a comment on an internal thread, an android engineer said
> linkToDeath should work for this case. But can you test it? I tried looking at
> the code, but it goes into the kernel, and I'm not brave enough to go
further..

i'm not sure that i understand how linkToDeath will solve it.  we have nothing
to linkToDeath on, except the client callback.  but that can outlive |this| by
an arbitrarily long time, at the client's option.  it might even be re-used
across multiple DS instances.

how will that help?

actually, now that i think about it, we won't be finalized anyway. 
getWindowTokenProvider has us.

so maybe i'll drop the finalizer.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
File
media/base/android/java/src/org/chromium/media/DialogSurfaceManagerImpl.java
(right):

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceManagerImpl.java:45:
// loop.  Including a looper on it is not a good idea.
On 2017/01/12 20:24:19, boliu wrote:
> On 2017/01/11 22:17:57, liberato wrote:
> > On 2017/01/04 23:14:39, boliu wrote:
> > > the browser UI thread does have a java looper, so if this is only ever
runs
> in
> > > the browser, can just use the UI thread
> > 
> > i want to avoid that so that synchronous callbacks for surface destruction
> don't
> > block the ui thread.  i'll update the comment to make that clearer.
> 
> Ahh, I was curious what surface view implement, but it looks like it doesn't
> bother waiting for gpu to clean up at all? So is blocking in destroy really
> necessary? perhaps the gpu code is already robust enough to handle this?

i'll have to dig up where it does this, but CVV does require sync cleanup, and
does wait during IPC to the gpu for it to finish.  it's required, else media
codec can crash.

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
File media/base/android/java/src/org/chromium/media/DialogSurfaceWrapper.java
(right):

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/jav...
media/base/android/java/src/org/chromium/media/DialogSurfaceWrapper.java:47:
public IBinder asBinder() {
On 2017/01/12 20:24:19, boliu wrote:
> On 2017/01/11 22:17:58, liberato wrote:
> > On 2017/01/04 01:48:45, boliu wrote:
> > > does returning null here work? won't that break... something? I think you
> are
> > > supposed to extend IDialogSurface.Stub, then you don't need to worry about
> > this
> > > 
> > > otoh, does the wrapper need to inherit from aidl interface at all? is
> anything
> > > calling this object through aidl? because it looks very confusing that a
> > method
> > > is both overriding IInterface (ie it's being called through aidl) and have
> > > @CalledByNative
> > 
> > i don't think that returning null here will break anything, since binder
never
> > finds out about this object anyway.
> > 
> > it's not needed to inherit from the aidl interface.  however, i wanted to
> > enforce that it implements IDialogSurface, and this was the only way to do
so
> > that i could see.  i.e., if this ever diverges from IDialogSurface, then
> > something's broken.
> > 
> > WDYT?
> 
> I feel is still leads to too much confusion.

per our offline renaming discussion, this won't be an I* anymore.  i just forgot
to update my CL comment before sending.  sorry about that.

liberato (no reviews please)

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dialog_surface_callback.cc File media/base/android/dialog_surface_callback.cc (right): https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dialog_surface_callback.cc#newcode129 media/base/android/dialog_surface_callback.cc:129: void DialogSurfaceCallback::OnCallbackProperThread( On 2017/02/03 21:28:32, liberato wrote: > On ...

3 years, 10 months ago (2017-02-07 00:20:20 UTC) #65

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
File media/base/android/dialog_surface_callback.cc (right):

https://codereview.chromium.org/2178973004/diff/420001/media/base/android/dia...
media/base/android/dialog_surface_callback.cc:129: void
DialogSurfaceCallback::OnCallbackProperThread(
On 2017/02/03 21:28:32, liberato wrote:
> On 2017/01/12 20:24:18, boliu wrote:
> > On 2017/01/11 22:17:56, liberato wrote:
> > > On 2017/01/04 01:48:44, boliu wrote:
> > > > hmm, if we could have passed a WeakPtr opaquely through java and back,
how
> > > much
> > > > of this stuff becomes unnecessary?
> > > 
> > > good question.  never thought of it.
> > > 
> > > i think that if we still need to remember a task runner, then not too much
> is
> > > saved.  if we could use a single task runner for all, then yeah, pretty
much
> > all
> > > of it can be taken out.
> > > 
> > > but, see above for why i think that we can't get around remembering a task
> > > runner.
> > 
> > What if DialogSurfaceCallback is refcoutnedthreadsafe, and java takes a
single
> > reference? Java has an explicit onDestroy to drop the reference?
> 
> it can work.
> 
> there are lots of thread hopping issues, though.  random thread calls into
java
> to start the callback => jni posts to native thread, returns or waits.  native
> thread processes the callback which might onDestroy on a different thread (gpu
> main thread).
> 
> we need to make sure that onDestroy doesn't happen while the native post is
> still pending.  that can be done by duplicating the ref in the jni call, and
> passing it along with the post.
> 
> we also need to make sure that onDestroy is synchronized with the random
threads
> that come into the callback on java.  that would be easy, but we have to make
> sure that one can still cause onDestroy from a native callback on a native
> thread, which might happen while the (sync) callback is still waiting on a
> random jni thread.  could deadlock, i'd suspect, unless we're careful. 
probably
> onDestroy has to post in java, to prevent a deadlock.
> 
> i'll give it a try, and see if it ends up being more or less code.
> 
> (later)
> 
> ownership gets to be very messy.  i had the callback native object hand a
newly
> allocated ref to java.  somebody (DialogSurface) has to call a method on the
> callback to destroy the extra ref, else it'll live on forever.  i don't see
> anything on the java side that we could use to do it automatically.
> 
> plus, there are the thread hops mentioned above.
> 
> unless you feel strongly, i'd rather keep it this way.  ownership is clearer
and
> it's hard to leak.

ah, the problem with my attempt with WeakRefs was that i'm silly.  :)  i tried
this again today, and it seems to fit much better this time.

still re-writing, so maybe i'm being too optimistic.  seems likely to work,
though.

thanks!

boliu

I feel threading is still too hard to understand and trace at the moment. Eg ...

3 years, 10 months ago (2017-02-08 00:01:59 UTC) #66

liberato (no reviews please)

3 years, 10 months ago (2017-02-08 02:34:12 UTC) #67

On 2017/02/08 00:01:59, boliu wrote:
> I feel threading is still too hard to understand and trace at the moment. Eg
> DialogAndroidOverlay[Core] (java and native parts) run on binder threads, UI
> thread, and dialog thread. AndroidOverlayCallback still looks pretty complex
as
> well, though I guess that's getting better. 
> 
> I don't know if there's general good solution though. wish we could tell
binder
> to call us on a specific thread like how things work with chrome IPC.
> 
> The next best thing is keep objects single threaded where possible, and
document
> extensively lifetime and threading info.
> 
> And now that almost everything is oneway, there's one more binder edge case:
can
> a single binder object be called on multiple binder threads *concurrently*?
I'm
> guessing answer is yes, in which case... I probably need to look at everything
> again.
> 
> inline comments are all minor things
> 
>
https://codereview.chromium.org/2178973004/diff/480001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java
> (right):
> 
>
https://codereview.chromium.org/2178973004/diff/480001/content/public/android...
>
content/public/android/java/src/org/chromium/content/app/ChildProcessServiceImpl.java:403:
> try {
> all the other calls have a null check for mCallback
> 
>
https://codereview.chromium.org/2178973004/diff/480001/content/public/android...
> File
>
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java
> (right):
> 
>
https://codereview.chromium.org/2178973004/diff/480001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java:73:
> @CalledByNative
> not needed, this doesn't generate jni headers
> 
>
https://codereview.chromium.org/2178973004/diff/480001/content/public/android...
>
content/public/android/java/src/org/chromium/content/browser/androidoverlay/AndroidOverlayProviderImpl.java:113:
> // Called with mLock held only.
> synhronized blocks are re-entrant safe, so rather than a comment and hoping
> caller remembers, could just add synchronized block here as well
> 
>
https://codereview.chromium.org/2178973004/diff/480001/media/base/android/and...
> File media/base/android/android_overlay_callback.cc (right):
> 
>
https://codereview.chromium.org/2178973004/diff/480001/media/base/android/and...
> media/base/android/android_overlay_callback.cc:78: task_runner_ =
> base::ThreadTaskRunnerHandle::Get();
> else DCHECK it's still the same thread?
> 
>
https://codereview.chromium.org/2178973004/diff/480001/media/base/android/jav...
> File
media/base/android/java/src/org/chromium/media/AndroidOverlayCallback.java
> (right):
> 
>
https://codereview.chromium.org/2178973004/diff/480001/media/base/android/jav...
> media/base/android/java/src/org/chromium/media/AndroidOverlayCallback.java:22:
> public AndroidOverlayCallback(long nativeId) {
> private
> 
>
https://codereview.chromium.org/2178973004/diff/480001/media/base/android/jav...
> media/base/android/java/src/org/chromium/media/AndroidOverlayCallback.java:27:
> public static AndroidOverlayCallback create(long nativeId) {
> private
> 
>
https://codereview.chromium.org/2178973004/diff/480001/media/base/android/jav...
> media/base/android/java/src/org/chromium/media/AndroidOverlayCallback.java:45:
}
> catch (RemoteException e) {
> Feel free to Log.d errors like this, Log.d is stripped out of release builds
> 
>
https://codereview.chromium.org/2178973004/diff/480001/media/base/android/jav...
> File
>
media/base/android/java/src/org/chromium/media/AndroidOverlayProviderProxy.java
> (right):
> 
>
https://codereview.chromium.org/2178973004/diff/480001/media/base/android/jav...
>
media/base/android/java/src/org/chromium/media/AndroidOverlayProviderProxy.java:43:
> throws RemoteException {
> any exception thrown by @CalledByNative methods will lead to an immediate
native
> crash
> 
> in this case, null is a valid return value rather than a failure case, so need
> to catch that exception here and return null instead
> 
>
https://codereview.chromium.org/2178973004/diff/480001/media/base/android/jav...
> File media/base/android/java/src/org/chromium/media/AndroidOverlayProxy.java
> (right):
> 
>
https://codereview.chromium.org/2178973004/diff/480001/media/base/android/jav...
> media/base/android/java/src/org/chromium/media/AndroidOverlayProxy.java:31:
> public void release() {
> private here and below
> 
>
https://codereview.chromium.org/2178973004/diff/480001/media/base/android/jav...
> File
media/base/android/java/src/org/chromium/media/IAndroidOverlayCallback.aidl
> (right):
> 
>
https://codereview.chromium.org/2178973004/diff/480001/media/base/android/jav...
>
media/base/android/java/src/org/chromium/media/IAndroidOverlayCallback.aidl:12:
> // Notify the client that the Androi Surface has been destroyed.  The client
> android typo

thanks for the comments.  i'm overhauling the callbacks to make overlay creation
oneway as well.

just a few quick replies.

> run on binder threads, UI thread, and dialog thread.

the browser UI thread isn't involved.  there's one dialog thread for all
DialogSurface instances.  android callbacks (surfaceCreated) come in on this
thread also.  we want to preserve that, since it keeps us from blocking the UI
thread when a surface is destroyed.

> I don't know if there's general good solution though. wish we could tell
binder
> to call us on a specific thread like how things work with chrome IPC.

DialogSurfaceImpl tries to simulate this -- it just posts to the handler for all
the binder calls, except release() where it has some internal state to clean up.
 DialogSurfaceCore is single-threaded (Dialog thread).  i'll add docs to make it
clearer.  goal is to need to think about only one binder thread and one
DialogSurface thread.

> a single binder object be called on multiple binder threads *concurrently*?
I'm

you're right, it can.  that's why DialogSurfaceImpl synchronizes immediately. 
actually, a misbehaving client could do so even without oneway, so we have to
handle it.  it just calls from multiple threads on the client side.  oneway just
makes it easier.

thanks
-fl

Issue 2178973004: DialogSurfaceManager implementation. (Closed)

Description

Patch Set 1 #

Patch Set 2 : DialogSurfaceManager implementation. #

Patch Set 3 : rebased. #

Patch Set 4 : moved jni out of content/app #

Patch Set 5 : started kinda using mHandler #

Patch Set 6 : cl feedback #

Patch Set 7 : cl feedback #

Patch Set 8 : gyp #

Patch Set 9 : cl feedback #

Patch Set 10 : cl feedback #

Patch Set 11 : addedd DVLOG #

Patch Set 12 : removed unneeded jni registration fns #

Patch Set 13 : renamed DialogSurface* to AndroidSurface* #

Patch Set 14 : DialogSurface => DialogSurfaceHolder #

Patch Set 15 : window token post for thread safety #

Patch Set 16 : added locking for SurfaceManager::instance #

Patch Set 17 : DialogSurfaceManager.java => DialogSurfaceManagerImpl.java and stopped using activity for window to… #

Patch Set 18 : switched to UnguessableToken #

Patch Set 19 : removed whitespace changes #

Patch Set 20 : simplified DialogSurface, unified CL #

Patch Set 21 : removed IDialogSurfaceActivityMapper from common.aidl #

Patch Set 22 : cl feedback #

Patch Set 23 : renamed to AndroidOverlay, stopped modifying ContentViewCore #

Patch Set 24 : cleanup #

Messages