Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2242453002:
Avoid planar YUV resources when one component EGL images are not supported (Closed)
|
Created:
4 years, 4 months ago by Tobias Sargeant Modified:
4 years, 3 months ago CC:
chromium-reviews, posciak+watch_chromium.org, piman+watch_chromium.org, cc-bugs_chromium.org, timav Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAvoid planar YUV resources when one component EGL images are not supported
Some EGL drivers do not support binding one component textures to an
EGL image. One component textures are used for YUV video frames, so
to support those drivers in WebView, we need to do YUV to RGB
conversion in software, and then upload an RGB texture instead of a
set of 3 Y,U,V textures.
BUG=579060, 632461
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/410f515e005fad3917df3d44c2b35d75e9100f44
Cr-Commit-Position: refs/heads/master@{#418819}
Patch Set 1 #
Total comments: 6
Patch Set 2 : switch to checked versions of calculations #Patch Set 3 : modify comment to explain the use of RGBA_8888 to trigger a driver workaround #
Total comments: 15
Patch Set 4 : Address comments on PS3; attempt to reuse more of CreateForSoftwarePlanes. #Patch Set 5 : rebase #
Total comments: 11
Patch Set 6 : PS5 comments #Patch Set 7 : Fix cc_unittests #
Total comments: 10
Patch Set 8 : PS7 comments; remove the spitzer-disabling workaround. #Patch Set 9 : Add SyncToken comment #
Total comments: 4
Patch Set 10 : Abstract out resource immutability. #Messages
Total messages: 71 (41 generated)
Description was changed from ========== Avoid planar YUV resources when one component EGL images are not supported Some EGL drivers do not support binding one component textures to an EGL image. One component textures are used for YUV video frames, so to support those drivers in WebView, we need to do YUV to RGB conversion in software, and then upload an RGB texture instead of a set of 3 Y,U,V textures. BUG=579060, 632461 ========== to ========== Avoid planar YUV resources when one component EGL images are not supported Some EGL drivers do not support binding one component textures to an EGL image. One component textures are used for YUV video frames, so to support those drivers in WebView, we need to do YUV to RGB conversion in software, and then upload an RGB texture instead of a set of 3 Y,U,V textures. BUG=579060, 632461 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Avoid planar YUV resources when one component EGL images are not supported Some EGL drivers do not support binding one component textures to an EGL image. One component textures are used for YUV video frames, so to support those drivers in WebView, we need to do YUV to RGB conversion in software, and then upload an RGB texture instead of a set of 3 Y,U,V textures. BUG=579060, 632461 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Avoid planar YUV resources when one component EGL images are not supported Some EGL drivers do not support binding one component textures to an EGL image. One component textures are used for YUV video frames, so to support those drivers in WebView, we need to do YUV to RGB conversion in software, and then upload an RGB texture instead of a set of 3 Y,U,V textures. BUG=579060, 632461 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
The CQ bit was checked by tobiasjs@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Avoid planar YUV resources when one component EGL images are not supported Some EGL drivers do not support binding one component textures to an EGL image. One component textures are used for YUV video frames, so to support those drivers in WebView, we need to do YUV to RGB conversion in software, and then upload an RGB texture instead of a set of 3 Y,U,V textures. BUG=579060, 632461 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Avoid planar YUV resources when one component EGL images are not supported Some EGL drivers do not support binding one component textures to an EGL image. One component textures are used for YUV video frames, so to support those drivers in WebView, we need to do YUV to RGB conversion in software, and then upload an RGB texture instead of a set of 3 Y,U,V textures. BUG=579060, 632461 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
tobiasjs@chromium.org changed reviewers: + danakj@chromium.org, dcheng@chromium.org, liberato@chromium.org, sievers@chromium.org
Hi, Resurrecting this from https://codereview.chromium.org/1761893003/ - where it languished for long enough that it seemed best to start with a clean slate. Reason for reviving is that spitzer apparently also uses YUV resources and so this has gone from affecting only WebRTC on a small fraction of devices to affecting video too & it seems better to do this than to disable spitzer on those devices. If you could take a look, that'd be great, thanks.
lgtm, though i'm not super familiar with this part of the resource updater. https://codereview.chromium.org/2242453002/diff/1/cc/resources/video_resource... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2242453002/diff/1/cc/resources/video_resource... cc/resources/video_resource_updater.cc:367: if (!software_compositor && is it possible to avoid switching the resource format to 8888 in the first place when using the software compositor?
https://codereview.chromium.org/2242453002/diff/1/cc/resources/video_resource... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2242453002/diff/1/cc/resources/video_resource... cc/resources/video_resource_updater.cc:288: MathUtil::UncheckedRoundUp<size_t>(bytes_per_row, 4u); Sorry, I think I've asked this before, but can these calculations overflow? Does it matter?
On 2016/08/12 22:50:47, dcheng wrote: > https://codereview.chromium.org/2242453002/diff/1/cc/resources/video_resource... > File cc/resources/video_resource_updater.cc (right): > > https://codereview.chromium.org/2242453002/diff/1/cc/resources/video_resource... > cc/resources/video_resource_updater.cc:288: > MathUtil::UncheckedRoundUp<size_t>(bytes_per_row, 4u); > Sorry, I think I've asked this before, but can these calculations overflow? Does > it matter? You did, and there was a discussion on the other CL. The UncheckedRoundUp matches other code in the same file that's doing the same thing. Revisiting it, I think this is safe because if the x dimension was sized such that this overflowed then the memory allocation for the data held by video_frame would have already failed.
https://codereview.chromium.org/2242453002/diff/1/cc/resources/video_resource... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2242453002/diff/1/cc/resources/video_resource... cc/resources/video_resource_updater.cc:288: MathUtil::UncheckedRoundUp<size_t>(bytes_per_row, 4u); On 2016/08/12 22:50:47, dcheng wrote: > Sorry, I think I've asked this before, but can these calculations overflow? Does > it matter? Ack. Commented in reply. https://codereview.chromium.org/2242453002/diff/1/cc/resources/video_resource... cc/resources/video_resource_updater.cc:367: if (!software_compositor && On 2016/08/12 22:09:23, liberato wrote: > is it possible to avoid switching the resource format to 8888 in the first place > when using the software compositor? It will only be set to 8888 for GPU resources, so in a sense the !software_compositor is redundant. I think I'd like to keep here though, for readability, unless you think otherwise?
On 2016/08/15 09:25:59, Tobias Sargeant wrote: > On 2016/08/12 22:50:47, dcheng wrote: > > > https://codereview.chromium.org/2242453002/diff/1/cc/resources/video_resource... > > File cc/resources/video_resource_updater.cc (right): > > > > > https://codereview.chromium.org/2242453002/diff/1/cc/resources/video_resource... > > cc/resources/video_resource_updater.cc:288: > > MathUtil::UncheckedRoundUp<size_t>(bytes_per_row, 4u); > > Sorry, I think I've asked this before, but can these calculations overflow? > Does > > it matter? > > You did, and there was a discussion on the other CL. The UncheckedRoundUp > matches other code in the same file that's doing the same thing. Revisiting it, > I think this is safe because if the x dimension was sized such that this > overflowed then the memory allocation for the data held by video_frame would > have already failed. Oh oops. I got confused about the replies. I tried tracing this back a bit but got lost by VideoFrameProvider::GetCurrentFrame(). The question I mistakenly asked on the other CL is relevant here still though: as a reviewer, how do I verify this?
On 2016/08/15 18:32:58, dcheng wrote: > Oh oops. I got confused about the replies. I tried tracing this back a bit but > got lost by VideoFrameProvider::GetCurrentFrame(). The question I mistakenly > asked on the other CL is relevant here still though: as a reviewer, how do I > verify this? Other than my hand-waving argument that VideoFrame points at allocated memory that would not have been able to be allocated in any case where a size_t overflow could occur, I can't find a good answer either. On that basis, I've switched my use and the other use of the Unchecked* functions to Checked*.
https://codereview.chromium.org/2242453002/diff/1/cc/resources/video_resource... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2242453002/diff/1/cc/resources/video_resource... cc/resources/video_resource_updater.cc:367: if (!software_compositor && On 2016/08/15 09:33:04, Tobias Sargeant wrote: > On 2016/08/12 22:09:23, liberato wrote: > > is it possible to avoid switching the resource format to 8888 in the first > place > > when using the software compositor? > > It will only be set to 8888 for GPU resources, so in a sense the > !software_compositor is redundant. I think I'd like to keep here though, for > readability, unless you think otherwise? makes sense. probably deserves a comment to that effect. also, might be more appropriate as a DCHECK. i don't understand the semantics of a mismatch between |software_compositor| vs |output_resource_format| well enough to say which is better.
https://codereview.chromium.org/2242453002/diff/1/cc/resources/video_resource... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2242453002/diff/1/cc/resources/video_resource... cc/resources/video_resource_updater.cc:367: if (!software_compositor && On 2016/08/16 14:55:46, liberato wrote: > On 2016/08/15 09:33:04, Tobias Sargeant wrote: > > On 2016/08/12 22:09:23, liberato wrote: > > > is it possible to avoid switching the resource format to 8888 in the first > > place > > > when using the software compositor? > > > > It will only be set to 8888 for GPU resources, so in a sense the > > !software_compositor is redundant. I think I'd like to keep here though, for > > readability, unless you think otherwise? > > makes sense. probably deserves a comment to that effect. > > also, might be more appropriate as a DCHECK. i don't understand the semantics > of a mismatch between |software_compositor| vs |output_resource_format| well > enough to say which is better. I updated the comment to make the intent clearer.
ipc lgtm to be clear, i'm ok with the unchecked variants, but maybe we need to make the untrusted / trusted boundary a bit clearer in this code to make that viable.
On 2016/08/16 20:58:55, dcheng wrote: > ipc lgtm > > to be clear, i'm ok with the unchecked variants, but maybe we need to make the > untrusted / trusted boundary a bit clearer in this code to make that viable. (the unchecked variants in principle, not necessarily in this particular bit of code)
Hi Dana and Daniel, When you have a moment, could you PTAL? Thanks.
just some nits for gpu/ https://codereview.chromium.org/2242453002/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2242453002/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:3596: caps.force_software_yuv_conversion = nit: Instead of 'force_software_yuv_conversion' maybe we can come up with a better name that doesn't imply a prescription with knowledge about the client pipeline, but rather is a simple feature (or feature broken) flag that says something about the driver capabilities. How about saying that 'shared (as in across contexts with a mailbox) one component textures' cannot be used? https://codereview.chromium.org/2242453002/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:3597: mailbox_manager()->UsesSync() && nit: can you put a comment to explain why we only need to do it for 'UsesSync()'? https://codereview.chromium.org/2242453002/diff/40001/gpu/config/gpu_driver_b... File gpu/config/gpu_driver_bug_workaround_type.h (right): https://codereview.chromium.org/2242453002/diff/40001/gpu/config/gpu_driver_b... gpu/config/gpu_driver_bug_workaround_type.h:23: avoid_one_component_egl_images) \ nit: before BROKEN_...
I'd like if this was reusing the existing path a bit more, it looks like it's basically the software compositing path but with a context so it needs to upload? https://codereview.chromium.org/2242453002/diff/40001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/2242453002/diff/40001/cc/resources/resource_p... cc/resources/resource_provider.cc:448: yuv_resource_format_ = caps.texture_rg ? RED_8 : LUMINANCE_8; Theres now 3 places in a row that set yuv_highbit_resource_format_, and 2 that set yuv_resource_format_. Can you adjust this code with if/elseif/elses so that each of these is set a single time? https://codereview.chromium.org/2242453002/diff/40001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2242453002/diff/40001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:292: MathUtil::CheckedRoundUp<size_t>(bytes_per_row, 4u); If the format is RGBA8888 how can the bytes per row not be a multiple of 4? https://codereview.chromium.org/2242453002/diff/40001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:391: // Drop recycled resources that are the wrong format. You're skipping code like this. I would have expected you to be able to use this function more, just override the resource format, plane count. https://codereview.chromium.org/2242453002/diff/40001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:471: video_renderer_.reset(new media::SkCanvasVideoRenderer); Why are you using a diff skia thing to convert yuv to rgb than what we have here? Can you maybe share this code? The above if could be if (software_compositor || convert_to_rgba) { instead or something? https://codereview.chromium.org/2242453002/diff/40001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:478: video_renderer_->Copy(video_frame, &canvas, media::Context3D()); Then you could pass a context here when !software_compositor and not need to do the upload yourself maybe? I'm not actually sure if that makes sense. https://codereview.chromium.org/2242453002/diff/40001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:479: plane_resource.SetUniqueId(video_frame->unique_id(), 0); And then you don't miss important stuff like this.
https://codereview.chromium.org/2242453002/diff/40001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/2242453002/diff/40001/cc/resources/resource_p... cc/resources/resource_provider.cc:448: yuv_resource_format_ = caps.texture_rg ? RED_8 : LUMINANCE_8; On 2016/08/25 01:25:39, danakj wrote: > Theres now 3 places in a row that set yuv_highbit_resource_format_, and 2 that > set yuv_resource_format_. > > Can you adjust this code with if/elseif/elses so that each of these is set a > single time? Done. https://codereview.chromium.org/2242453002/diff/40001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2242453002/diff/40001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:292: MathUtil::CheckedRoundUp<size_t>(bytes_per_row, 4u); On 2016/08/25 01:25:39, danakj wrote: > If the format is RGBA8888 how can the bytes per row not be a multiple of 4? True, removed. https://codereview.chromium.org/2242453002/diff/40001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:471: video_renderer_.reset(new media::SkCanvasVideoRenderer); On 2016/08/25 01:25:39, danakj wrote: > Why are you using a diff skia thing to convert yuv to rgb than what we have > here? Can you maybe share this code? The above if could be if > (software_compositor || convert_to_rgba) { instead or something? I've reworked this to keep more of the original code flow. I didn't manage to merge the two different skia YUV conversions though. That might be possible, but I thought I'd check that you were happier with this iteration first. It may be that the software path is faster going through the static method; the Copy() method uses Paint() which handles all kinds of transformations that are not needed in this 1:1 pixel mapping case. https://codereview.chromium.org/2242453002/diff/40001/gpu/command_buffer/serv... File gpu/command_buffer/service/gles2_cmd_decoder.cc (right): https://codereview.chromium.org/2242453002/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:3596: caps.force_software_yuv_conversion = On 2016/08/24 22:39:18, sievers wrote: > nit: Instead of 'force_software_yuv_conversion' maybe we can come up with a > better name that doesn't imply a prescription with knowledge about the client > pipeline, but rather is a simple feature (or feature broken) flag that says > something about the driver capabilities. > > How about saying that 'shared (as in across contexts with a mailbox) one > component textures' cannot be used? Done. https://codereview.chromium.org/2242453002/diff/40001/gpu/command_buffer/serv... gpu/command_buffer/service/gles2_cmd_decoder.cc:3597: mailbox_manager()->UsesSync() && On 2016/08/24 22:39:18, sievers wrote: > nit: can you put a comment to explain why we only need to do it for > 'UsesSync()'? Done. https://codereview.chromium.org/2242453002/diff/40001/gpu/config/gpu_driver_b... File gpu/config/gpu_driver_bug_workaround_type.h (right): https://codereview.chromium.org/2242453002/diff/40001/gpu/config/gpu_driver_b... gpu/config/gpu_driver_bug_workaround_type.h:23: avoid_one_component_egl_images) \ On 2016/08/24 22:39:18, sievers wrote: > nit: before BROKEN_... Done.
gpu lgtm
Thanks, looks better. Few comments https://codereview.chromium.org/2242453002/diff/80001/cc/resources/resource_p... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/2242453002/diff/80001/cc/resources/resource_p... cc/resources/resource_provider.cc:449: if (caps.disable_one_component_textures) { Thanks this is nicer https://codereview.chromium.org/2242453002/diff/80001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2242453002/diff/80001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:204: (resource_provider_->GetTextureHint(it->resource_id()) == doesn't this need to match the immutable hint? In the callsite where the hint is false, it expects the resource to not be immutable currently right? https://codereview.chromium.org/2242453002/diff/80001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:408: DCHECK(software_compositor == plane_resource.mailbox().IsZero()); DCHECK_EQ https://codereview.chromium.org/2242453002/diff/80001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:444: TextureMailbox mailbox(plane_resource.mailbox(), gpu::SyncToken(), why no SyncToken? https://codereview.chromium.org/2242453002/diff/80001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:615: RecycleOrAllocateResource(output_plane_resource_size, copy_target_format, Hm, but if it uses a resource with references on it because Matches() is true, then the whole point is that it does not need to modify the resource which is why it can return it. This code would get a resource with references on it still, and write to it.
The CQ bit was checked by tobiasjs@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...
https://codereview.chromium.org/2242453002/diff/80001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2242453002/diff/80001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:204: (resource_provider_->GetTextureHint(it->resource_id()) == On 2016/08/26 18:24:23, danakj wrote: > doesn't this need to match the immutable hint? In the callsite where the hint is > false, it expects the resource to not be immutable currently right? This should only return resources if they have similar immutability to what would otherwise be allocated. It seems like it would be ok to return a mutable texture if an immutable texture is asked for, although I presume that immutability is a performance optimization. https://codereview.chromium.org/2242453002/diff/80001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:408: DCHECK(software_compositor == plane_resource.mailbox().IsZero()); On 2016/08/26 18:24:23, danakj wrote: > DCHECK_EQ Done. https://codereview.chromium.org/2242453002/diff/80001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:444: TextureMailbox mailbox(plane_resource.mailbox(), gpu::SyncToken(), On 2016/08/26 18:24:23, danakj wrote: > why no SyncToken? This is identical to the mailbox construction on l567. Is there a reason why they should be different? https://codereview.chromium.org/2242453002/diff/80001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:615: RecycleOrAllocateResource(output_plane_resource_size, copy_target_format, On 2016/08/26 18:24:23, danakj wrote: > Hm, but if it uses a resource with references on it because Matches() is true, > then the whole point is that it does not need to modify the resource which is > why it can return it. This code would get a resource with references on it > still, and write to it. Passing -1 for the unique_id and plane_number prevents ref'ed resources from ever matching. I will make that more explicit.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by tobiasjs@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for your replies, I think this makes sense. I have a few more comments about helping to make this more clear, and maybe have less branching. https://codereview.chromium.org/2242453002/diff/80001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2242453002/diff/80001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:444: TextureMailbox mailbox(plane_resource.mailbox(), gpu::SyncToken(), On 2016/08/30 11:31:05, Tobias Sargeant wrote: > On 2016/08/26 18:24:23, danakj wrote: > > why no SyncToken? > > This is identical to the mailbox construction on l567. Is there a reason why > they should be different? Oh, I guess this class is relying on it sharing the context provider with the compositor, okay. Would you mind saying this in a comment (at both places maybe?), because I forgot even. https://codereview.chromium.org/2242453002/diff/120001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2242453002/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater.cc:184: if (unique_id >= 0 && plane_index >= 0) { i dont think we should bake in assumptions about the namespace of unique_id in this file, VideoFrames could come with negative ids. But branching on plane_index only would be okay. That matches your comment below also. Then maybe use 0 for the unique_id and -1 for the plane_index? (Or just add another bool for "allow_return_with_references" or something, up to you) https://codereview.chromium.org/2242453002/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater.cc:194: for (auto it = all_resources_.begin(); it != all_resources_.end(); ++it) { I kinda feel like it was a bit more clear when this was a single loop. The first check was if Matches, which could be guarded by do we want to allow. Then the 2nd check inside the loop was for if there's no refs. Now the !has_refs is very hidden. I'd prefer to combine these two walks (its more efficient too), or please provide comments on each loop explaining why we have two and what they each hope to provide. Also we lost the bingo comment about "its safe to use this even if there are references" which I think had value. Why'd it go away? https://codereview.chromium.org/2242453002/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater.cc:205: if (immutable_hint == Can you make this an else if () { instead of else { if () {? https://codereview.chromium.org/2242453002/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater.cc:230: if (resource_id == 0) Maybe it used to? But CreateResource() can't return 0: https://cs.chromium.org/chromium/src/cc/resources/resource_provider.cc?rcl=0&... So this if check doesn't need to exist. And that means this function can't return end(). Which means CopyPlaneTexture can't return false, and doesn't need to be a bool, etc. https://codereview.chromium.org/2242453002/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater.cc:621: -1, can you use temp vars to give these litarals names (similar to is_immutable), and you can put the comment on them there then.
The CQ bit was checked by tobiasjs@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/...)
The CQ bit was checked by tobiasjs@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #8 (id:140001) has been deleted
Patchset #8 (id:160001) has been deleted
The CQ bit was checked by tobiasjs@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2242453002/diff/80001/cc/resources/video_reso... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2242453002/diff/80001/cc/resources/video_reso... cc/resources/video_resource_updater.cc:444: TextureMailbox mailbox(plane_resource.mailbox(), gpu::SyncToken(), On 2016/09/02 22:58:15, danakj wrote: > On 2016/08/30 11:31:05, Tobias Sargeant wrote: > > On 2016/08/26 18:24:23, danakj wrote: > > > why no SyncToken? > > > > This is identical to the mailbox construction on l567. Is there a reason why > > they should be different? > > Oh, I guess this class is relying on it sharing the context provider with the > compositor, okay. Would you mind saying this in a comment (at both places > maybe?), because I forgot even. I've added the comment. Given our discussion, it looks like I could also remove the sync token created in CopyPlaneTexture given that the copy happens on the compositor context. Am I correct? https://codereview.chromium.org/2242453002/diff/120001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2242453002/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater.cc:184: if (unique_id >= 0 && plane_index >= 0) { On 2016/09/02 22:58:15, danakj wrote: > i dont think we should bake in assumptions about the namespace of unique_id in > this file, VideoFrames could come with negative ids. But branching on > plane_index only would be okay. That matches your comment below also. Then maybe > use 0 for the unique_id and -1 for the plane_index? (Or just add another bool > for "allow_return_with_references" or something, up to you) Done. Went with checking only the plane index for != -1, and added comments to that effect. https://codereview.chromium.org/2242453002/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater.cc:194: for (auto it = all_resources_.begin(); it != all_resources_.end(); ++it) { On 2016/09/02 22:58:15, danakj wrote: > I kinda feel like it was a bit more clear when this was a single loop. The first > check was if Matches, which could be guarded by do we want to allow. Then the > 2nd check inside the loop was for if there's no refs. Now the !has_refs is very > hidden. I'd prefer to combine these two walks (its more efficient too), or > please provide comments on each loop explaining why we have two and what they > each hope to provide. Also we lost the bingo comment about "its safe to use this > even if there are references" which I think had value. Why'd it go away? The original loop was subtle, in that it returned reusable resources immediately but delayed returning a recyclable resource until the whole list was checked. I felt like that makes having two loops cleaner, however it was hard to keep the existing behaviour (which is to recycle the oldest available resource). For that reason I've gone back to the single loop. https://codereview.chromium.org/2242453002/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater.cc:205: if (immutable_hint == On 2016/09/02 22:58:15, danakj wrote: > Can you make this an else if () { instead of else { if () {? Done. https://codereview.chromium.org/2242453002/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater.cc:230: if (resource_id == 0) On 2016/09/02 22:58:15, danakj wrote: > Maybe it used to? But CreateResource() can't return 0: > https://cs.chromium.org/chromium/src/cc/resources/resource_provider.cc?rcl=0&... > > So this if check doesn't need to exist. And that means this function can't > return end(). Which means CopyPlaneTexture can't return false, and doesn't need > to be a bool, etc. Cleaned this up. https://codereview.chromium.org/2242453002/diff/120001/cc/resources/video_res... cc/resources/video_resource_updater.cc:621: -1, On 2016/09/02 22:58:15, danakj wrote: > can you use temp vars to give these litarals names (similar to is_immutable), > and you can put the comment on them there then. Done.
On Mon, Sep 12, 2016 at 7:25 AM, <tobiasjs@chromium.org> wrote: > > https://codereview.chromium.org/2242453002/diff/80001/cc/ > resources/video_resource_updater.cc > File cc/resources/video_resource_updater.cc (right): > > https://codereview.chromium.org/2242453002/diff/80001/cc/ > resources/video_resource_updater.cc#newcode444 > cc/resources/video_resource_updater.cc:444: TextureMailbox > mailbox(plane_resource.mailbox(), gpu::SyncToken(), > On 2016/09/02 22:58:15, danakj wrote: > > On 2016/08/30 11:31:05, Tobias Sargeant wrote: > > > On 2016/08/26 18:24:23, danakj wrote: > > > > why no SyncToken? > > > > > > This is identical to the mailbox construction on l567. Is there a > reason why > > > they should be different? > > > > Oh, I guess this class is relying on it sharing the context provider > with the > > compositor, okay. Would you mind saying this in a comment (at both > places > > maybe?), because I forgot even. > > I've added the comment. Given our discussion, it looks like I could also > remove the sync token created in CopyPlaneTexture given that the copy > happens on the compositor context. Am I correct? > Ya I think that you're right. Would you like to remove it in another CL? -- 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/2242453002/diff/200001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2242453002/diff/200001/cc/resources/video_res... cc/resources/video_resource_updater.cc:212: (resource_provider_->GetTextureHint(it->resource_id()) == There's nothing innately GPU-only about this is there? The idea of ResourceProvider is it mostly abstracts away gpu vs software things, so we should try to not branch on that when dealing with ResourceProvider, so I feel like we should do this in the software_resource case too (ie always do it). https://codereview.chromium.org/2242453002/diff/200001/cc/resources/video_res... File cc/resources/video_resource_updater.h (right): https://codereview.chromium.org/2242453002/diff/200001/cc/resources/video_res... cc/resources/video_resource_updater.h:144: // the resource is assumed to have the right data already, and be "and be read-only" => "and will only be used for reading"
https://codereview.chromium.org/2242453002/diff/200001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2242453002/diff/200001/cc/resources/video_res... cc/resources/video_resource_updater.cc:212: (resource_provider_->GetTextureHint(it->resource_id()) == On 2016/09/12 22:06:53, danakj wrote: > There's nothing innately GPU-only about this is there? The idea of > ResourceProvider is it mostly abstracts away gpu vs software things, so we > should try to not branch on that when dealing with ResourceProvider, so I feel > like we should do this in the software_resource case too (ie always do it). What do you think of ResourceProvider::IsImmutable, which always returns true for software resources, and uses GetTextureHint for GPU resources? Given that is_immutable means that the GL texture can't be respecified, it seems like it follows that all software resources are immutable.
https://codereview.chromium.org/2242453002/diff/200001/cc/resources/video_res... File cc/resources/video_resource_updater.cc (right): https://codereview.chromium.org/2242453002/diff/200001/cc/resources/video_res... cc/resources/video_resource_updater.cc:212: (resource_provider_->GetTextureHint(it->resource_id()) == On 2016/09/12 22:29:45, Tobias Sargeant wrote: > On 2016/09/12 22:06:53, danakj wrote: > > There's nothing innately GPU-only about this is there? The idea of > > ResourceProvider is it mostly abstracts away gpu vs software things, so we > > should try to not branch on that when dealing with ResourceProvider, so I feel > > like we should do this in the software_resource case too (ie always do it). > > What do you think of ResourceProvider::IsImmutable, which always returns true > for software resources, and uses GetTextureHint for GPU resources? Given that > is_immutable means that the GL texture can't be respecified, it seems like it > follows that all software resources are immutable. I think they already are, since Resource constructors set them to immutable by default. Who knows what fancy shared memory things we invent in the future though.
The CQ bit was checked by tobiasjs@chromium.org to run a CQ dry run
tobiasjs@chromium.org changed reviewers: + kbr@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
+timav, fyi. Ken, could you PTAL at content/browser/gpu/gpu_data_manager_impl_private.cc - we don't need to disable spitzer on PowerVR any more, because doing YUV->RGB conversion here means not needing to share 1 component textures via EGL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Yup. LGTM
The CQ bit was checked by tobiasjs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from liberato@chromium.org, dcheng@chromium.org, sievers@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2242453002/#ps220001 (title: "Abstract out resource immutability.")
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 tobiasjs@chromium.org
The CQ bit was checked by tobiasjs@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Avoid planar YUV resources when one component EGL images are not supported Some EGL drivers do not support binding one component textures to an EGL image. One component textures are used for YUV video frames, so to support those drivers in WebView, we need to do YUV to RGB conversion in software, and then upload an RGB texture instead of a set of 3 Y,U,V textures. BUG=579060, 632461 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Avoid planar YUV resources when one component EGL images are not supported Some EGL drivers do not support binding one component textures to an EGL image. One component textures are used for YUV video frames, so to support those drivers in WebView, we need to do YUV to RGB conversion in software, and then upload an RGB texture instead of a set of 3 Y,U,V textures. BUG=579060, 632461 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #10 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Avoid planar YUV resources when one component EGL images are not supported Some EGL drivers do not support binding one component textures to an EGL image. One component textures are used for YUV video frames, so to support those drivers in WebView, we need to do YUV to RGB conversion in software, and then upload an RGB texture instead of a set of 3 Y,U,V textures. BUG=579060, 632461 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Avoid planar YUV resources when one component EGL images are not supported Some EGL drivers do not support binding one component textures to an EGL image. One component textures are used for YUV video frames, so to support those drivers in WebView, we need to do YUV to RGB conversion in software, and then upload an RGB texture instead of a set of 3 Y,U,V textures. BUG=579060, 632461 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/410f515e005fad3917df3d44c2b35d75e9100f44 Cr-Commit-Position: refs/heads/master@{#418819} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/410f515e005fad3917df3d44c2b35d75e9100f44 Cr-Commit-Position: refs/heads/master@{#418819} |
