Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2763223003:
Reland of cc: Don't use StreamVideoDrawQuad on any platform but Android. (Closed)
|
Created:
3 years, 9 months ago by Daniele Castagna Modified:
3 years, 6 months ago CC:
cc-bugs_chromium.org, chromium-reviews, inactive_dshwang_plz_cc_intel, posciak+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptioncc: Don't use StreamVideoDrawQuad on any platform but Android.
VideoLayerImpl is appending StreamVideoDrawQuads on CrOS.
StreamVideoDrawQuads should be used on Android since almost all its
features are supported by TextureDrawQuad.
The only difference is that StreamVideoDrawQuad calls
glUniformMatrix4fvStreamTextureMatrixCHROMIUM before drawing, that
just sets a uniform matrix on CrOS.
This CL makes sure VideoLayerImpl produces StreamVideoDrawQuads only
on Android.
BUG=702750
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2763223003
Cr-Original-Commit-Position: refs/heads/master@{#460547}
Committed: https://chromium.googlesource.com/chromium/src/+/517d0115d3c435383f79a0b730b6514a6cc97f44
Review-Url: https://codereview.chromium.org/2763223003
Cr-Commit-Position: refs/heads/master@{#481624}
Committed: https://chromium.googlesource.com/chromium/src/+/a7cd47aa097a96ee6c54d6af087f57767b3a401e
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address Dana's comments. Fix unittest. #
Total comments: 4
Patch Set 3 : Address danakj comments. Add unittest. #Patch Set 4 : Use premultiplied for RGBA. #Patch Set 5 : Fix tests. #
Messages
Total messages: 51 (31 generated)
Description was changed from ========== cc: Don't use StreamVideoDrawQuad on any platform but Android BUG=702750 ========== to ========== cc: Don't use StreamVideoDrawQuad on any platform but Android BUG=702750 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by dcastagna@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== cc: Don't use StreamVideoDrawQuad on any platform but Android BUG=702750 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Don't use StreamVideoDrawQuad on any platform but Android. It seems like VideoLayerImpl is appending StreamVideoDrawQuads on CrOS. IIUC this drawquad should be used on Android since almost all its features are supported by TextureDrawQuad. The only difference is that StreamVideoDrawQuad calls glUniformMatrix4fvStreamTextureMatrixCHROMIUM before drawing, that just sets a uniform matrix on CrOS. This CL makes sure we're VideoLayerImpl produces StreamVideoDrawQuads only on Android. BUG=702750 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
dcastagna@chromium.org changed reviewers: + reveman@chromium.org
Description was changed from ========== cc: Don't use StreamVideoDrawQuad on any platform but Android. It seems like VideoLayerImpl is appending StreamVideoDrawQuads on CrOS. IIUC this drawquad should be used on Android since almost all its features are supported by TextureDrawQuad. The only difference is that StreamVideoDrawQuad calls glUniformMatrix4fvStreamTextureMatrixCHROMIUM before drawing, that just sets a uniform matrix on CrOS. This CL makes sure we're VideoLayerImpl produces StreamVideoDrawQuads only on Android. BUG=702750 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Don't use StreamVideoDrawQuad on any platform but Android. It seems like VideoLayerImpl is appending StreamVideoDrawQuads on CrOS. IIUC this drawquad should be used on Android since almost all its features are supported by TextureDrawQuad. The only difference is that StreamVideoDrawQuad calls glUniformMatrix4fvStreamTextureMatrixCHROMIUM before drawing, that just sets a uniform matrix on CrOS. This CL makes sure we're VideoLayerImpl produces StreamVideoDrawQuads only on Android. BUG=702750 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
lgtm as it's really confusing that we use this code on cros
Dongseong, FYI: it seems like we might have been using the wrong DrawQuad for videos on CrOS...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
danakj@chromium.org changed reviewers: + danakj@chromium.org
https://codereview.chromium.org/2763223003/diff/1/cc/resources/video_resource... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2763223003/diff/1/cc/resources/video_resource... cc/resources/video_resource_updater.cc:48: #if defined(OS_ANDROID) Please don't write OS defines inside cc. They make unit tests become platform specific. If you need a platform setting use LayerTreeSettings.
https://codereview.chromium.org/2763223003/diff/1/cc/resources/video_resource... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2763223003/diff/1/cc/resources/video_resource... cc/resources/video_resource_updater.cc:48: #if defined(OS_ANDROID) On 2017/03/22 14:44:23, danakj wrote: > Please don't write OS defines inside cc. They make unit tests become platform > specific. If you need a platform setting use LayerTreeSettings. Oh, this isn't in the layer tree really. Maybe here a constructor parameter to the VideoResourceUpdater can tell it if stream resources are to be used or not. Then unit tests can set that or not and verify behaviour.
Description was changed from ========== cc: Don't use StreamVideoDrawQuad on any platform but Android. It seems like VideoLayerImpl is appending StreamVideoDrawQuads on CrOS. IIUC this drawquad should be used on Android since almost all its features are supported by TextureDrawQuad. The only difference is that StreamVideoDrawQuad calls glUniformMatrix4fvStreamTextureMatrixCHROMIUM before drawing, that just sets a uniform matrix on CrOS. This CL makes sure we're VideoLayerImpl produces StreamVideoDrawQuads only on Android. BUG=702750 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Don't use StreamVideoDrawQuad on any platform but Android. It seems like VideoLayerImpl is appending StreamVideoDrawQuads on CrOS. IIUC this drawquad should be used on Android since almost all its features are supported by TextureDrawQuad. The only difference is that StreamVideoDrawQuad calls glUniformMatrix4fvStreamTextureMatrixCHROMIUM before drawing, that just sets a uniform matrix on CrOS. This CL makes sure VideoLayerImpl produces StreamVideoDrawQuads only on Android. BUG=702750 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
The CQ bit was checked by dcastagna@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/03/22 at 14:47:04, danakj wrote: > https://codereview.chromium.org/2763223003/diff/1/cc/resources/video_resource... > File cc/resources/video_resource_updater.cc (right): > > https://codereview.chromium.org/2763223003/diff/1/cc/resources/video_resource... > cc/resources/video_resource_updater.cc:48: #if defined(OS_ANDROID) > On 2017/03/22 14:44:23, danakj wrote: > > Please don't write OS defines inside cc. They make unit tests become platform > > specific. If you need a platform setting use LayerTreeSettings. > > Oh, this isn't in the layer tree really. Maybe here a constructor parameter to the VideoResourceUpdater can tell it if stream resources are to be used or not. Then unit tests can set that or not and verify behaviour. We can still pass the option via LayerTreeSettings, in this way we can have the #ifdef code in render_widget_compositor.cc, with the rest of the ifdefs. Can you also please confirm that using a StreamVideoDrawQuad on non android platforms was unintended to begin with?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On Thu, Mar 23, 2017 at 10:25 PM, <dcastagna@chromium.org> wrote: > On 2017/03/22 at 14:47:04, danakj wrote: > > > https://codereview.chromium.org/2763223003/diff/1/cc/ > resources/video_resource_updater.cc > > File cc/resources/video_resource_updater.cc (right): > > > > > https://codereview.chromium.org/2763223003/diff/1/cc/ > resources/video_resource_updater.cc#newcode48 > > cc/resources/video_resource_updater.cc:48: #if defined(OS_ANDROID) > > On 2017/03/22 14:44:23, danakj wrote: > > > Please don't write OS defines inside cc. They make unit tests become > platform > > > specific. If you need a platform setting use LayerTreeSettings. > > > > Oh, this isn't in the layer tree really. Maybe here a constructor > parameter to > the VideoResourceUpdater can tell it if stream resources are to be used or > not. > Then unit tests can set that or not and verify behaviour. > > We can still pass the option via LayerTreeSettings, in this way we can > have the > #ifdef code in render_widget_compositor.cc, with the rest of the ifdefs. > Can you also please confirm that using a StreamVideoDrawQuad on non android > platforms was unintended to begin with? > I would think so, cuz it was added for android specific things originally (actually for having a single texture that gets magically updated by the media code without going through the compositor at all). But it's hard to follow the whole evolution of the media stack. I'm not sure if android still has any reason for StreamVideoDrawQuad anymore tbh. > > > > https://codereview.chromium.org/2763223003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Description was changed from ========== cc: Don't use StreamVideoDrawQuad on any platform but Android. It seems like VideoLayerImpl is appending StreamVideoDrawQuads on CrOS. IIUC this drawquad should be used on Android since almost all its features are supported by TextureDrawQuad. The only difference is that StreamVideoDrawQuad calls glUniformMatrix4fvStreamTextureMatrixCHROMIUM before drawing, that just sets a uniform matrix on CrOS. This CL makes sure VideoLayerImpl produces StreamVideoDrawQuads only on Android. BUG=702750 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Don't use StreamVideoDrawQuad on any platform but Android. VideoLayerImpl is appending StreamVideoDrawQuads on CrOS. StreamVideoDrawQuads should be used on Android since almost all its features are supported by TextureDrawQuad. The only difference is that StreamVideoDrawQuad calls glUniformMatrix4fvStreamTextureMatrixCHROMIUM before drawing, that just sets a uniform matrix on CrOS. This CL makes sure VideoLayerImpl produces StreamVideoDrawQuads only on Android. BUG=702750 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
On 2017/03/24 at 14:51:34, danakj wrote: > On Thu, Mar 23, 2017 at 10:25 PM, <dcastagna@chromium.org> wrote: > > > On 2017/03/22 at 14:47:04, danakj wrote: > > > > > https://codereview.chromium.org/2763223003/diff/1/cc/ > > resources/video_resource_updater.cc > > > File cc/resources/video_resource_updater.cc (right): > > > > > > > > https://codereview.chromium.org/2763223003/diff/1/cc/ > > resources/video_resource_updater.cc#newcode48 > > > cc/resources/video_resource_updater.cc:48: #if defined(OS_ANDROID) > > > On 2017/03/22 14:44:23, danakj wrote: > > > > Please don't write OS defines inside cc. They make unit tests become > > platform > > > > specific. If you need a platform setting use LayerTreeSettings. > > > > > > Oh, this isn't in the layer tree really. Maybe here a constructor > > parameter to > > the VideoResourceUpdater can tell it if stream resources are to be used or > > not. > > Then unit tests can set that or not and verify behaviour. > > > > We can still pass the option via LayerTreeSettings, in this way we can > > have the > > #ifdef code in render_widget_compositor.cc, with the rest of the ifdefs. > > Can you also please confirm that using a StreamVideoDrawQuad on non android > > platforms was unintended to begin with? > > > > I would think so, cuz it was added for android specific things originally > (actually for having a single texture that gets magically updated by the > media code without going through the compositor at all). But it's hard to > follow the whole evolution of the media stack. I'm not sure if android > still has any reason for StreamVideoDrawQuad anymore tbh. > Ok, thanks for the explanation. Can you take another look at this CL now that I addressed you comments? I'd like to land this so that we can promote videos to overlays using the same logic we use for TextureDrawQuads. > > > > > > > > > https://codereview.chromium.org/2763223003/ > > > > -- > You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. >
LGTM https://codereview.chromium.org/2763223003/diff/20001/cc/resources/video_reso... File cc/resources/video_resource_updater.h (right): https://codereview.chromium.org/2763223003/diff/20001/cc/resources/video_reso... cc/resources/video_resource_updater.h:78: bool use_stream_video_draw_quad = false); i would prefer not to use a default value here, it doesn't seem like a strong case for one https://codereview.chromium.org/2763223003/diff/20001/cc/resources/video_reso... File cc/resources/video_resource_updater_unittest.cc (right): https://codereview.chromium.org/2763223003/diff/20001/cc/resources/video_reso... cc/resources/video_resource_updater_unittest.cc:486: resource_provider3d_.get(), true); nit: give the true a name either with a temp bool var or with a comment like true /* name */
The CQ bit was checked by dcastagna@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dcastagna@chromium.org changed reviewers: + avi@chromium.org
The media team confirmed StreamVideoDrawQuad is still needed on Android. +avi for ownership on content/* https://codereview.chromium.org/2763223003/diff/20001/cc/resources/video_reso... File cc/resources/video_resource_updater.h (right): https://codereview.chromium.org/2763223003/diff/20001/cc/resources/video_reso... cc/resources/video_resource_updater.h:78: bool use_stream_video_draw_quad = false); On 2017/03/28 at 15:44:13, danakj wrote: > i would prefer not to use a default value here, it doesn't seem like a strong case for one Done. https://codereview.chromium.org/2763223003/diff/20001/cc/resources/video_reso... File cc/resources/video_resource_updater_unittest.cc (right): https://codereview.chromium.org/2763223003/diff/20001/cc/resources/video_reso... cc/resources/video_resource_updater_unittest.cc:486: resource_provider3d_.get(), true); On 2017/03/28 at 15:44:13, danakj wrote: > nit: give the true a name either with a temp bool var or with a comment like true /* name */ Done.
lgtm stampity stamp
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dcastagna@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2763223003/#ps40001 (title: "Address danakj comments. Add unittest.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the test!
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490824777589140,
"parent_rev": "c5f0349b6e01219686c8d0e76884553125f1aae9", "commit_rev":
"517d0115d3c435383f79a0b730b6514a6cc97f44"}
Message was sent while issue was closed.
Description was changed from ========== cc: Don't use StreamVideoDrawQuad on any platform but Android. VideoLayerImpl is appending StreamVideoDrawQuads on CrOS. StreamVideoDrawQuads should be used on Android since almost all its features are supported by TextureDrawQuad. The only difference is that StreamVideoDrawQuad calls glUniformMatrix4fvStreamTextureMatrixCHROMIUM before drawing, that just sets a uniform matrix on CrOS. This CL makes sure VideoLayerImpl produces StreamVideoDrawQuads only on Android. BUG=702750 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Don't use StreamVideoDrawQuad on any platform but Android. VideoLayerImpl is appending StreamVideoDrawQuads on CrOS. StreamVideoDrawQuads should be used on Android since almost all its features are supported by TextureDrawQuad. The only difference is that StreamVideoDrawQuad calls glUniformMatrix4fvStreamTextureMatrixCHROMIUM before drawing, that just sets a uniform matrix on CrOS. This CL makes sure VideoLayerImpl produces StreamVideoDrawQuads only on Android. BUG=702750 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2763223003 Cr-Commit-Position: refs/heads/master@{#460547} Committed: https://chromium.googlesource.com/chromium/src/+/517d0115d3c435383f79a0b730b6... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/517d0115d3c435383f79a0b730b6...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2920893003/ by posciak@chromium.org. The reason for reverting is: crbug.com/724812.
On 2017/06/02 at 00:05:09, posciak wrote: > A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2920893003/ by posciak@chromium.org. > > The reason for reverting is: crbug.com/724812. crbug.com/ was caused by incorrectly returning RGB_RESOURCE instead of RGBA_PREMULTIPLIED_RESOURCE for RGBA in ResourceTypeForVideoFrame. Re-landing this patch.
The CQ bit was checked by dcastagna@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, avi@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2763223003/#ps60001 (title: "Use premultiplied for RGBA.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by dcastagna@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, avi@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/2763223003/#ps80001 (title: "Fix tests.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498154648502250,
"parent_rev": "c8b216bbd914a0e91b66d13b2030735c2ee2d964", "commit_rev":
"a7cd47aa097a96ee6c54d6af087f57767b3a401e"}
Message was sent while issue was closed.
Description was changed from ========== cc: Don't use StreamVideoDrawQuad on any platform but Android. VideoLayerImpl is appending StreamVideoDrawQuads on CrOS. StreamVideoDrawQuads should be used on Android since almost all its features are supported by TextureDrawQuad. The only difference is that StreamVideoDrawQuad calls glUniformMatrix4fvStreamTextureMatrixCHROMIUM before drawing, that just sets a uniform matrix on CrOS. This CL makes sure VideoLayerImpl produces StreamVideoDrawQuads only on Android. BUG=702750 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2763223003 Cr-Commit-Position: refs/heads/master@{#460547} Committed: https://chromium.googlesource.com/chromium/src/+/517d0115d3c435383f79a0b730b6... ========== to ========== cc: Don't use StreamVideoDrawQuad on any platform but Android. VideoLayerImpl is appending StreamVideoDrawQuads on CrOS. StreamVideoDrawQuads should be used on Android since almost all its features are supported by TextureDrawQuad. The only difference is that StreamVideoDrawQuad calls glUniformMatrix4fvStreamTextureMatrixCHROMIUM before drawing, that just sets a uniform matrix on CrOS. This CL makes sure VideoLayerImpl produces StreamVideoDrawQuads only on Android. BUG=702750 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2763223003 Cr-Original-Commit-Position: refs/heads/master@{#460547} Committed: https://chromium.googlesource.com/chromium/src/+/517d0115d3c435383f79a0b730b6... Review-Url: https://codereview.chromium.org/2763223003 Cr-Commit-Position: refs/heads/master@{#481624} Committed: https://chromium.googlesource.com/chromium/src/+/a7cd47aa097a96ee6c54d6af087f... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/a7cd47aa097a96ee6c54d6af087f... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
