Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2052103002:
Enable deferred rendering strategy for Android WebView for Spitzer (Closed)
|
Created:
4 years, 6 months ago by Tima Vaisburd Modified:
4 years, 6 months ago CC:
chromium-reviews, posciak+watch_chromium.org, feature-media-reviews_chromium.org, piman+watch_chromium.org, sgurun-gerrit only, watk, DaleCurtis Base URL:
https://chromium.googlesource.com/chromium/src.git@spitzer-aw-disable-tests Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEnable deferred rendering strategy for Android WebView for Spitzer
In Android Webview (single process) AVDA deferred strategy can work
if we require texture copying during composition with frame's
COPY_REQUIRED flag. This CL sets AVDA deferred strategy for all
cases and sets COPY_REQUIRED for WebView.
This is prerequisite for https://codereview.chromium.org/2057743002
BUG=582170, 597495
Committed: https://crrev.com/bb95c90f63793ada603c740e3a48a2c5fbc01d2e
Cr-Commit-Position: refs/heads/master@{#401967}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Have deferred strategy for WebView but never SurfaceView in fullscreen; rebased #Patch Set 3 : Removed test files that we added by mistake #
Total comments: 11
Patch Set 4 : Renamed one method, rebased #Patch Set 5 : Addressed comments, fixed Windows compilation #
Messages
Total messages: 26 (8 generated)
timav@chromium.org changed reviewers: + boliu@chromium.org, liberato@chromium.org
I changed the order of CL dependencies, this is CL 2 out of 3 for COPY_REQUIRED. Frank> what happened with the errors from the mailbox manager with COPY_REQUIRED? Still working on this. Sending early to verify the approach, now I pass the flag with capabilities from AVDA. https://codereview.chromium.org/2052103002/diff/1/media/filters/gpu_video_dec... File media/filters/gpu_video_decoder.cc (right): https://codereview.chromium.org/2052103002/diff/1/media/filters/gpu_video_dec... media/filters/gpu_video_decoder.cc:205: requires_texture_copy_ = Bo Liu> do this: Bo Liu> https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?rc... I followed liberato@'s advise to pass the flag through Capabilities, in this case it seems we can keep the existing command line option?
Description was changed from ========== Enable deferred rendering strategy for Android WebView for Spitzer In Android Webview (single process) AVDA deferred strategy can work if we require texture copying during composition with frame's COPY_REQUIRED flag. This CL sets AVDA deferred strategy for all cases and sets COPY_REQUIRED for WebView. BUG=582170, 597495 ========== to ========== Enable deferred rendering strategy for Android WebView for Spitzer In Android Webview (single process) AVDA deferred strategy can work if we require texture copying during composition with frame's COPY_REQUIRED flag. This CL sets AVDA deferred strategy for all cases and sets COPY_REQUIRED for WebView. This is prerequisite for https://codereview.chromium.org/2057743002 BUG=582170, 597495 ==========
Description was changed from ========== Enable deferred rendering strategy for Android WebView for Spitzer In Android Webview (single process) AVDA deferred strategy can work if we require texture copying during composition with frame's COPY_REQUIRED flag. This CL sets AVDA deferred strategy for all cases and sets COPY_REQUIRED for WebView. This is prerequisite for https://codereview.chromium.org/2057743002 BUG=582170, 597495 ========== to ========== Enable deferred rendering strategy for Android WebView for Spitzer In Android Webview (single process) AVDA deferred strategy can work if we require texture copying during composition with frame's COPY_REQUIRED flag. This CL sets AVDA deferred strategy for all cases and sets COPY_REQUIRED for WebView. This is prerequisite for https://codereview.chromium.org/2057743002 BUG=582170, 597495 ==========
https://codereview.chromium.org/2052103002/diff/1/media/filters/gpu_video_dec... File media/filters/gpu_video_decoder.cc (right): https://codereview.chromium.org/2052103002/diff/1/media/filters/gpu_video_dec... media/filters/gpu_video_decoder.cc:205: requires_texture_copy_ = On 2016/06/09 22:51:26, Tima Vaisburd wrote: > Bo Liu> do this: > Bo Liu> > https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?rc... > > I followed liberato@'s advise to pass the flag through Capabilities, in this > case it seems we can keep the existing command line option? You are not actually using the command line here though? Using GpuPreferences is fine
https://codereview.chromium.org/2052103002/diff/1/media/filters/gpu_video_dec... File media/filters/gpu_video_decoder.cc (right): https://codereview.chromium.org/2052103002/diff/1/media/filters/gpu_video_dec... media/filters/gpu_video_decoder.cc:205: requires_texture_copy_ = On 2016/06/10 00:41:03, boliu wrote: > On 2016/06/09 22:51:26, Tima Vaisburd wrote: > > Bo Liu> do this: > > Bo Liu> > > > https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?rc... > > > > I followed liberato@'s advise to pass the flag through Capabilities, in this > > case it seems we can keep the existing command line option? > > You are not actually using the command line here though? > > Using GpuPreferences is fine VideoDecodeAccelerator::Capabilities extracts the flag from GpuPreferences. They copy command line flag that is set here: https://cs.chromium.org/chromium/src/android_webview/lib/main/aw_main_delegat...
https://codereview.chromium.org/2052103002/diff/1/media/filters/gpu_video_dec... File media/filters/gpu_video_decoder.cc (right): https://codereview.chromium.org/2052103002/diff/1/media/filters/gpu_video_dec... media/filters/gpu_video_decoder.cc:205: requires_texture_copy_ = On 2016/06/10 00:49:41, Tima Vaisburd wrote: > On 2016/06/10 00:41:03, boliu wrote: > > On 2016/06/09 22:51:26, Tima Vaisburd wrote: > > > Bo Liu> do this: > > > Bo Liu> > > > > > > https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?rc... > > > > > > I followed liberato@'s advise to pass the flag through Capabilities, in this > > > case it seems we can keep the existing command line option? > > > > You are not actually using the command line here though? > > > > Using GpuPreferences is fine > > VideoDecodeAccelerator::Capabilities extracts the flag from GpuPreferences. which is fine > They > copy command line flag that is set here: > https://cs.chromium.org/chromium/src/android_webview/lib/main/aw_main_delegat... I meant you are not using the switch in this CL
https://codereview.chromium.org/2052103002/diff/1/media/filters/gpu_video_dec... l.598 Changed from if (requires_texture_copy_) frame->metadata()->SetBoolean(VideoFrameMetadata::COPY_REQUIRED, true); to if (requires_texture_copy_ && current_surface_id_ == -1) frame->metadata()->SetBoolean(VideoFrameMetadata::COPY_REQUIRED, true); assuming that current_surface_id will indicate whether we are in full screen r not. I set it GpuVideoDecoder::CompleteInitialization(). -- 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.
Oops, wrong thread, sorry. Please ignore. On Fri, Jun 10, 2016 at 4:10 PM, Tima Vaisburd <timav@google.com> wrote: > > > https://codereview.chromium.org/2052103002/diff/1/media/filters/gpu_video_dec... > l.598 > > Changed from > if (requires_texture_copy_) > frame->metadata()->SetBoolean(VideoFrameMetadata::COPY_REQUIRED, true); > to > if (requires_texture_copy_ && current_surface_id_ == -1) > frame->metadata()->SetBoolean(VideoFrameMetadata::COPY_REQUIRED, true); > > assuming that current_surface_id will indicate whether we are in full > screen r not. I set it GpuVideoDecoder::CompleteInitialization(). > -- 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.
timav@chromium.org changed reviewers: + watk@chromium.org
Following offline discussion I never create SurfaceView for WebView. PTAL.
oops, forgot to press 'send'. lgtm % nit. -fl https://codereview.chromium.org/2052103002/diff/40001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2052103002/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1675: return gpu_preferences.enable_threaded_texture_mailboxes; i think that it should either (a) return false if we're using the deferred strategy, or (b) the function name should qualify that it's only applicable to the deferred strategy.
https://codereview.chromium.org/2052103002/diff/40001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2052103002/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1675: return gpu_preferences.enable_threaded_texture_mailboxes; On 2016/06/24 17:15:45, liberato wrote: > i think that it should either (a) return false if we're using the deferred > strategy, or (b) the function name should qualify that it's only applicable to > the deferred strategy. I think only (b) is correct: we always use deferred strategy, for WebView it does require texture copy and for Chrome it does not. Or am I missing something?
https://codereview.chromium.org/2052103002/diff/40001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2052103002/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1675: return gpu_preferences.enable_threaded_texture_mailboxes; On 2016/06/24 17:22:54, Tima Vaisburd wrote: > On 2016/06/24 17:15:45, liberato wrote: > > i think that it should either (a) return false if we're using the deferred > > strategy, or (b) the function name should qualify that it's only applicable to > > the deferred strategy. > > I think only (b) is correct: we always use deferred strategy, for WebView it > does require texture copy and for Chrome it does not. Or am I missing something? either a or b works. if we (a) keep the current name, then we should be sure that it returns false if UseDeferred...() returns false, since that's what the name (and comments) say. the texture copy is not required for the copying strategy. either (a) or (b) is fine with me. as it is, this will return true even if UseDeferred selects the copying strategy. that's unexpected behavior given the function name and comments.
https://codereview.chromium.org/2052103002/diff/40001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2052103002/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1675: return gpu_preferences.enable_threaded_texture_mailboxes; On 2016/06/24 17:28:32, liberato wrote: > On 2016/06/24 17:22:54, Tima Vaisburd wrote: > > On 2016/06/24 17:15:45, liberato wrote: > > > i think that it should either (a) return false if we're using the deferred > > > strategy, or (b) the function name should qualify that it's only applicable > to > > > the deferred strategy. > > > > I think only (b) is correct: we always use deferred strategy, for WebView it > > does require texture copy and for Chrome it does not. Or am I missing > something? > > either a or b works. if we (a) keep the current name, then we should be sure > that it returns false if UseDeferred...() returns false, since that's what the > name (and comments) say. the texture copy is not required for the copying > strategy. > > either (a) or (b) is fine with me. as it is, this will return true even if > UseDeferred selects the copying strategy. that's unexpected behavior given the > function name and comments. I see. I renamed the method to UseTextureCopyForDeferredStrategy
https://codereview.chromium.org/2052103002/diff/40001/media/filters/gpu_video... File media/filters/gpu_video_decoder.cc (right): https://codereview.chromium.org/2052103002/diff/40001/media/filters/gpu_video... media/filters/gpu_video_decoder.cc:615: // with COPY_REQUIRED. See http://crbug.com/582170. Comment doesn't make much sense unless you know the history of this work. It's also not obvious why this line relates to webview. https://codereview.chromium.org/2052103002/diff/40001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2052103002/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:537: strategy_.reset(new AndroidCopyingBackingStrategy(this)); No way to get into this branch now. I hope that means we can delete the copying strategy :) https://codereview.chromium.org/2052103002/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1668: return true; Delete this now? https://codereview.chromium.org/2052103002/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1746: // Fullscreen external SurfaceView is disabled for WenView. s/wenview/webview
https://codereview.chromium.org/2052103002/diff/40001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2052103002/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1668: return true; On 2016/06/24 17:54:46, watk wrote: > Delete this now? i think holding onto it still has value. once we go a quarter without using it internally, then yes :)
https://codereview.chromium.org/2052103002/diff/40001/media/filters/gpu_video... File media/filters/gpu_video_decoder.cc (right): https://codereview.chromium.org/2052103002/diff/40001/media/filters/gpu_video... media/filters/gpu_video_decoder.cc:615: // with COPY_REQUIRED. See http://crbug.com/582170. On 2016/06/24 17:54:46, watk wrote: > Comment doesn't make much sense unless you know the history of this work. It's > also not obvious why this line relates to webview. I removed the comment altogether. https://codereview.chromium.org/2052103002/diff/40001/media/gpu/android_video... File media/gpu/android_video_decode_accelerator.cc (right): https://codereview.chromium.org/2052103002/diff/40001/media/gpu/android_video... media/gpu/android_video_decode_accelerator.cc:1746: // Fullscreen external SurfaceView is disabled for WenView. On 2016/06/24 17:54:46, watk wrote: > s/wenview/webview Done.
lgtm
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from liberato@chromium.org Link to the patchset: https://codereview.chromium.org/2052103002/#ps80001 (title: "Addressed comments, fixed Windows compilation")
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 ========== Enable deferred rendering strategy for Android WebView for Spitzer In Android Webview (single process) AVDA deferred strategy can work if we require texture copying during composition with frame's COPY_REQUIRED flag. This CL sets AVDA deferred strategy for all cases and sets COPY_REQUIRED for WebView. This is prerequisite for https://codereview.chromium.org/2057743002 BUG=582170, 597495 ========== to ========== Enable deferred rendering strategy for Android WebView for Spitzer In Android Webview (single process) AVDA deferred strategy can work if we require texture copying during composition with frame's COPY_REQUIRED flag. This CL sets AVDA deferred strategy for all cases and sets COPY_REQUIRED for WebView. This is prerequisite for https://codereview.chromium.org/2057743002 BUG=582170, 597495 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Enable deferred rendering strategy for Android WebView for Spitzer In Android Webview (single process) AVDA deferred strategy can work if we require texture copying during composition with frame's COPY_REQUIRED flag. This CL sets AVDA deferred strategy for all cases and sets COPY_REQUIRED for WebView. This is prerequisite for https://codereview.chromium.org/2057743002 BUG=582170, 597495 ========== to ========== Enable deferred rendering strategy for Android WebView for Spitzer In Android Webview (single process) AVDA deferred strategy can work if we require texture copying during composition with frame's COPY_REQUIRED flag. This CL sets AVDA deferred strategy for all cases and sets COPY_REQUIRED for WebView. This is prerequisite for https://codereview.chromium.org/2057743002 BUG=582170, 597495 Committed: https://crrev.com/bb95c90f63793ada603c740e3a48a2c5fbc01d2e Cr-Commit-Position: refs/heads/master@{#401967} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/bb95c90f63793ada603c740e3a48a2c5fbc01d2e Cr-Commit-Position: refs/heads/master@{#401967} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
