Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2136103010:
Add StreamTextureWrapper/StreamTextureWrapperImpl (Closed)
|
Created:
4 years, 5 months ago by tguilbert Modified:
4 years, 4 months ago CC:
avayvod+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, mlamouri+watch-content_chromium.org, mlamouri+watch-media_chromium.org, posciak+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd StreamTextureWrapperImpl
This change introduces the StreamTextureWrapper interface and its
implementation. It is meant to be used as an alternative way to provide
an Android Surface to the upcoming MediaPlayerRenderer
(see crbug.com/631199). The main advantage of using a StreamTexture is
that it allows non-fullscreen playback, as opposed to the SurfaceView.
The actual transfering of the underlying SurfaceTexture accross
processes will be covered in crbug.com/627658.
The StreamTextureWrapper class encapsulates most of the StreamTexture
logic from WMPA.
TEST=video works in current MediaPlayerRenderer prototype. Added a simple contruction/destruction UT for STW_Impl.
BUG=627632
Committed: https://crrev.com/1ed89c4109f83c56b60ede3e3c0b615c20feef37
Cr-Commit-Position: refs/heads/master@{#408279}
Patch Set 1 #Patch Set 2 #
Total comments: 16
Patch Set 3 #Patch Set 4 : Addressing comments #Patch Set 5 : Renamed to StreamTextureWrapper, handled destruction #
Total comments: 34
Patch Set 6 : Addressed comments #Patch Set 7 : Fixed include guards #
Total comments: 48
Patch Set 8 : Addressing comments #
Total comments: 9
Patch Set 9 : privatized destructors #
Total comments: 2
Patch Set 10 : Added simple UT #Patch Set 11 : Marking DTor as override instead of virtual #Patch Set 12 : Adding CONTENT_EXPORT to StreamTextureFactory #Messages
Total messages: 50 (22 generated)
tguilbert@chromium.org changed reviewers: + liberato@chromium.org, watk@chromium.org
PTAL :) N.B: I am currently investigating a bug related to WMPI crashing on destruction, when using this class. Please keep thread/process safety in mind while reviewing (although this review does not include integration with WMPI). Thank you very much! Thomas
looks nice. i could use a little more high-level info about how this is going to be used. e.g., who creates what, when, etc. mostly, i'm interested in how the browser gets pulled into the act. i'm figuring that the renderer coordinates it all, but i can picture other ways. i didn't do a super thorough review. wanted to make sure that i've got the basic idea first. thanks -fl https://codereview.chromium.org/2136103010/diff/20001/content/renderer/media/... File content/renderer/media/android/surface_texture_frame_provider_impl.cc (right): https://codereview.chromium.org/2136103010/diff/20001/content/renderer/media/... content/renderer/media/android/surface_texture_frame_provider_impl.cc:19: void OnReleaseTexture( static void https://codereview.chromium.org/2136103010/diff/20001/content/renderer/media/... content/renderer/media/android/surface_texture_frame_provider_impl.cc:39: GLES2Interface* gl = factory_->ContextGL(); can call OnReleaseTexture() to avoid duplication. https://codereview.chromium.org/2136103010/diff/20001/content/renderer/media/... content/renderer/media/android/surface_texture_frame_provider_impl.cc:48: { nit: don't need {} here. actually, can just SetCurrentFrameInternal(nullptr). https://codereview.chromium.org/2136103010/diff/20001/content/renderer/media/... content/renderer/media/android/surface_texture_frame_provider_impl.cc:86: { nit: don't need {} https://codereview.chromium.org/2136103010/diff/20001/content/renderer/media/... content/renderer/media/android/surface_texture_frame_provider_impl.cc:129: // |enable_texture_copy_| in WMPA? Should it be added here? WPMA had the it's needed for webview. COPY_REQUIRED tells the resource updater to make a copy of the SurfaceTexture front buffer into a 2D texture, and then send the 2D texture to the browser compositor. the reason that webview needs this is that its equivalent of the browser compositor operates with a different GL share group. it can't access the SurfaceTexture's texture. it can access the 2D texture via another mechanism. it's fairly easy to fix, but i'd leave it for future work. there's enough going on here already. opening a bug about supporting webview here and leaving a TODO is okay. https://codereview.chromium.org/2136103010/diff/20001/media/blink/surface_tex... File media/blink/surface_texture_frame_provider.h (right): https://codereview.chromium.org/2136103010/diff/20001/media/blink/surface_tex... media/blink/surface_texture_frame_provider.h:16: class SurfaceTextureFrameProvider : public cc::VideoFrameProvider { not sure if the additional abstraction is needed -- it's already a VideoFrameProvider. is something going to know that it has a SurfaceTextureFrameProvider that isn't also going to know about SurfaceTextureFrameProviderImpl? the reason i ask is that STFP is already a VFP, which is pretty abstract. maybe this would be better as a separate interface that has just GetSurfaceTextureIdentifier() and UpdateTextureSize(). SurfaceTextureFrameProviderImpl could inherit from that too. avoids requiring the consumer to understand cc/layers/VFP. it depends a lot on how you're planning to do setup / tear down of all of this. https://codereview.chromium.org/2136103010/diff/20001/media/blink/surface_tex... media/blink/surface_texture_frame_provider.h:30: virtual int GetSurfaceTextureIdentifier() = 0; is this to tell the browser how to find the Surface for MP? maybe just leave it out altogether since it isn't called, isn't documented, and isn't implemented. :)
https://codereview.chromium.org/2136103010/diff/20001/content/renderer/media/... File content/renderer/media/android/surface_texture_frame_provider_impl.cc (right): https://codereview.chromium.org/2136103010/diff/20001/content/renderer/media/... content/renderer/media/android/surface_texture_frame_provider_impl.cc:19: void OnReleaseTexture( On 2016/07/13 16:42:50, liberato wrote: > static void Done. https://codereview.chromium.org/2136103010/diff/20001/content/renderer/media/... content/renderer/media/android/surface_texture_frame_provider_impl.cc:39: GLES2Interface* gl = factory_->ContextGL(); On 2016/07/13 16:42:50, liberato wrote: > can call OnReleaseTexture() to avoid duplication. In order to call OnReleaseTexture(), I need a sync_token. I am not fluent in GL calls. Should I acquire one somehow and then call OnReleaseTexture(), or leave it as is? https://codereview.chromium.org/2136103010/diff/20001/content/renderer/media/... content/renderer/media/android/surface_texture_frame_provider_impl.cc:48: { On 2016/07/13 16:42:50, liberato wrote: > nit: don't need {} here. > > actually, can just SetCurrentFrameInternal(nullptr). Done. https://codereview.chromium.org/2136103010/diff/20001/content/renderer/media/... content/renderer/media/android/surface_texture_frame_provider_impl.cc:86: { On 2016/07/13 16:42:50, liberato wrote: > nit: don't need {} Done. https://codereview.chromium.org/2136103010/diff/20001/content/renderer/media/... content/renderer/media/android/surface_texture_frame_provider_impl.cc:129: // |enable_texture_copy_| in WMPA? Should it be added here? WPMA had the On 2016/07/13 16:42:50, liberato wrote: > it's needed for webview. COPY_REQUIRED tells the resource updater to make a > copy of the SurfaceTexture front buffer into a 2D texture, and then send the 2D > texture to the browser compositor. > > the reason that webview needs this is that its equivalent of the browser > compositor operates with a different GL share group. it can't access the > SurfaceTexture's texture. it can access the 2D texture via another mechanism. > > it's fairly easy to fix, but i'd leave it for future work. there's enough going > on here already. opening a bug about supporting webview here and leaving a TODO > is okay. Ok! Thanks for the background. Opened 628066. https://codereview.chromium.org/2136103010/diff/20001/media/blink/surface_tex... File media/blink/surface_texture_frame_provider.h (right): https://codereview.chromium.org/2136103010/diff/20001/media/blink/surface_tex... media/blink/surface_texture_frame_provider.h:16: class SurfaceTextureFrameProvider : public cc::VideoFrameProvider { On 2016/07/13 16:42:50, liberato wrote: > not sure if the additional abstraction is needed -- it's already a > VideoFrameProvider. is something going to know that it has a > SurfaceTextureFrameProvider that isn't also going to know about > SurfaceTextureFrameProviderImpl? > > the reason i ask is that STFP is already a VFP, which is pretty abstract. maybe > this would be better as a separate interface that has just > GetSurfaceTextureIdentifier() and UpdateTextureSize(). > SurfaceTextureFrameProviderImpl could inherit from that too. avoids requiring > the consumer to understand cc/layers/VFP. > > it depends a lot on how you're planning to do setup / tear down of all of this. I can show you the overall setup/tear down of this in the prototype that I have. This is going to be used instead of |compositor_| in WMPI, when creating the |video_weblayer_|. The main reason I created this abstraction was because the STFP_impl needs a StreamTextureFactory, which is in the content directory. From what I understand, I could not have the SFTP_impl live in the media folder since it couldn't include the StreamTextureFactory. Is there a better way to organize which file lives where? https://codereview.chromium.org/2136103010/diff/20001/media/blink/surface_tex... media/blink/surface_texture_frame_provider.h:30: virtual int GetSurfaceTextureIdentifier() = 0; On 2016/07/13 16:42:50, liberato wrote: > is this to tell the browser how to find the Surface for MP? > > maybe just leave it out altogether since it isn't called, isn't documented, and > isn't implemented. :) That is the plan :) I am using some hard coded values in my prototype, but you are right... No need in adding it here just yet then.
i'm not sure that being a VideoFrameProvider buys much, to be honest. we can chat offline. https://codereview.chromium.org/2136103010/diff/20001/content/renderer/media/... File content/renderer/media/android/surface_texture_frame_provider_impl.cc (right): https://codereview.chromium.org/2136103010/diff/20001/content/renderer/media/... content/renderer/media/android/surface_texture_frame_provider_impl.cc:39: GLES2Interface* gl = factory_->ContextGL(); On 2016/07/14 02:47:02, ThomasGuilbert wrote: > On 2016/07/13 16:42:50, liberato wrote: > > can call OnReleaseTexture() to avoid duplication. > > In order to call OnReleaseTexture(), I need a sync_token. I am not fluent in GL > calls. Should I acquire one somehow and then call OnReleaseTexture(), or leave > it as is? i think that SyncToken() creates a token that returns !SyncToken::HasData(), so you could make the wait conditional. or can leave it as is, not really a big deal either way. https://codereview.chromium.org/2136103010/diff/20001/media/blink/surface_tex... File media/blink/surface_texture_frame_provider.h (right): https://codereview.chromium.org/2136103010/diff/20001/media/blink/surface_tex... media/blink/surface_texture_frame_provider.h:16: class SurfaceTextureFrameProvider : public cc::VideoFrameProvider { On 2016/07/14 02:47:03, ThomasGuilbert wrote: > On 2016/07/13 16:42:50, liberato wrote: > > not sure if the additional abstraction is needed -- it's already a > > VideoFrameProvider. is something going to know that it has a > > SurfaceTextureFrameProvider that isn't also going to know about > > SurfaceTextureFrameProviderImpl? > > > > the reason i ask is that STFP is already a VFP, which is pretty abstract. > maybe > > this would be better as a separate interface that has just > > GetSurfaceTextureIdentifier() and UpdateTextureSize(). > > SurfaceTextureFrameProviderImpl could inherit from that too. avoids requiring > > the consumer to understand cc/layers/VFP. > > > > it depends a lot on how you're planning to do setup / tear down of all of > this. > > I can show you the overall setup/tear down of this in the prototype that I have. > This is going to be used instead of |compositor_| in WMPI, when creating the > |video_weblayer_|. > > The main reason I created this abstraction was because the STFP_impl needs a > StreamTextureFactory, which is in the content directory. > > From what I understand, I could not have the SFTP_impl live in the media folder > since it couldn't include the StreamTextureFactory. > > Is there a better way to organize which file lives where? hrm, not sure. i forgot that StreamTextureFactory is in content/, which is really annoying. STF should probably be in media, to be honest. not sure how big of a move that is, or if it's worth it.
PTAL! - I renamed the classes to StreamTextureWrapper* - I added a default_deleter and deletion logic that proved necessary, now that the StreamTextureWrapper will live within a media::Renderer in WMPI Thank you! Thomas https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... File content/renderer/media/android/stream_texture_wrapper_impl.h (right): https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.h:53: scoped_refptr<StreamTextureFactory> factory, FYI: I looked into moving StreamTextureFactory and related classes into media, but I was not able to, since they themself depend on "content/common/gpu/client/context_provider_command_buffer.h" https://codereview.chromium.org/2136103010/diff/80001/media/base/android/stre... File media/base/android/stream_texture_wrapper.cc (left): https://codereview.chromium.org/2136103010/diff/80001/media/base/android/stre... media/base/android/stream_texture_wrapper.cc:5: #ifndef TOOLS_CLANG_PLUGINS_TESTS_SYSTEM_VECTOR_ I am not sure how this showed up in this CL...
On 2016/07/21 22:03:38, ThomasGuilbert wrote: > PTAL! > > - I renamed the classes to StreamTextureWrapper* > - I added a default_deleter and deletion logic that proved necessary, now that > the StreamTextureWrapper will live within a media::Renderer in WMPI > > Thank you! > Thomas > > https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... > File content/renderer/media/android/stream_texture_wrapper_impl.h (right): > > https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... > content/renderer/media/android/stream_texture_wrapper_impl.h:53: > scoped_refptr<StreamTextureFactory> factory, > FYI: I looked into moving StreamTextureFactory and related classes into media, > but I was not able to, since they themself depend on > "content/common/gpu/client/context_provider_command_buffer.h" > > https://codereview.chromium.org/2136103010/diff/80001/media/base/android/stre... > File media/base/android/stream_texture_wrapper.cc (left): > > https://codereview.chromium.org/2136103010/diff/80001/media/base/android/stre... > media/base/android/stream_texture_wrapper.cc:5: #ifndef > TOOLS_CLANG_PLUGINS_TESTS_SYSTEM_VECTOR_ > I am not sure how this showed up in this CL... I also forgot to mention that StreamTextureWrapper is now no longer a VideoFrameProvider.
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org
Bunch of nits. https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... File content/renderer/media/android/stream_texture_wrapper_impl.cc (right): https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.cc:15: static const uint32_t kGLTextureExternalOES = 0x8D65; What is this? https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.cc:19: static void OnReleaseTexture( No static necessary when anonymous namespace is used. https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.cc:38: client_(NULL), nullptr, possibly prefer in header initialization. https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.cc:51: texture_id_ = 0; Any point? https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.cc:56: SetCurrentFrameInternal(NULL); nullptr. https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.cc:119: if (natural_size_ == new_size) Can't do this if on the wrong thread. https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.cc:151: void StreamTextureWrapperImpl::InitializeInternal( Maybe InitializeOnMainThread? https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... File content/renderer/media/android/stream_texture_wrapper_impl.h (right): https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.h:5: #ifndef _CONTENT_RENDERER_MEDIA_ANDROID_STREAM_TEXTURE_WRAPPER_IMPL_H_ drop leading _ https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.h:18: // thread, but will be initialized/destroyed on |main_tread_task_runner, and Missing closing | https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.h:50: class StreamTextureWrapperImpl : public media::StreamTextureWrapper { Need CONTENT_EXPORT? https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.h:54: const scoped_refptr<base::SingleThreadTaskRunner>& main_task_runner); We use pass-by-value for scoped_refptr now since they're movable. https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.h:90: unsigned int texture_id_; We try to avoid using unsigned int unless necessary. I know some of the gpu types like texture ids are just "unsigned" though. https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.h:97: unsigned int stream_id_; stream_texture_id_ ? Just unsigned as well? https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.h:117: }; DISALLOW_COPY_AND_ASSIGN() ?
Addressed Dale's comments. https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... File content/renderer/media/android/stream_texture_wrapper_impl.cc (right): https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.cc:15: static const uint32_t kGLTextureExternalOES = 0x8D65; On 2016/07/21 22:14:22, DaleCurtis wrote: > What is this? I copied it as it was from WMPA. However, it seems like the proper macro is defined when we get to this point :) https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.cc:19: static void OnReleaseTexture( On 2016/07/21 22:14:22, DaleCurtis wrote: > No static necessary when anonymous namespace is used. Done. However, liberato@ had asked me to add it in my previous versions though. I'm fine with either. https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.cc:38: client_(NULL), On 2016/07/21 22:14:22, DaleCurtis wrote: > nullptr, possibly prefer in header initialization. Done. https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.cc:51: texture_id_ = 0; On 2016/07/21 22:14:22, DaleCurtis wrote: > Any point? Not so much, you are right. https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.cc:56: SetCurrentFrameInternal(NULL); On 2016/07/21 22:14:22, DaleCurtis wrote: > nullptr. Done. https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.cc:119: if (natural_size_ == new_size) On 2016/07/21 22:14:22, DaleCurtis wrote: > Can't do this if on the wrong thread. You are right! https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.cc:151: void StreamTextureWrapperImpl::InitializeInternal( On 2016/07/21 22:14:22, DaleCurtis wrote: > Maybe InitializeOnMainThread? Done. https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... File content/renderer/media/android/stream_texture_wrapper_impl.h (right): https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.h:5: #ifndef _CONTENT_RENDERER_MEDIA_ANDROID_STREAM_TEXTURE_WRAPPER_IMPL_H_ On 2016/07/21 22:14:23, DaleCurtis wrote: > drop leading _ Done. https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.h:18: // thread, but will be initialized/destroyed on |main_tread_task_runner, and On 2016/07/21 22:14:22, DaleCurtis wrote: > Missing closing | Done. https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.h:50: class StreamTextureWrapperImpl : public media::StreamTextureWrapper { On 2016/07/21 22:14:22, DaleCurtis wrote: > Need CONTENT_EXPORT? Yes, I'm not building as component ATM however, because I need to shuffle files around in the prototype to stop breaking DEPCHECKS. https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.h:54: const scoped_refptr<base::SingleThreadTaskRunner>& main_task_runner); On 2016/07/21 22:14:23, DaleCurtis wrote: > We use pass-by-value for scoped_refptr now since they're movable. Done. https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.h:90: unsigned int texture_id_; On 2016/07/21 22:14:23, DaleCurtis wrote: > We try to avoid using unsigned int unless necessary. I know some of the gpu > types like texture ids are just "unsigned" though. StreamTextureFactory returns unsigned. I think this is one of those gpu cases. https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.h:97: unsigned int stream_id_; On 2016/07/21 22:14:22, DaleCurtis wrote: > stream_texture_id_ ? Just unsigned as well? Yes. https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.h:117: }; On 2016/07/21 22:14:22, DaleCurtis wrote: > DISALLOW_COPY_AND_ASSIGN() ? Done.
lgtm % nits. https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... File content/renderer/media/android/stream_texture_wrapper_impl.cc (right): https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.cc:143: main_task_runner_->PostTask( not sure if this matters for how you're using this, but it's a little confusing that initialization completes at some point in the future when it's run on the main thread. consider if(main..->BelongsToCurrentThread) InitializeOnMain...() else post. without seeing how it's used, i'm not sure if it's a problem or not. maybe it's fine as it is. https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.cc:166: init_cb.Run(); might want to add a comment in the .h that init_cb will be run on the main thread. https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.cc:172: // itself on |compositor_task_runner_|. s/compositor/main/ https://codereview.chromium.org/2136103010/diff/120001/media/blink/stream_tex... File media/blink/stream_texture_wrapper.h (right): https://codereview.chromium.org/2136103010/diff/120001/media/blink/stream_tex... media/blink/stream_texture_wrapper.h:22: virtual ~StreamTextureWrapper() {} since this has special requirements for delete, maybe make this (and ~Impl) protected. not sure if it actually works. :)
https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... File content/renderer/media/android/stream_texture_wrapper_impl.h (right): https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.h:90: unsigned int texture_id_; On 2016/07/21 at 23:06:42, ThomasGuilbert wrote: > On 2016/07/21 22:14:23, DaleCurtis wrote: > > We try to avoid using unsigned int unless necessary. I know some of the gpu > > types like texture ids are just "unsigned" though. > > StreamTextureFactory returns unsigned. I think this is one of those gpu cases. They're just "unsigned" though, without the "int"
I mostly had nits. Also, reminder to remove your comments in the CL description that are aimed at the reviewers. https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... File content/renderer/media/android/stream_texture_wrapper_impl.h (right): https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.h:18: // thread, but will be initialized/destroyed on |main_tread_task_runner, and main_tread_task_runner is not a thing? https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.h:19: // will signal the readyness of new frames on |compositor_task_runner_|. readiness https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... File content/renderer/media/android/stream_texture_wrapper_impl.cc (right): https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.cc:15: static const uint32_t kGLTextureExternalOES = GL_TEXTURE_EXTERNAL_OES; Any reason not to inline this? https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.cc:18: // File-static function is to allow it to run even after this class is deleted. nit: Looks like you copied this, but "file-static" is a weird term. It's a "non-member function" https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.cc:102: // } Delete commented code? Put it in the bug maybe. https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.cc:173: stream_texture_proxy_.reset(); I wish STP had clearer docs about its threading contract. Not sure if this is safe to call on any thread? https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... File content/renderer/media/android/stream_texture_wrapper_impl.h (right): https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.h:19: // thread, but will be initialized/destroyed on |main_tread_task_runner|, and typo: main_tread_task_runner Remove comma in "any, thread," nit: The threading contract could be clearer. i.e., that you can basically call all methods on any thread. https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.h:20: // will signal the readyness of new frames on |compositor_task_runner_|. readiness https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.h:25: // https://developer.android.com/reference/android/graphics/SurfaceTexture.html nit: doesn't feel like the right place to describe SurfaceTexture, but up to you if you want to keep it. https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.h:59: // StreamTextureWrapper implementation. Since you're only implementing one interface now, you can probably delete this comment. https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.h:62: // |compositor_task_runner|. Second sentence isn't super clear. It's the proxy that's being set up/bound. https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.h:80: // Destroys the StreamTextureWrapper safely on the |main_task_runner_|. StreamTextureWrapperImpl. Or even better, |this|. https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.h:95: // point for when the mailbox was produced. There's no sync point here? https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.h:102: // frame available. It should be bound to the compositor thread. frame _is_ available https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.h:105: cc::VideoFrameProvider::Client* client_; Looks like you don't need this after Initialize(). https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.h:108: gfx::Size natural_size_; This is not used to allocate a texture though. The most important thing is it's the size of the video frames. https://codereview.chromium.org/2136103010/diff/120001/media/blink/stream_tex... File media/blink/stream_texture_wrapper.h (right): https://codereview.chromium.org/2136103010/diff/120001/media/blink/stream_tex... media/blink/stream_texture_wrapper.h:5: #ifndef MEDIA_BLINK_STREAM_TEXTURE_WRAPPER_H_ Does this need to go in media/blink for some reason? Just seems like a strange place. media/base/android? https://codereview.chromium.org/2136103010/diff/120001/media/blink/stream_tex... media/blink/stream_texture_wrapper.h:15: // and registration for latter retrieval (in the Browser process). later https://codereview.chromium.org/2136103010/diff/120001/media/blink/stream_tex... media/blink/stream_texture_wrapper.h:26: // to |client|. should be a comment about where/when init_cb is run. https://codereview.chromium.org/2136103010/diff/120001/media/blink/stream_tex... media/blink/stream_texture_wrapper.h:33: // Should be called whenever the Video's natural size changes. uncapitalize Video https://codereview.chromium.org/2136103010/diff/120001/media/blink/stream_tex... media/blink/stream_texture_wrapper.h:41: // Safely destroys the StreamTextureWrapper on the appropritate thread. appropriate I would delete the comment about threading on the interface. The caller doesn't care about that implementation detail.
https://codereview.chromium.org/2136103010/diff/120001/media/blink/stream_tex... File media/blink/stream_texture_wrapper.h (right): https://codereview.chromium.org/2136103010/diff/120001/media/blink/stream_tex... media/blink/stream_texture_wrapper.h:41: // Safely destroys the StreamTextureWrapper on the appropritate thread. On 2016/07/23 03:13:40, watk wrote: > appropriate > > I would delete the comment about threading on the interface. The caller doesn't > care about that implementation detail. Actually, I was thinking about this. I think the reason it's hard to write good docs for this is because its only reason for existing is that we have to put STWI in content. As a result we end up duplicating the docs on STW and STWI and they're likely to go out of sync. Maybe you should just write something like: See StreamTextureWrapperImpl.
Description was changed from ========== Add SurfaceTextureFrameProviderImpl This change introduces the SurfaceTextureFrameProvider interface and its implementation. It is meant to be used as an alternative way to provide an Android Surface to the upcoming MediaPlayerRenderer. The main advantage of using a SurfaceTexture is that it allows non-fullscreen playback, as opposed to the SurfaceView. The actual transfering of the SurfaceTexture accross processes will be covered in crbug.com/627658. The SurfaceTextureFrameProvider class encapsulates most of the StreamTexture logic from WMPA. I am very open to renaming the class to "StreamTextureFrameProvider". There is a rather lenghty summary of the StreamTexture's mechanism that prefaces the class, and is a good introduction to what a StreamTexture is. I do not have a clear idea how to write UTs for this class, due to the complexity of the underlying layers. All suggestions are welcome. N.B: I am currently investigating a crash whenever WMPI is destroyed, which may very well be tied to improper destruction of this class. TEST=video works in current MediaPlayerRenderer prototype. BUG=627632 ========== to ========== Add StreamTextureWrapperImpl This change introduces the StreamTextureWrapper interface and its implementation. It is meant to be used as an alternative way to provide an Android Surface to the upcoming MediaPlayerRenderer (see crbug.com/631199). The main advantage of using a StreamTexture is that it allows non-fullscreen playback, as opposed to the SurfaceView. The actual transfering of the underlying SurfaceTexture accross processes will be covered in crbug.com/627658. The StreamTextureWrapper class encapsulates most of the StreamTexture logic from WMPA. TEST=video works in current MediaPlayerRenderer prototype. BUG=627632 ==========
https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... File content/renderer/media/android/stream_texture_wrapper_impl.h (right): https://codereview.chromium.org/2136103010/diff/80001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.h:90: unsigned int texture_id_; On 2016/07/22 18:09:31, DaleCurtis wrote: > On 2016/07/21 at 23:06:42, ThomasGuilbert wrote: > > On 2016/07/21 22:14:23, DaleCurtis wrote: > > > We try to avoid using unsigned int unless necessary. I know some of the gpu > > > types like texture ids are just "unsigned" though. > > > > StreamTextureFactory returns unsigned. I think this is one of those gpu cases. > > They're just "unsigned" though, without the "int" Ah, yes :) Done! https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... File content/renderer/media/android/stream_texture_wrapper_impl.cc (right): https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.cc:15: static const uint32_t kGLTextureExternalOES = GL_TEXTURE_EXTERNAL_OES; On 2016/07/23 03:13:39, watk wrote: > Any reason not to inline this? No strong reason other than, AFAIK, using the const uint32_t includes compile time type safety (as opposed to only using the pre-processor macro). https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.cc:18: // File-static function is to allow it to run even after this class is deleted. On 2016/07/23 03:13:39, watk wrote: > nit: Looks like you copied this, but "file-static" is a weird term. It's a > "non-member function" Done. https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.cc:102: // } On 2016/07/23 03:13:39, watk wrote: > Delete commented code? Put it in the bug maybe. Done. https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.cc:143: main_task_runner_->PostTask( On 2016/07/22 16:13:57, liberato wrote: > not sure if this matters for how you're using this, but it's a little confusing > that initialization completes at some point in the future when it's run on the > main thread. > > consider if(main..->BelongsToCurrentThread) InitializeOnMain...() else post. > > without seeing how it's used, i'm not sure if it's a problem or not. maybe it's > fine as it is. It would not have been a problem the way it's used right now, but that is a good point! Done. https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.cc:166: init_cb.Run(); On 2016/07/22 16:13:57, liberato wrote: > might want to add a comment in the .h that init_cb will be run on the main > thread. In the prototype, I had bound |init_cb| to the media thread, prior to calling initialize. I have now moved that binding to inside intialize, and added a comment in the .h Thanks for catching this! https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.cc:172: // itself on |compositor_task_runner_|. On 2016/07/22 16:13:57, liberato wrote: > s/compositor/main/ Done. https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.cc:173: stream_texture_proxy_.reset(); On 2016/07/23 03:13:39, watk wrote: > I wish STP had clearer docs about its threading contract. Not sure if this is > safe to call on any thread? The STP acquires a lock before releasing its reference |client_|, and then deletes itself on the message_loop that it belongs to... From my understanding, it is safe to delete the STP on any thread as long as you use the custom deleter (which is always the case for ScopedStreamTextureProxy). https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... File content/renderer/media/android/stream_texture_wrapper_impl.h (right): https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.h:19: // thread, but will be initialized/destroyed on |main_tread_task_runner|, and On 2016/07/23 03:13:39, watk wrote: > typo: main_tread_task_runner > Remove comma in "any, thread," > nit: The threading contract could be clearer. i.e., that you can basically call > all methods on any thread. Done. https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.h:20: // will signal the readyness of new frames on |compositor_task_runner_|. On 2016/07/23 03:13:40, watk wrote: > readiness Done. https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.h:25: // https://developer.android.com/reference/android/graphics/SurfaceTexture.html On 2016/07/23 03:13:40, watk wrote: > nit: doesn't feel like the right place to describe SurfaceTexture, but up to you > if you want to keep it. I'm fine with removing this. https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.h:59: // StreamTextureWrapper implementation. On 2016/07/23 03:13:40, watk wrote: > Since you're only implementing one interface now, you can probably delete this > comment. Done. https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.h:62: // |compositor_task_runner|. On 2016/07/23 03:13:40, watk wrote: > Second sentence isn't super clear. It's the proxy that's being set up/bound. Done. https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.h:80: // Destroys the StreamTextureWrapper safely on the |main_task_runner_|. On 2016/07/23 03:13:40, watk wrote: > StreamTextureWrapperImpl. Or even better, |this|. Done. https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.h:95: // point for when the mailbox was produced. On 2016/07/23 03:13:40, watk wrote: > There's no sync point here? Removed that part from the comment (which came from WMPA). https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.h:102: // frame available. It should be bound to the compositor thread. On 2016/07/23 03:13:39, watk wrote: > frame _is_ available Done. https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.h:105: cc::VideoFrameProvider::Client* client_; On 2016/07/23 03:13:40, watk wrote: > Looks like you don't need this after Initialize(). Ack. Removed from member variables and added to the Initialize(). https://codereview.chromium.org/2136103010/diff/120001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.h:108: gfx::Size natural_size_; On 2016/07/23 03:13:40, watk wrote: > This is not used to allocate a texture though. The most important thing is it's > the size of the video frames. Done. https://codereview.chromium.org/2136103010/diff/120001/media/blink/stream_tex... File media/blink/stream_texture_wrapper.h (right): https://codereview.chromium.org/2136103010/diff/120001/media/blink/stream_tex... media/blink/stream_texture_wrapper.h:5: #ifndef MEDIA_BLINK_STREAM_TEXTURE_WRAPPER_H_ On 2016/07/23 03:13:40, watk wrote: > Does this need to go in media/blink for some reason? Just seems like a strange > place. media/base/android? I had it in media/base/android earlier, but ran into dep check errors. It needs to live in media/blink because it is including cc/layers. I opened 631178 to address this issue, because also changing StreamTextureProxy is beyond the scope of this CL. https://codereview.chromium.org/2136103010/diff/120001/media/blink/stream_tex... media/blink/stream_texture_wrapper.h:15: // and registration for latter retrieval (in the Browser process). On 2016/07/23 03:13:40, watk wrote: > later Done. https://codereview.chromium.org/2136103010/diff/120001/media/blink/stream_tex... media/blink/stream_texture_wrapper.h:22: virtual ~StreamTextureWrapper() {} On 2016/07/22 16:13:57, liberato wrote: > since this has special requirements for delete, maybe make this (and ~Impl) > protected. not sure if it actually works. :) I have moved this destructor to protected. It does not work for StreamTextureWrapperImpl however, since Destroy uses main_thread_task_runner_->DeleteSoon(), which uses the public destructor. I could change the Destroy() to instead post to the main thread and then call "delete this", but I am not familiar with all the possible implications of such a change... Do you have any preferences? https://codereview.chromium.org/2136103010/diff/120001/media/blink/stream_tex... media/blink/stream_texture_wrapper.h:26: // to |client|. On 2016/07/23 03:13:40, watk wrote: > should be a comment about where/when init_cb is run. Done. https://codereview.chromium.org/2136103010/diff/120001/media/blink/stream_tex... media/blink/stream_texture_wrapper.h:33: // Should be called whenever the Video's natural size changes. On 2016/07/23 03:13:40, watk wrote: > uncapitalize Video Done. https://codereview.chromium.org/2136103010/diff/120001/media/blink/stream_tex... media/blink/stream_texture_wrapper.h:41: // Safely destroys the StreamTextureWrapper on the appropritate thread. On 2016/07/23 03:13:40, watk wrote: > appropriate > > I would delete the comment about threading on the interface. The caller doesn't > care about that implementation detail. Done. https://codereview.chromium.org/2136103010/diff/120001/media/blink/stream_tex... media/blink/stream_texture_wrapper.h:41: // Safely destroys the StreamTextureWrapper on the appropritate thread. On 2016/07/25 21:47:14, watk wrote: > On 2016/07/23 03:13:40, watk wrote: > > appropriate > > > > I would delete the comment about threading on the interface. The caller > doesn't > > care about that implementation detail. > > Actually, I was thinking about this. I think the reason it's hard to write good > docs for this is because its only reason for existing is that we have to put > STWI in content. As a result we end up duplicating the docs on STW and STWI and > they're likely to go out of sync. Maybe you should just write something like: > See StreamTextureWrapperImpl. > I did it one way in this patch. I can also remove all function comments except "See StreamTextureWrapperImpl" to truly force readers to see the latest comments.
https://codereview.chromium.org/2136103010/diff/140001/content/renderer/media... File content/renderer/media/android/stream_texture_wrapper_impl.cc (right): https://codereview.chromium.org/2136103010/diff/140001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.cc:137: InitializeOnMainThread(client, init_cb); It's worth noting in the header that init_cb could be called before Initialize() returns. Or alternatively, always post init_cb. https://codereview.chromium.org/2136103010/diff/140001/content/renderer/media... File content/renderer/media/android/stream_texture_wrapper_impl.h (right): https://codereview.chromium.org/2136103010/diff/140001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.h:83: void Destroy() override; Destroy should probably not be public. You should be able to friend the default_delete so that it can call it.
Should we have a media/blink/android directory? https://codereview.chromium.org/2136103010/diff/140001/content/renderer/media... File content/renderer/media/android/stream_texture_wrapper_impl.cc (right): https://codereview.chromium.org/2136103010/diff/140001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.cc:20: const scoped_refptr<content::StreamTextureFactory>& factories, no const& for scoped_refptr ? https://codereview.chromium.org/2136103010/diff/140001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.cc:56: base::AutoLock auto_lock(current_frame_lock_); What thread is this called on if not the main thread? Compositor and render? https://codereview.chromium.org/2136103010/diff/140001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.cc:137: InitializeOnMainThread(client, init_cb); On 2016/07/26 at 20:02:32, watk wrote: > It's worth noting in the header that init_cb could be called before Initialize() returns. Or alternatively, always post init_cb. +1, keep it consistent to reduce oddities.
@Dale we should not have a media/blink/android folder. I am planning on moving StreamTextureWrapper back to media/base/android, once I remove StreamTextureProxy's coupling to VideoFrameProvider::Client. See crbug.com/631178. https://codereview.chromium.org/2136103010/diff/140001/content/renderer/media... File content/renderer/media/android/stream_texture_wrapper_impl.cc (right): https://codereview.chromium.org/2136103010/diff/140001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.cc:20: const scoped_refptr<content::StreamTextureFactory>& factories, On 2016/07/26 20:13:40, DaleCurtis wrote: > no const& for scoped_refptr ? Missed this one. Done. https://codereview.chromium.org/2136103010/diff/140001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.cc:56: base::AutoLock auto_lock(current_frame_lock_); On 2016/07/26 20:13:40, DaleCurtis wrote: > What thread is this called on if not the main thread? Compositor and render? On the compositor thread only (|compositor_task_runner_|). https://codereview.chromium.org/2136103010/diff/140001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.cc:137: InitializeOnMainThread(client, init_cb); On 2016/07/26 20:13:40, DaleCurtis wrote: > On 2016/07/26 at 20:02:32, watk wrote: > > It's worth noting in the header that init_cb could be called before > Initialize() returns. Or alternatively, always post init_cb. > > +1, keep it consistent to reduce oddities. Done. https://codereview.chromium.org/2136103010/diff/140001/content/renderer/media... File content/renderer/media/android/stream_texture_wrapper_impl.h (right): https://codereview.chromium.org/2136103010/diff/140001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.h:83: void Destroy() override; On 2016/07/26 20:02:32, watk wrote: > Destroy should probably not be public. You should be able to friend the > default_delete so that it can call it. Changed the logic, and then re-integrated this patch #9 into the prototype to confirm that it does work as intended.
sgtm, seems like you could write a tiny unit test for the impl class. Just construct, destroy, call a dummy init? https://codereview.chromium.org/2136103010/diff/160001/content/renderer/media... File content/renderer/media/android/stream_texture_wrapper_impl.cc (right): https://codereview.chromium.org/2136103010/diff/160001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.cc:145: media::BindToCurrentLoop(init_cb))); No need to bind to current loop if you're always posting.
Description was changed from ========== Add StreamTextureWrapperImpl This change introduces the StreamTextureWrapper interface and its implementation. It is meant to be used as an alternative way to provide an Android Surface to the upcoming MediaPlayerRenderer (see crbug.com/631199). The main advantage of using a StreamTexture is that it allows non-fullscreen playback, as opposed to the SurfaceView. The actual transfering of the underlying SurfaceTexture accross processes will be covered in crbug.com/627658. The StreamTextureWrapper class encapsulates most of the StreamTexture logic from WMPA. TEST=video works in current MediaPlayerRenderer prototype. BUG=627632 ========== to ========== Add StreamTextureWrapperImpl This change introduces the StreamTextureWrapper interface and its implementation. It is meant to be used as an alternative way to provide an Android Surface to the upcoming MediaPlayerRenderer (see crbug.com/631199). The main advantage of using a StreamTexture is that it allows non-fullscreen playback, as opposed to the SurfaceView. The actual transfering of the underlying SurfaceTexture accross processes will be covered in crbug.com/627658. The StreamTextureWrapper class encapsulates most of the StreamTexture logic from WMPA. TEST=video works in current MediaPlayerRenderer prototype. Added a simple contruction/destruction UT for STW_Impl. BUG=627632 ==========
@Dale I added a simple UT that creates and destroys a StreamTextureWrapperImpl. It is a bit more of a "compilation guard" that makes sure that Destroy() is accessible to StreamTextureWrapper::Deleter. Running a dummy Initialize() proved to be infeasible, due to STWI's need for a SteamTextureFactory, which itself needs a ContextProviderCommandBuffer (which, AFAIK, does not have a test equivalent). https://codereview.chromium.org/2136103010/diff/160001/content/renderer/media... File content/renderer/media/android/stream_texture_wrapper_impl.cc (right): https://codereview.chromium.org/2136103010/diff/160001/content/renderer/media... content/renderer/media/android/stream_texture_wrapper_impl.cc:145: media::BindToCurrentLoop(init_cb))); On 2016/07/26 22:33:09, DaleCurtis wrote: > No need to bind to current loop if you're always posting. Talked offline. Summary: The owner of this class will not be WPMI directly, but MediaPlayerRendererHost. This means that it is acceptable to bind to the loop here, rather than to use the BIND_TO_RENDER_LOOP macro, already present in WMPI.
lgtm
lgtm
The CQ bit was checked by watk@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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by tguilbert@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 tguilbert@chromium.org
The CQ bit was checked by tguilbert@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from liberato@chromium.org, watk@chromium.org, dalecurtis@chromium.org Link to the patchset: https://codereview.chromium.org/2136103010/#ps200001 (title: "Marking DTor as override instead of virtual")
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 tguilbert@chromium.org
The CQ bit was checked by tguilbert@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org, dalecurtis@chromium.org, liberato@chromium.org Link to the patchset: https://codereview.chromium.org/2136103010/#ps220001 (title: "Adding CONTENT_EXPORT to StreamTextureFactory")
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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tguilbert@chromium.org
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: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by tguilbert@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 ========== Add StreamTextureWrapperImpl This change introduces the StreamTextureWrapper interface and its implementation. It is meant to be used as an alternative way to provide an Android Surface to the upcoming MediaPlayerRenderer (see crbug.com/631199). The main advantage of using a StreamTexture is that it allows non-fullscreen playback, as opposed to the SurfaceView. The actual transfering of the underlying SurfaceTexture accross processes will be covered in crbug.com/627658. The StreamTextureWrapper class encapsulates most of the StreamTexture logic from WMPA. TEST=video works in current MediaPlayerRenderer prototype. Added a simple contruction/destruction UT for STW_Impl. BUG=627632 ========== to ========== Add StreamTextureWrapperImpl This change introduces the StreamTextureWrapper interface and its implementation. It is meant to be used as an alternative way to provide an Android Surface to the upcoming MediaPlayerRenderer (see crbug.com/631199). The main advantage of using a StreamTexture is that it allows non-fullscreen playback, as opposed to the SurfaceView. The actual transfering of the underlying SurfaceTexture accross processes will be covered in crbug.com/627658. The StreamTextureWrapper class encapsulates most of the StreamTexture logic from WMPA. TEST=video works in current MediaPlayerRenderer prototype. Added a simple contruction/destruction UT for STW_Impl. BUG=627632 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Add StreamTextureWrapperImpl This change introduces the StreamTextureWrapper interface and its implementation. It is meant to be used as an alternative way to provide an Android Surface to the upcoming MediaPlayerRenderer (see crbug.com/631199). The main advantage of using a StreamTexture is that it allows non-fullscreen playback, as opposed to the SurfaceView. The actual transfering of the underlying SurfaceTexture accross processes will be covered in crbug.com/627658. The StreamTextureWrapper class encapsulates most of the StreamTexture logic from WMPA. TEST=video works in current MediaPlayerRenderer prototype. Added a simple contruction/destruction UT for STW_Impl. BUG=627632 ========== to ========== Add StreamTextureWrapperImpl This change introduces the StreamTextureWrapper interface and its implementation. It is meant to be used as an alternative way to provide an Android Surface to the upcoming MediaPlayerRenderer (see crbug.com/631199). The main advantage of using a StreamTexture is that it allows non-fullscreen playback, as opposed to the SurfaceView. The actual transfering of the underlying SurfaceTexture accross processes will be covered in crbug.com/627658. The StreamTextureWrapper class encapsulates most of the StreamTexture logic from WMPA. TEST=video works in current MediaPlayerRenderer prototype. Added a simple contruction/destruction UT for STW_Impl. BUG=627632 Committed: https://crrev.com/1ed89c4109f83c56b60ede3e3c0b615c20feef37 Cr-Commit-Position: refs/heads/master@{#408279} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/1ed89c4109f83c56b60ede3e3c0b615c20feef37 Cr-Commit-Position: refs/heads/master@{#408279} |
