Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2390783003:
Make stream_id internal to StreamTextureHost. (Closed)
|
Created:
4 years, 2 months ago by sivag Modified:
4 years, 2 months ago Reviewers:
Ken Russell (switch to Gerrit), tguilbert (use other), liberato (no reviews please), tguilbert, piman CC:
chromium-reviews, mlamouri+watch-content_chromium.org, posciak+watch_chromium.org, avayvod+watch_chromium.org, jam, feature-media-reviews_chromium.org, darin-cc_chromium.org, mcasas+watch+vc_chromium.org, piman+watch_chromium.org, mlamouri+watch-media_chromium.org, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake stream_id internal to StreamTextureHost.
1.Move code in between StreamTextureFactory/Proxy
and StreamTextureHost.
2.EstablishPeer and SetSize would go throughStreamTextureProxy
instead, so outside of proxy/host nobody(WebMediaPlayer)needs
to know the stream/route id).
3.Also rename stream_id to route_id.
BUG=516585, 339191
Committed: https://crrev.com/6216f318b43cb2dbb784409c5be8122d1b4fd582
Cr-Commit-Position: refs/heads/master@{#426739}
Patch Set 1 #Patch Set 2 : Remove stream_id from WebMediaPlayer. #Patch Set 3 : Rebasing to TOT. #
Total comments: 6
Patch Set 4 : Addressed review comments. #
Total comments: 7
Patch Set 5 : Handle failure conditions of route_id. #Messages
Total messages: 46 (31 generated)
Description was changed from ========== Make stream_id internal to StreamTextureHost. Move code around a bit in between StreamTextureFactory/Proxy and StreamTextureHost (EstablishPeer and SetSize would go through StreamTextureProxy instead, so outside of proxy/host nobody(WebMediaPlayer)needs to know the stream/route id). Also rename stream_id to route_id. BUG=516585 ========== to ========== Make stream_id internal to StreamTextureHost. 1.Move code around a bit in between StreamTextureFactory/Proxy and StreamTextureHost. 2.EstablishPeer and SetSize would go through StreamTextureProxy instead, so outside of proxy/host nobody(WebMediaPlayer)needs to know the stream/route id). 3.Also rename stream_id to route_id. BUG=516585 ==========
Description was changed from ========== Make stream_id internal to StreamTextureHost. 1.Move code around a bit in between StreamTextureFactory/Proxy and StreamTextureHost. 2.EstablishPeer and SetSize would go through StreamTextureProxy instead, so outside of proxy/host nobody(WebMediaPlayer)needs to know the stream/route id). 3.Also rename stream_id to route_id. BUG=516585 ========== to ========== Make stream_id internal to StreamTextureHost. 1.Move code around a bit in between StreamTextureFactory/Proxy and StreamTextureHost. 2.EstablishPeer and SetSize would go throughStreamTextureProxy instead, so outside of proxy/host nobody(WebMediaPlayer)needs to know the stream/route id). 3.Also rename stream_id to route_id. BUG=516585 ==========
The CQ bit was checked by siva.gunturi@samsung.com 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.
The CQ bit was checked by siva.gunturi@samsung.com 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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by siva.gunturi@samsung.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Make stream_id internal to StreamTextureHost. 1.Move code around a bit in between StreamTextureFactory/Proxy and StreamTextureHost. 2.EstablishPeer and SetSize would go throughStreamTextureProxy instead, so outside of proxy/host nobody(WebMediaPlayer)needs to know the stream/route id). 3.Also rename stream_id to route_id. BUG=516585 ========== to ========== Make stream_id internal to StreamTextureHost. 1.Move code in between StreamTextureFactory/Proxy and StreamTextureHost. 2.EstablishPeer and SetSize would go throughStreamTextureProxy instead, so outside of proxy/host nobody(WebMediaPlayer)needs to know the stream/route id). 3.Also rename stream_id to route_id. BUG=516585 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
siva.gunturi@samsung.com changed reviewers: + liberato@chromium.org, tguilbert@chromium.org
ptal..
https://codereview.chromium.org/2390783003/diff/40001/content/renderer/media/... File content/renderer/media/android/stream_texture_wrapper_impl.cc (right): https://codereview.chromium.org/2390783003/diff/40001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.cc:48: texture_id_ = 0; why? (i have a habit of doing this too -- but i haven't noticed the practice often in chromium.) if it's really needed, please comment why. https://codereview.chromium.org/2390783003/diff/40001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/2390783003/diff/40001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1158: int32_t route_id = DoCreateStreamTexture(); can this fail? https://codereview.chromium.org/2390783003/diff/40001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1159: stream_texture_proxy_.reset(stream_texture_factory_->CreateProxy(route_id)); perhaps this can be folded into DoCreateStreamTexture? or even into CreateProxy()? i.e., is there any use of factory::CreateStreamTexture that isn't immediately followed by CreateProxy? i don't remember if tguilbert's work on MediaPlayerRenderer needs that, but i don't think so.
The CQ bit was checked by siva.gunturi@samsung.com 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...
siva.gunturi@samsung.com changed reviewers: + kbr@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ptal... https://codereview.chromium.org/2390783003/diff/40001/content/renderer/media/... File content/renderer/media/android/stream_texture_wrapper_impl.cc (right): https://codereview.chromium.org/2390783003/diff/40001/content/renderer/media/... content/renderer/media/android/stream_texture_wrapper_impl.cc:48: texture_id_ = 0; On 2016/10/07 20:01:26, liberato wrote: > why? (i have a habit of doing this too -- but i haven't noticed the practice > often in chromium.) > > if it's really needed, please comment why. There is no reason to do it in the destructor. I will remove this. https://codereview.chromium.org/2390783003/diff/40001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/2390783003/diff/40001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1158: int32_t route_id = DoCreateStreamTexture(); On 2016/10/07 20:01:26, liberato wrote: > can this fail? Done. https://codereview.chromium.org/2390783003/diff/40001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1159: stream_texture_proxy_.reset(stream_texture_factory_->CreateProxy(route_id)); On 2016/10/07 20:01:26, liberato wrote: > perhaps this can be folded into DoCreateStreamTexture? or even into > CreateProxy()? i.e., is there any use of factory::CreateStreamTexture that > isn't immediately followed by CreateProxy? > > i don't remember if tguilbert's work on MediaPlayerRenderer needs that, but i > don't think so. Done.
looks good. just a few minor points that might be me forgetting some detail somewhere :) thanks -fl https://codereview.chromium.org/2390783003/diff/60001/content/renderer/media/... File content/renderer/media/android/stream_texture_factory.h (right): https://codereview.chromium.org/2390783003/diff/60001/content/renderer/media/... content/renderer/media/android/stream_texture_factory.h:95: // Create the StreamTextureProxy object. please comment the function args, now that the caller doesn't call CreateStreamTexture directly. also, perhaps make this explicitly guarantee that |texture_id| will be set to 0 if it fails. this can clean up wmpa a bit (please see comments there). last, and this was an existing issue with the docs, you might want to mention that the client has to clean up |texture_id|. https://codereview.chromium.org/2390783003/diff/60001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/2390783003/diff/60001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:202: if (stream_texture_proxy_.get() && texture_id_) { there are several places where one checks for a stream texture proxy and also texture id. it's a little confusing (and was, before this CL, also), since the stream texture isn't actually used in the block that checks for it. it's just to make sure that |texture_id_| is defined, i think. please correct me if i'm wrong about that. i didn't see any case where we invalidate |texture_id_| but don't clear it also. if StreamTextureProxy guaranteed that it would be zero on failure, or if wmpa did that, then i think we could just check and clean up |texture_id_|. it's a little confusing that one must check |stream_texture_proxy_| to understand if |texture_id_| is meaningful. this was an issue before, too. however, since this CL is generally improving readability, it would be nice to fix up this too. https://codereview.chromium.org/2390783003/diff/60001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:1162: return; this is where one might set |texture_id_| to zero, if StreamTextureProxy doesn't guarantee it.
The content/renderer/gpu changes LGTM overall. https://codereview.chromium.org/2390783003/diff/60001/content/renderer/gpu/st... File content/renderer/gpu/stream_texture_host_android.cc (right): https://codereview.chromium.org/2390783003/diff/60001/content/renderer/gpu/st... content/renderer/gpu/stream_texture_host_android.cc:62: new GpuStreamTextureMsg_EstablishPeer(route_id_, frame_id, player_id)); Not knowing this API, it seems strange that the frame_id would be part of establishing the stream texture's peer, since presumably it changes all the time and presumably the StreamTexture and its peer don't.
The CQ bit was checked by siva.gunturi@samsung.com 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...
ptal... https://codereview.chromium.org/2390783003/diff/60001/content/renderer/gpu/st... File content/renderer/gpu/stream_texture_host_android.cc (right): https://codereview.chromium.org/2390783003/diff/60001/content/renderer/gpu/st... content/renderer/gpu/stream_texture_host_android.cc:62: new GpuStreamTextureMsg_EstablishPeer(route_id_, frame_id, player_id)); On 2016/10/10 23:34:24, Ken Russell OOO-till-Oct-17 wrote: > Not knowing this API, it seems strange that the frame_id would be part of > establishing the stream texture's peer, since presumably it changes all the time > and presumably the StreamTexture and its peer don't. its used to get the MediaPlayer from the manager and set the videosurface. Need to check if we can avoid that. https://cs.chromium.org/chromium/src/content/browser/media/android/browser_me... https://codereview.chromium.org/2390783003/diff/60001/content/renderer/media/... File content/renderer/media/android/stream_texture_factory.h (right): https://codereview.chromium.org/2390783003/diff/60001/content/renderer/media/... content/renderer/media/android/stream_texture_factory.h:95: // Create the StreamTextureProxy object. On 2016/10/10 16:46:13, liberato wrote: > please comment the function args, now that the caller doesn't call > CreateStreamTexture directly. > > also, perhaps make this explicitly guarantee that |texture_id| will be set to 0 > if it fails. this can clean up wmpa a bit (please see comments there). > > last, and this was an existing issue with the docs, you might want to mention > that the client has to clean up |texture_id|. Done. https://codereview.chromium.org/2390783003/diff/60001/content/renderer/media/... File content/renderer/media/android/webmediaplayer_android.cc (right): https://codereview.chromium.org/2390783003/diff/60001/content/renderer/media/... content/renderer/media/android/webmediaplayer_android.cc:202: if (stream_texture_proxy_.get() && texture_id_) { On 2016/10/10 16:46:13, liberato wrote: > there are several places where one checks for a stream texture proxy and also > texture id. it's a little confusing (and was, before this CL, also), since the > stream texture isn't actually used in the block that checks for it. > > it's just to make sure that |texture_id_| is defined, i think. please correct > me if i'm wrong about that. i didn't see any case where we invalidate > |texture_id_| but don't clear it also. > > if StreamTextureProxy guaranteed that it would be zero on failure, or if wmpa > did that, then i think we could just check and clean up |texture_id_|. it's a > little confusing that one must check |stream_texture_proxy_| to understand if > |texture_id_| is meaningful. > > this was an issue before, too. however, since this CL is generally improving > readability, it would be nice to fix up this too. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/12 11:25:33, sivag wrote: > ptal... > > https://codereview.chromium.org/2390783003/diff/60001/content/renderer/gpu/st... > File content/renderer/gpu/stream_texture_host_android.cc (right): > > https://codereview.chromium.org/2390783003/diff/60001/content/renderer/gpu/st... > content/renderer/gpu/stream_texture_host_android.cc:62: new > GpuStreamTextureMsg_EstablishPeer(route_id_, frame_id, player_id)); > On 2016/10/10 23:34:24, Ken Russell OOO-till-Oct-17 wrote: > > Not knowing this API, it seems strange that the frame_id would be part of > > establishing the stream texture's peer, since presumably it changes all the > time > > and presumably the StreamTexture and its peer don't. > > its used to get the MediaPlayer from the manager > and set the videosurface. Need to check if we can avoid > that. > > https://cs.chromium.org/chromium/src/content/browser/media/android/browser_me... > > https://codereview.chromium.org/2390783003/diff/60001/content/renderer/media/... > File content/renderer/media/android/stream_texture_factory.h (right): > > https://codereview.chromium.org/2390783003/diff/60001/content/renderer/media/... > content/renderer/media/android/stream_texture_factory.h:95: // Create the > StreamTextureProxy object. > On 2016/10/10 16:46:13, liberato wrote: > > please comment the function args, now that the caller doesn't call > > CreateStreamTexture directly. > > > > also, perhaps make this explicitly guarantee that |texture_id| will be set to > 0 > > if it fails. this can clean up wmpa a bit (please see comments there). > > > > last, and this was an existing issue with the docs, you might want to mention > > that the client has to clean up |texture_id|. > > Done. > > https://codereview.chromium.org/2390783003/diff/60001/content/renderer/media/... > File content/renderer/media/android/webmediaplayer_android.cc (right): > > https://codereview.chromium.org/2390783003/diff/60001/content/renderer/media/... > content/renderer/media/android/webmediaplayer_android.cc:202: if > (stream_texture_proxy_.get() && texture_id_) { > On 2016/10/10 16:46:13, liberato wrote: > > there are several places where one checks for a stream texture proxy and also > > texture id. it's a little confusing (and was, before this CL, also), since > the > > stream texture isn't actually used in the block that checks for it. > > > > it's just to make sure that |texture_id_| is defined, i think. please correct > > me if i'm wrong about that. i didn't see any case where we invalidate > > |texture_id_| but don't clear it also. > > > > if StreamTextureProxy guaranteed that it would be zero on failure, or if wmpa > > did that, then i think we could just check and clean up |texture_id_|. it's a > > little confusing that one must check |stream_texture_proxy_| to understand if > > |texture_id_| is meaningful. > > > > this was an issue before, too. however, since this CL is generally improving > > readability, it would be nice to fix up this too. > > Done. lg*m -- this CL is a good improvement to readability and general code sanity. :) let's wait for tguilbert's comments too, since he's been working with this code most recently. thanks -fl
@ThomasGuilbert, Ptal..
siva.gunturi@samsung.com changed reviewers: + piman@chromium.org
@Thomas, liberato, Any further comments on this patch?
On 2016/10/20 10:09:55, sivag wrote: > @Thomas, liberato, > > Any further comments on this patch? LGTM (but I don't have committer rights just yet, so liberato has the final LGTM) Sorry for the delay :/ I am just starting to be added to code reviews (rather than only sending them out), and realize now that my email filters are too aggressive! I will update my workflow accordingly.
On 2016/10/20 20:17:30, tguilbert wrote: > On 2016/10/20 10:09:55, sivag wrote: > > @Thomas, liberato, > > > > Any further comments on this patch? > > LGTM (but I don't have committer rights just yet, so liberato has the final > LGTM) > > Sorry for the delay :/ I am just starting to be added to code reviews (rather > than only sending them out), and realize now that my email filters are too > aggressive! I will update my workflow accordingly. LGTM from the proper account this time.
On 2016/10/20 20:19:24, tguilbert wrote: > On 2016/10/20 20:17:30, tguilbert wrote: > > On 2016/10/20 10:09:55, sivag wrote: > > > @Thomas, liberato, > > > > > > Any further comments on this patch? > > > > LGTM (but I don't have committer rights just yet, so liberato has the final > > LGTM) > > > > Sorry for the delay :/ I am just starting to be added to code reviews (rather > > than only sending them out), and realize now that my email filters are too > > aggressive! I will update my workflow accordingly. > > LGTM from the proper account this time. lgtm
The CQ bit was checked by siva.gunturi@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org Link to the patchset: https://codereview.chromium.org/2390783003/#ps80001 (title: "Handle failure conditions of route_id.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Make stream_id internal to StreamTextureHost. 1.Move code in between StreamTextureFactory/Proxy and StreamTextureHost. 2.EstablishPeer and SetSize would go throughStreamTextureProxy instead, so outside of proxy/host nobody(WebMediaPlayer)needs to know the stream/route id). 3.Also rename stream_id to route_id. BUG=516585 ========== to ========== Make stream_id internal to StreamTextureHost. 1.Move code in between StreamTextureFactory/Proxy and StreamTextureHost. 2.EstablishPeer and SetSize would go throughStreamTextureProxy instead, so outside of proxy/host nobody(WebMediaPlayer)needs to know the stream/route id). 3.Also rename stream_id to route_id. BUG=516585, 339191 ==========
Message was sent while issue was closed.
Description was changed from ========== Make stream_id internal to StreamTextureHost. 1.Move code in between StreamTextureFactory/Proxy and StreamTextureHost. 2.EstablishPeer and SetSize would go throughStreamTextureProxy instead, so outside of proxy/host nobody(WebMediaPlayer)needs to know the stream/route id). 3.Also rename stream_id to route_id. BUG=516585, 339191 ========== to ========== Make stream_id internal to StreamTextureHost. 1.Move code in between StreamTextureFactory/Proxy and StreamTextureHost. 2.EstablishPeer and SetSize would go throughStreamTextureProxy instead, so outside of proxy/host nobody(WebMediaPlayer)needs to know the stream/route id). 3.Also rename stream_id to route_id. BUG=516585, 339191 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Make stream_id internal to StreamTextureHost. 1.Move code in between StreamTextureFactory/Proxy and StreamTextureHost. 2.EstablishPeer and SetSize would go throughStreamTextureProxy instead, so outside of proxy/host nobody(WebMediaPlayer)needs to know the stream/route id). 3.Also rename stream_id to route_id. BUG=516585, 339191 ========== to ========== Make stream_id internal to StreamTextureHost. 1.Move code in between StreamTextureFactory/Proxy and StreamTextureHost. 2.EstablishPeer and SetSize would go throughStreamTextureProxy instead, so outside of proxy/host nobody(WebMediaPlayer)needs to know the stream/route id). 3.Also rename stream_id to route_id. BUG=516585, 339191 Committed: https://crrev.com/6216f318b43cb2dbb784409c5be8122d1b4fd582 Cr-Commit-Position: refs/heads/master@{#426739} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6216f318b43cb2dbb784409c5be8122d1b4fd582 Cr-Commit-Position: refs/heads/master@{#426739} |
