Closed Bug 1138320 Opened 10 years ago Closed 10 years ago

Set screensharing mode or video mode for WebRTC video sources

Categories

(Core :: WebRTC, defect, P2)

Product:
Core
Component:
WebRTC
Platform:
x86
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla39
Tracking Flags:
Tracking Status
firefox39 --- fixed

People

(Reporter: ruil2, Assigned: ruil2)

Details

Attachments

(1 file, 10 obsolete files)

get video source type and set video codec mode( use size_t instead of "auto")
13.32 KB, patch
ruil2
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_2) AppleWebKit/600.3.18 (KHTML, like Gecko) Version/8.0.3 Safari/600.3.18 Steps to reproduce: share screen content in FF squared client Actual results: content type doesn't delivery to video codec. Expected results: content type should be transferred to video codec
Attached patch set_content_type.txt (obsolete) — — Splinter Review
Attachment #8571209 - Attachment is patch: true
Attachment #8571209 - Flags: review?(rjesup)
Comment on attachment 8571209 [details] [diff] [review] set_content_type.txt Review of attachment 8571209 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/DOMMediaStream.h @@ +121,5 @@ > virtual void StopTrack(TrackID aTrackID); > > virtual DOMLocalMediaStream* AsDOMLocalMediaStream() { return nullptr; } > > + virtual int GetMediaSourceType(TrackID aTrackID){ return -1; } not really an int @@ +347,5 @@ > > virtual void Stop(); > > virtual MediaEngineSource* GetMediaEngine(TrackID aTrackID) { return nullptr; } > + virtual int GetMediaSourceType(TrackID aTrackID){return -1;}; Not really an int ::: dom/media/MediaManager.cpp @@ +715,5 @@ > } > + virtual int GetMediaSourceType(TrackID aTrackID) MOZ_OVERRIDE > + { > + if (aTrackID == kVideoTrack) { > + return int(mVideoSource->GetMediaSource()); we shouldn't be using int... ::: dom/media/webrtc/MediaEngine.h @@ +154,5 @@ > void SetHasFakeTracks(bool aHasFakeTracks) { > mHasFakeTracks = aHasFakeTracks; > } > > +//protected: why? If there is a reason, it shouldn't just be commented out. And this exposes the internal vars too, so this looks like debugging ::: dom/media/webrtc/MediaEngineWebRTC.cpp @@ -134,5 @@ > LOG(("VieCapture:SetAndroidObjects Failed")); > return; > } > #endif > - spurious change ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp @@ +546,4 @@ > > + webrtc::VideoCodecMode mode = webrtc::kRealtimeVideo; > + if(type == 1) //screen > + mode = webrtc::kScreensharing; braces, space after if, don't use random ints as enums/bools @@ +600,5 @@ > video_codec.resolution_divisor = 1; // We could try using it to handle odd resolutions > #endif > video_codec.qpMax = 56; > video_codec.numberOfSimulcastStreams = 1; > + video_codec.mode = mCodecMode;//webrtc::kRealtimeVideo; remove comment ::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp @@ +750,5 @@ > > + nsRefPtr<LocalSourceStreamInfo> stream = > + mPCMedia->GetLocalStreamById(aTrack.GetStreamId()); > + int type = stream->GetMediaSourceType(aTrack.GetMediaType() == SdpMediaSection::kVideo); > + conduit->ConfigureCodecMode(type); This isn't an int... And screenshares may include audio in the future. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp @@ +1070,5 @@ > +int > +SourceStreamInfo::GetMediaSourceType(bool bVideo) > +{ > + int type = 0; > + int kTrackId = bVideo?1:2; This is going to run afoul of other changes surrounding trackids, now that we have more than one video or audio track per connection. Also: the 1/2 thing is *only* for tracks created by gUM. That's not an invariant across MSG ::: media/webrtc/trunk/webrtc/video_engine/vie_input_manager.cc @@ -359,5 @@ > > // Create different DeviceInfo by _config; > VideoCaptureModule::DeviceInfo* ViEInputManager::GetDeviceInfo() { > CaptureDeviceType type = config_.Get<CaptureDeviceInfo>().type; > - spurious change
Attachment #8571209 - Flags: review?(rjesup) → review-
FYI, you may want to look at the patch and how kVideoTrack is used (here, and currently in gUM). Per discussion with jib, this id is not exposed to content, and also gUM can only return one audio and 1 video track in a single call.
Comment on attachment 8571742 [details] updated patch according to the comments obsoleted
Attached patch update_type_setting.txt (obsolete) — — Splinter Review
Attachment #8571742 - Attachment is obsolete: true
Comment on attachment 8571749 [details] [diff] [review] update_type_setting.txt You need to check the patch checkbox for bugzilla to recognize them as patches (and make them reviewable). Or you could give them a .patch file ending.
Attachment #8571749 - Attachment is patch: true
Comment on attachment 8571749 [details] [diff] [review] update_type_setting.txt Review of attachment 8571749 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/DOMMediaStream.h @@ +52,5 @@ > // For classes that need fixed track IDs > enum { > kVideoTrack = 1, > kAudioTrack = 2, > kTrackCount This enum is being moved to MediaEngine.h in bug 1129263. MediaEngine/MediaManager will also be the only place where they are still used. Other classes might still have access through includes, but are not meant to use them. ::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp @@ +750,5 @@ > > + //get source type and set codec mode > + nsRefPtr<LocalSourceStreamInfo> stream = mPCMedia->GetLocalStreamById(aTrack.GetStreamId()); > + MediaSourceTypeEnum source = stream->GetMediaSourceType(true); > + conduit->ConfigureCodecMode((source == kScreenSource)? true:false); I think you'd be better off with stream->GetMediaStream()->AsDOMLocalMediaStream()->GetMediaEngine()->GetMediaSource() here. That way you can skip most of your helper methods. You could figure out the TrackID to give GetMediaEngine() through, say, DOMMediaStream::GetVideoTracks().
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #8) > Comment on attachment 8571749 [details] [diff] [review] > update_type_setting.txt > > Review of attachment 8571749 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/media/DOMMediaStream.h > @@ +52,5 @@ > > // For classes that need fixed track IDs > > enum { > > kVideoTrack = 1, > > kAudioTrack = 2, > > kTrackCount > > This enum is being moved to MediaEngine.h in bug 1129263. > MediaEngine/MediaManager will also be the only place where they are still > used. Other classes might still have access through includes, but are not > meant to use them. > > ::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp > @@ +750,5 @@ > > > > + //get source type and set codec mode > > + nsRefPtr<LocalSourceStreamInfo> stream = mPCMedia->GetLocalStreamById(aTrack.GetStreamId()); > > + MediaSourceTypeEnum source = stream->GetMediaSourceType(true); > > + conduit->ConfigureCodecMode((source == kScreenSource)? true:false); > > I think you'd be better off with > stream->GetMediaStream()->AsDOMLocalMediaStream()->GetMediaEngine()- > >GetMediaSource() here. > [karina] yes, at the beginning, I try to use this method to get the source type. But I can't success in compiling. > That way you can skip most of your helper methods. > > You could figure out the TrackID to give GetMediaEngine() through, say, > DOMMediaStream::GetVideoTracks().
(In reply to karina from comment #9) > [karina] yes, at the beginning, I try to use this method to get the source > type. But I can't success in compiling. What's the error?
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #10) > (In reply to karina from comment #9) > > [karina] yes, at the beginning, I try to use this method to get the source > > type. But I can't success in compiling. > > What's the error? /Project/firefox/media/webrtc/signaling/../../../xpcom/base/nsRefPtr.h:93:14: error: member access into incomplete type 'mozilla::MediaEngineSource' 0:19.74 mRawPtr->AddRef(); 0:19.74 ^ 0:19.74 /Project/firefox/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:1075:45: note: in instantiation of member function 'nsRefPtr<mozilla::MediaEngineSource>::nsRefPtr' requested here 0:19.74 nsRefPtr<MediaEngineSource>mediaEngine= domLocalStream->GetMediaEngine(kTrackId); 0:19.74 ^ 0:19.74 /Project/firefox/media/webrtc/signaling/../../../dom/media/DOMMediaStream.h:36:7: note: forward declaration of 'mozilla::MediaEngineSource' 0:19.74 class MediaEngineSource; 0:19.74 ^ 0:19.74 In file included from /Project/firefox/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:13: 0:19.74 In file included from /Project/firefox/media/webrtc/signaling/../../../media/mtransport/nricectx.h:63: 0:19.74 In file included from /Project/firefox/media/webrtc/signaling/../../../xpcom/base/nsAutoPtr.h:11: 0:19.74 /Project/firefox/media/webrtc/signaling/../../../xpcom/base/nsRefPtr.h:60:14: error: member access into incomplete type 'mozilla::MediaEngineSource' 0:19.74 mRawPtr->Release(); 0:19.74 ^ 0:19.74 /Project/firefox/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:1075:45: note: in instantiation of member function 'nsRefPtr<mozilla::MediaEngineSource>::~nsRefPtr' requested here 0:19.74 nsRefPtr<MediaEngineSource>mediaEngine= domLocalStream->GetMediaEngine(kTrackId); 0:19.74 ^ 0:19.74 /Project/firefox/media/webrtc/signaling/../../../dom/media/DOMMediaStream.h:36:7: note: forward declaration of 'mozilla::MediaEngineSource' 0:19.74 class MediaEngineSource; 0:19.74 ^
You can for instance #include "MediaEngine.h" in MediaPipelineFactory.cpp if you stay within the #ifdef MOZILLA_INTERNAL_API. Then you'll also have to put the GetMediaSource() inside a similar ifdef. I'm not sure how our reviewers feel about these ifdefs but that's how I'd start out.
Attachment #8571209 - Attachment is obsolete: true
Comment on attachment 8571749 [details] [diff] [review] update_type_setting.txt Review of attachment 8571749 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp @@ +542,5 @@ > +MediaConduitErrorCode > +WebrtcVideoConduit::ConfigureCodecMode(bool bScreen) > +{ > + CSFLogDebug(logTag, "%s ", __FUNCTION__); > + if(bScreen) Is Randell mentioned in the previous review "braces, space after if" ::: media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp @@ +198,5 @@ > mMaxPayloadSize = aMaxPayloadSize; > if (aCodecSettings->codecSpecific.H264.packetizationMode == 1) { > mMaxPayloadSize = 4*1024*1024; // insanely large > } > + if (aCodecSettings->mode == webrtc::kScreensharing) braces here as well.
Attachment #8571749 - Attachment is obsolete: true
Attachment #8572424 - Attachment is obsolete: true
any comments on this patch?thanks!
Attachment #8572437 - Flags: review?(rjesup)
Patch is bitrotten but only in a trivial way. Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9f86f13448f
Comment on attachment 8572437 [details] [diff] [review] get source type by using existed function(GetMediaSource) ( Review of attachment 8572437 [details] [diff] [review]: ----------------------------------------------------------------- Definitely closer, but still some questions/issues to resolve ::: dom/media/DOMMediaStream.h @@ +62,5 @@ > + kApplicationSource = 2, > + kWindowSource = 3, > + kBrowserSource = 4, > + kMicrophoneSource = 5, > + kUnknowSource = 6 See comments for FakeMediaStreams.h ALso, is this actually needed at all? ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp @@ +539,5 @@ > mReceiverTransport = aTransport; > return kMediaConduitNoError; > } > +MediaConduitErrorCode > +WebrtcVideoConduit::ConfigureCodecMode(bool bScreen) See VideoConduit.h @@ +541,5 @@ > } > +MediaConduitErrorCode > +WebrtcVideoConduit::ConfigureCodecMode(bool bScreen) > +{ > + CSFLogDebug(logTag, "%s ", __FUNCTION__); Add the parameter here ::: media/webrtc/signaling/src/media-conduit/VideoConduit.h @@ +101,5 @@ > > + /** > + * Function to configure sending codec mode for different content > + */ > + virtual MediaConduitErrorCode ConfigureCodecMode(bool bScreen) MOZ_OVERRIDE; bScreen is ambiguous, and also we don't use hungarian notation. aIsScreenShare or would be better ::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp @@ +750,5 @@ > > + //get source type and set codec mode > + nsRefPtr<LocalSourceStreamInfo> stream = mPCMedia->GetLocalStreamById(aTrack.GetStreamId()); > + MediaSourceTypeEnum source = stream->GetMediaSourceType(true); > + conduit->ConfigureCodecMode((source == kScreenSource)? true:false); nits: spaces around ? and : ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp @@ +1070,5 @@ > +MediaSourceTypeEnum > +SourceStreamInfo::GetMediaSourceType(bool bVideo) > +{ > +#ifdef MOZILLA_INTERNAL_API > + int kTrackId = bVideo?1:2; some comments as before about the hard-coded track ids @@ +1076,5 @@ > + if (domLocalStream) { > + MediaEngineSource *engine = domLocalStream->GetMediaEngine(kTrackId); > + if (engine) { > + dom::MediaSourceEnum source = engine->GetMediaSource(); > + switch (source) { Is there a reason to not return a dom::MediaSourceEnum here? @@ +1095,5 @@ > + } > + } > + } > +#endif > + return kUnknowSource; mis-spelled ::: media/webrtc/signaling/test/FakeMediaStreams.h @@ +38,5 @@ > + kApplicationSource = 2, > + kWindowSource = 3, > + kBrowserSource = 4, > + kMicrophoneSource = 5, > + kUnknowSource = 6 mis-spelled Don't spell out the values for an enum unless they matter for some reason. And don't spell them all out unless the exact values of each matters
Attachment #8572437 - Flags: review?(rjesup) → review-
Status: UNCONFIRMED → NEW
Component: Untriaged → WebRTC
Ever confirmed: true
Product: Firefox → Core
Rank: 25
Priority: -- → P3
Summary: transfer source type to gmp plugin → Set screensharing mode or video mode for WebRTC video sources
Attachment #8572437 - Attachment is obsolete: true
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp > @@ +1070,5 @@ > > +MediaSourceTypeEnum > > +SourceStreamInfo::GetMediaSourceType(bool bVideo) > > +{ > > +#ifdef MOZILLA_INTERNAL_API > > + int kTrackId = bVideo?1:2; > > some comments as before about the hard-coded track ids [karina]I don't find a good method instead of using hard code track ids. could you give more comments? > > @@ +1076,5 @@ > > + if (domLocalStream) { > > + MediaEngineSource *engine = domLocalStream->GetMediaEngine(kTrackId); > > + if (engine) { > > + dom::MediaSourceEnum source = engine->GetMediaSource(); > > + switch (source) { > > Is there a reason to not return a dom::MediaSourceEnum here? [karina] can't find dom::MediaSourceEnum in MediaPiplelineFactory.cpp. so I use a new enum. but in the latest patch, only return screen content flag and remove the new enum type. > @@ +1095,5 @@ > > + } > > + } > > + } > > +#endif > > + return kUnknowSource; > > mis-spelled > > ::: media/webrtc/signaling/test/FakeMediaStreams.h > @@ +38,5 @@ > > + kApplicationSource = 2, > > + kWindowSource = 3, > > + kBrowserSource = 4, > > + kMicrophoneSource = 5, > > + kUnknowSource = 6 > > mis-spelled > Don't spell out the values for an enum unless they matter for some reason. > And don't spell them all out unless the exact values of each matters [karina]remove the enum
(In reply to karina from comment #20) > [karina]I don't find a good method instead of using hard code track ids. > could you give more comments? See comment 8; i.e., DOMMediaStream::GetVideoTracks(). > > @@ +1076,5 @@ > > > + if (domLocalStream) { > > > + MediaEngineSource *engine = domLocalStream->GetMediaEngine(kTrackId); > > > + if (engine) { > > > + dom::MediaSourceEnum source = engine->GetMediaSource(); > > > + switch (source) { > > > > Is there a reason to not return a dom::MediaSourceEnum here? > [karina] can't find dom::MediaSourceEnum in MediaPiplelineFactory.cpp. so I > use a new enum. but in the latest patch, only return screen content flag and > remove the new enum type. It's here: https://dxr.mozilla.org/mozilla-central/source/dom/webidl/Constraints.webidl You can get it by doing `#include "MediaTrackConstraints.h"`.
Attachment #8574488 - Attachment is obsolete: true
Comment on attachment 8575032 [details] [diff] [review] Set screensharing mode or video mode for WebRTC video Review of attachment 8575032 [details] [diff] [review]: ----------------------------------------------------------------- The normal work flow is to set feedback? or review? on the patch in order to get comments. I'm just assuming you want comments now, so here are some :) ::: media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h @@ +297,5 @@ > unsigned short height, > VideoType video_type, > uint64_t capture_time) = 0; > > + virtual MediaConduitErrorCode ConfigureCodecMode(bool bIsScreenShare) = 0; IMO, make this take a VideoCodecMode directly instead of a bool. ::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp @@ +758,5 @@ > > + //get source type and set codec mode > + nsRefPtr<LocalSourceStreamInfo> stream = mPCMedia->GetLocalStreamById(aTrack.GetStreamId()); > + bool bIsScreenShare = stream->ScreenSharingContent(true); > + conduit->ConfigureCodecMode(bIsScreenShare); Inline the body of SourceStreamInfo::ScreenSharingContent here to get rid of that method altogether. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp @@ +1080,5 @@ > + if (engine) { > + dom::MediaSourceEnum source = engine->GetMediaSource(); > + if (source == dom::MediaSourceEnum::Screen) { > + return true; > + } You can avoid these nested ifs by using guarding ifs instead. I.e., (for your first if): if (!domLocalStream || videoTracks.IsEmpty()) { return false; } MediaEngineSource* engine = ... <- no longer indented, improves readability.
Attachment #8575032 - Attachment is obsolete: true
Attachment #8575082 - Flags: review+
Attachment #8575082 - Flags: feedback+
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #23) > Comment on attachment 8575032 [details] [diff] [review] > Set screensharing mode or video mode for WebRTC video > > Review of attachment 8575032 [details] [diff] [review]: > ----------------------------------------------------------------- > > The normal work flow is to set feedback? or review? on the patch in order to > get comments. I'm just assuming you want comments now, so here are some :) > [karina] thanks for your info. it is very helpful. > ::: media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h > @@ +297,5 @@ > > unsigned short height, > > VideoType video_type, > > uint64_t capture_time) = 0; > > > > + virtual MediaConduitErrorCode ConfigureCodecMode(bool bIsScreenShare) = 0; > > IMO, make this take a VideoCodecMode directly instead of a bool. > [karina] use VideoCodecMode in the new patch > ::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp > @@ +758,5 @@ > > > > + //get source type and set codec mode > > + nsRefPtr<LocalSourceStreamInfo> stream = mPCMedia->GetLocalStreamById(aTrack.GetStreamId()); > > + bool bIsScreenShare = stream->ScreenSharingContent(true); > > + conduit->ConfigureCodecMode(bIsScreenShare); > > Inline the body of SourceStreamInfo::ScreenSharingContent here to get rid of > that method altogether. > [karina] inline this. > ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp > @@ +1080,5 @@ > > + if (engine) { > > + dom::MediaSourceEnum source = engine->GetMediaSource(); > > + if (source == dom::MediaSourceEnum::Screen) { > > + return true; > > + } > > You can avoid these nested ifs by using guarding ifs instead. I.e., (for > your first if): > > if (!domLocalStream || videoTracks.IsEmpty()) { > return false; > } > > MediaEngineSource* engine = ... <- no longer indented, improves readability. [karina]it is difficult to avoid the nested ifs when inline this function.
Comment on attachment 8575082 [details] [diff] [review] Set screensharing mode or video mode for WebRTC video Only a (qualified) reviewer should set review+. And in this case, no one gave feedback+ either. ? is a request for a review. A patch creator should generally only set review+ when carrying-forward a previous review that had nits or when updating a patch with review+ for bitrot. To request review, set review to ? and select an appropriate reviewer. For this code, you can start with me.
Attachment #8575082 - Flags: review+
Attachment #8575082 - Flags: feedback+
Attachment #8575082 - Flags: review?(rjesup)
Comment on attachment 8575082 [details] [diff] [review] Set screensharing mode or video mode for WebRTC video Review of attachment 8575082 [details] [diff] [review]: ----------------------------------------------------------------- r- for the tracks[0] issue and only setting kScreensharing for full-screen sharing ::: media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h @@ +15,5 @@ > > #include "ImageContainer.h" > > #include <vector> > +#include "webrtc/common_types.h" don't remove the blank line ::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp @@ +766,5 @@ > + nsTArray<nsRefPtr<mozilla::dom::VideoStreamTrack>> videoTracks; > + > + DOMLocalMediaStream* domLocalStream = mediastream->AsDOMLocalMediaStream(); > + mediastream->GetVideoTracks(videoTracks); > + if (domLocalStream && !videoTracks.IsEmpty()) { DOMLocalMediaStream* domLocalStream = mediastream->AsDOMLocalMediaStream(); if (domLocalStream) { nsTArray<nsRefPtr<mozilla::dom::VideoStreamTrack>> videoTracks; mediastream->GetVideoTracks(videoTracks); if (!videoTracks.IsEmpty()) { @@ +767,5 @@ > + > + DOMLocalMediaStream* domLocalStream = mediastream->AsDOMLocalMediaStream(); > + mediastream->GetVideoTracks(videoTracks); > + if (domLocalStream && !videoTracks.IsEmpty()) { > + MediaEngineSource *engine = domLocalStream->GetMediaEngine(videoTracks[0]->GetTrackID()); You need to find the correct track, not the first track, using aTrack.GetTrackId() @@ +771,5 @@ > + MediaEngineSource *engine = domLocalStream->GetMediaEngine(videoTracks[0]->GetTrackID()); > + if (engine) { > + dom::MediaSourceEnum source = engine->GetMediaSource(); > + if (source == dom::MediaSourceEnum::Screen) { > + mode = webrtc::kScreensharing; This needs to set kScreensharing for anything except live video, or more directly, for screen/window/application/browser sources.
Attachment #8575082 - Flags: review?(rjesup) → review-
Attachment #8575082 - Attachment is obsolete: true
Attachment #8575729 - Flags: review?(rjesup)
Comment on attachment 8575729 [details] [diff] [review] get video source type and set video codec mode Review of attachment 8575729 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/media-conduit/MediaConduitInterface.h @@ +15,5 @@ > > #include "ImageContainer.h" > > #include <vector> > +#include "webrtc/common_types.h" move above <vector>, and blank line after ::: media/webrtc/signaling/src/peerconnection/MediaPipelineFactory.cpp @@ +758,5 @@ > return rv; > > + rv = ConfigureVideoCodecMode(aTrack,*conduit); > + if (NS_FAILED(rv)) > + return rv; braces @@ +813,5 @@ > + > + dom::MediaSourceEnum source = engine->GetMediaSource(); > + webrtc::VideoCodecMode mode = webrtc::kRealtimeVideo; > + switch (source) { > + case dom::MediaSourceEnum::Browser: indent case: two spaces (and indent code two more) ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp @@ +1076,5 @@ > + nsTArray<nsRefPtr<mozilla::dom::VideoStreamTrack>> videoTracks; > + > + mMediaStream->GetVideoTracks(videoTracks); > + if (videoTracks.IsEmpty()) > + return nullptr; braces @@ +1078,5 @@ > + mMediaStream->GetVideoTracks(videoTracks); > + if (videoTracks.IsEmpty()) > + return nullptr; > + > + for (uint32_t i = 0; i < videoTracks.Length(); ++i) { size_t, or auto with the new-style for() loops @@ +1081,5 @@ > + > + for (uint32_t i = 0; i < videoTracks.Length(); ++i) { > + nsString aTrackId; > + videoTracks[i]->GetId(aTrackId); > + if(aTrackId.EqualsIgnoreCase(trackId.c_str())){ spaces after if and )
Attachment #8575729 - Flags: review?(rjesup) → review+
Assignee: nobody → ruil2
Just to be clear Karina, please upload a new patch with just these 6 small changes. Mark it r+ with a comment that says "Bringing forward r+ from rjesup", and I will push it to Mozilla-Inbound for you. Thanks.
Bringing forward r+ from rjesup
Attachment #8575729 - Attachment is obsolete: true
Attachment #8576351 - Flags: review+
Looks like the use of auto causes this un/signed mismatch: /builds/slave/try-l64-d-00000000000000000000/build/src/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp:1087:43: error: comparison between signed and unsigned integer expressions [-Werror=sign-compare] From the Linux64 try failure.
You can check bug 1126552 for the latest looping coolness for nsTArray.
Comment on attachment 8576351 [details] [diff] [review] get video source type and set video codec mode Review of attachment 8576351 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp @@ +1077,5 @@ > + > + mMediaStream->GetVideoTracks(videoTracks); > + if (videoTracks.IsEmpty()) { > + return nullptr; > + } Also, this IsEmpty() check is not needed.
(In reply to Ethan Hugg [:ehugg] from comment #33) > Looks like the use of auto causes this un/signed mismatch: Aha. My comment was mis-understood. It should be *either* for (size_t i = 0; (etc), or for (auto& track : videoTracks) { ... track.GetID(aTrackId); ... That's the new c++11 hotness referred to by Andreas
(In reply to Randell Jesup [:jesup] from comment #36) > (In reply to Ethan Hugg [:ehugg] from comment #33) > > Looks like the use of auto causes this un/signed mismatch: > > Aha. My comment was mis-understood. It should be *either* for (size_t i = > 0; (etc), or > for (auto& track : videoTracks) { > ... > track.GetID(aTrackId); > ... > > That's the new c++11 hotness referred to by Andreas I prefer to have the type spelled out. But yes, that's the one :)
Bringing forward r+ from rjesup
Attachment #8576351 - Attachment is obsolete: true
Attachment #8576458 - Flags: review+
Flags: firefox-backlog+
Priority: P3 → P2
https://hg.mozilla.org/mozilla-central/rev/b13605555872
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: