Add explicit flush after copyTextureCHROMIUM to make the change visible accross contexts.

Add explicit flush after copyTextureCHROMIUM to make the change visible across contexts.

This change ensures to copy *the current video frame* instead of to copy *a future video frame* at flush time. 

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

[junj, 2014-03-18 11:15:04]

This is a complementary patch with https://codereview.chromium.org/197213014/.
Please help to take a look.

[no sievers, 2014-03-18 22:47:32]

https://codereview.chromium.org/200763011/diff/1/content/renderer/media/rende...
File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right):

https://codereview.chromium.org/200763011/diff/1/content/renderer/media/rende...
content/renderer/media/renderer_gpu_video_accelerator_factories.cc:189:
context->flush();
Do we need this one? We are reading back the texture here only, so should be ok.
Well, the ReadPixels() implies a finish anyways.

[no sievers, 2014-03-18 22:54:29]

https://codereview.chromium.org/200763011/diff/1/content/renderer/media/andro...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/200763011/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:466:
web_graphics_context->flush();
Actually, you don't need this flush either since you are using the target
context to do the copy (I believe). And the source texture will not be changed
after it was initially created (and we call glFlush() there).

[junj, 2014-03-19 07:47:37]

Hi, sievers. Thanks for your comments and suggestions. Please see my comments
below.

https://codereview.chromium.org/200763011/diff/1/content/renderer/media/andro...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/200763011/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:466:
web_graphics_context->flush();
On 2014/03/18 22:54:29, sievers wrote:
> Actually, you don't need this flush either since you are using the target
> context to do the copy (I believe). And the source texture will not be changed
> after it was initially created (and we call glFlush() there).
> 

My understanding is that the video contents associated with the source texture
might change in the *video decoder context*, i.e. when a new video frame is
produced. The flush() here is just to make sure the *current video frame* is
copied instead of a *future video frame* being copied. That is, if no flush() is
here, the source texture might be override by new contents later and the target
context just sees the future content.

Maybe offset by one or two frames is not noticeable for video case, but just to
make the code seems more reasonable.

https://codereview.chromium.org/200763011/diff/1/content/renderer/media/rende...
File content/renderer/media/renderer_gpu_video_accelerator_factories.cc (right):

https://codereview.chromium.org/200763011/diff/1/content/renderer/media/rende...
content/renderer/media/renderer_gpu_video_accelerator_factories.cc:189:
context->flush();
On 2014/03/18 22:47:32, sievers wrote:
> Do we need this one? We are reading back the texture here only, so should be
ok.
> Well, the ReadPixels() implies a finish anyways.

Yes, the flush() is not needed here. I made a mistake and thought the *gles2*
was in another context.

[no sievers, 2014-03-19 17:02:19]

https://codereview.chromium.org/200763011/diff/1/content/renderer/media/andro...
File content/renderer/media/android/webmediaplayer_android.cc (right):

https://codereview.chromium.org/200763011/diff/1/content/renderer/media/andro...
content/renderer/media/android/webmediaplayer_android.cc:466:
web_graphics_context->flush();
On 2014/03/19 07:47:37, junj wrote:
> On 2014/03/18 22:54:29, sievers wrote:
> > Actually, you don't need this flush either since you are using the target
> > context to do the copy (I believe). And the source texture will not be
changed
> > after it was initially created (and we call glFlush() there).
> > 
> 
> My understanding is that the video contents associated with the source texture
> might change in the *video decoder context*, i.e. when a new video frame is
> produced. The flush() here is just to make sure the *current video frame* is
> copied instead of a *future video frame* being copied. That is, if no flush()
is
> here, the source texture might be override by new contents later and the
target
> context just sees the future content.
> 
> Maybe offset by one or two frames is not noticeable for video case, but just
to
> make the code seems more reasonable.

The contents for the StreamTexture are actually updated to the latest on use
(when you draw with it, it happens through WillUseTexImage() called from the
decoder in, for example, DrawElements and CopyTextureCHROMIUM).

I could see maybe how it would be a little odd if somebody defined a WebGL
texture from a video element and only flushed the context much later,
effectively deferring the copy and getting a very recent frame instead of the
old one. Not sure though if the behavior is necessarily defined for this though,
I think it's all a bit vague when it comes to the spec since the video element
does not have a notion of a frame in that regard.

So seems fine to make the change in here to me then. Also to make it consistent
with WebMediaPlayerImpl::copyVideoTextureToPlatformTexture().

https://codereview.chromium.org/200763011/diff/1/content/renderer/media/webme...
File content/renderer/media/webmediaplayer_impl.cc (left):

https://codereview.chromium.org/200763011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:685: // performance will be better
when it is added than not.
Why remove this comment though? Does it not apply anymore?

[junj, 2014-03-20 02:39:33]

Hi, sievers. Thanks for your comments.

https://codereview.chromium.org/200763011/diff/1/content/renderer/media/webme...
File content/renderer/media/webmediaplayer_impl.cc (left):

https://codereview.chromium.org/200763011/diff/1/content/renderer/media/webme...
content/renderer/media/webmediaplayer_impl.cc:685: // performance will be better
when it is added than not.
On 2014/03/19 17:02:19, sievers wrote:
> Why remove this comment though? Does it not apply anymore?

The performance(FPS) seems still better on win7 with flush() available using my
local video4earth test case.

The reason to remove this is to align with that in webmediaplayer_android.cc. As
we had discussed, the flush() might be *necessary here* if we expect the current
video frame to be copied instead of a future one.  So this comment seems
outdated and should be removed.

[junj, 2014-03-20 02:53:07]

The description is changed and I have uploaded a new CL for review. Please help
to take a look. Thanks.

[Ken Russell (switch to Gerrit), 2014-03-20 07:55:11]

I defer to sievers' review.

[no sievers, 2014-03-20 16:24:59]

lgtm

[junj, 2014-03-21 02:29:46]

On 2014/03/20 16:24:59, sievers wrote:
> lgtm

Hi, sievers. Thanks for reviewing the code.

Add acolwell and fischman for owners review. Please help to take a look. Thanks.

[Ami GONE FROM CHROMIUM, 2014-03-21 02:40:00]

LGTM but can this be tested somehow?

[junj, 2014-03-21 03:38:31]

On 2014/03/21 02:40:00, Ami Fischman wrote:
> LGTM but can this be tested somehow?

Hi, Ami. Thanks for helping to review. 

There seems to be some issue with triggering trybots from the web UI or via
local git cl try. I verified the webgl_conformance and several tests locally on
Android and it is OK with this CL. Land it now.

[junj, 2014-03-21 03:38:55]

The CQ bit was checked by jun.a.jiang@chromium.org

[commit-bot: I haz the power, 2014-03-21 03:39:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jun.a.jiang@intel.com/200763011/20001

[commit-bot: I haz the power, 2014-03-21 03:42:01]

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

[commit-bot: I haz the power, 2014-03-21 03:42:01]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[junj, 2014-03-21 04:06:25]

The CQ bit was checked by jun.a.jiang@chromium.org

[commit-bot: I haz the power, 2014-03-21 04:06:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jun.a.jiang@intel.com/200763011/20001

[commit-bot: I haz the power, 2014-03-21 05:04:06]

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

[commit-bot: I haz the power, 2014-03-21 05:04:07]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git,
chrome_elf_unittests, chromedriver_unittests, components_unittests,
compositor_unittests, content_browsertests, content_unittests, crypto_unittests,
events_unittests, gfx_unittests, google_apis_unittests, gpu_unittests,
installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Ami GONE FROM CHROMIUM, 2014-03-21 05:16:33]

I meant can the bug being fixed be tested against to prevent regression?
On Mar 20, 2014 8:38 PM, <jun.a.jiang@chromium.org> wrote:

> On 2014/03/21 02:40:00, Ami Fischman wrote:
>
>> LGTM but can this be tested somehow?
>>
>
> Hi, Ami. Thanks for helping to review.
>
> There seems to be some issue with triggering trybots from the web UI or via
> local git cl try. I verified the webgl_conformance and several tests
> locally on
> Android and it is OK with this CL. Land it now.
>
>
>
> https://1244-f33622853320-dot-chromiumcodereview-hr.appspot.com/200763011/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[junj, 2014-03-21 05:24:55]

On 2014/03/21 05:16:33, Ami Fischman wrote:
> I meant can the bug being fixed be tested against to prevent regression?
> On Mar 20, 2014 8:38 PM, <mailto:jun.a.jiang@chromium.org> wrote:
> 
> > On 2014/03/21 02:40:00, Ami Fischman wrote:
> >
> >> LGTM but can this be tested somehow?
> >>
> >
> > Hi, Ami. Thanks for helping to review.
> >
> > There seems to be some issue with triggering trybots from the web UI or via
> > local git cl try. I verified the webgl_conformance and several tests
> > locally on
> > Android and it is OK with this CL. Land it now.
> >
> >
> >
> > https://1244-f33622853320-dot-chromiumcodereview-hr.appspot.com/200763011/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Hi, Ami. The regression can be fixed by
https://codereview.chromium.org/197213014/. This CL is a complementary patch to
clean up and fix other potential issues.

[junj, 2014-03-21 05:30:22]

On 2014/03/21 05:24:55, junj wrote:
> On 2014/03/21 05:16:33, Ami Fischman wrote:
> > I meant can the bug being fixed be tested against to prevent regression?
> > On Mar 20, 2014 8:38 PM, <mailto:jun.a.jiang@chromium.org> wrote:
> > 
> > > On 2014/03/21 02:40:00, Ami Fischman wrote:
> > >
> > >> LGTM but can this be tested somehow?
> > >>
> > >
> > > Hi, Ami. Thanks for helping to review.
> > >
> > > There seems to be some issue with triggering trybots from the web UI or
via
> > > local git cl try. I verified the webgl_conformance and several tests
> > > locally on
> > > Android and it is OK with this CL. Land it now.
> > >
> > >
> > >
> > > https://1244-f33622853320-dot-chromiumcodereview-hr.appspot.com/200763011/
> > >
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> Hi, Ami. The regression can be fixed by
> https://codereview.chromium.org/197213014/. This CL is a complementary patch
to
> clean up and fix other potential issues.

And for test cases to track this kinds of regression, there is a bug opened at
http://code.google.com/p/chromium/issues/detail?id=354341.

[junj, 2014-03-21 07:01:10]

The CQ bit was checked by jun.a.jiang@chromium.org

[commit-bot: I haz the power, 2014-03-21 07:01:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jun.a.jiang@intel.com/200763011/20001

[commit-bot: I haz the power, 2014-03-21 09:06:11]

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

[commit-bot: I haz the power, 2014-03-21 09:06:11]

Retried try job too often on win_rel for step(s) net_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[junj, 2014-03-21 09:08:41]

The CQ bit was checked by jun.a.jiang@chromium.org

[commit-bot: I haz the power, 2014-03-21 09:09:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jun.a.jiang@intel.com/200763011/20001

[commit-bot: I haz the power, 2014-03-21 13:57:42]

Change committed as 258561