Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
166753002:
Roll WebRTC 5523:5548. (Closed)
|
Created:
6 years, 10 months ago by no longer working on chromium Modified:
6 years, 10 months ago Reviewers:
Ronghua Wu (Left Chromium), Henrik Grunell, Mallinath (Gone from Chromium), tommi (sloooow) - chröme CC:
chromium-reviews, chromoting-reviews_chromium.org, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionRoll WebRTC 5523:5548.
TEST=bots
Patch Set 1 #Patch Set 2 : #Patch Set 3 : removed webrtc:: from StatsOutputLevel #
Total comments: 1
Patch Set 4 : fixed the bots #
Total comments: 7
Messages
Total messages: 16 (0 generated)
Please review.
lgtm
https://codereview.chromium.org/166753002/diff/220001/DEPS File DEPS (right): https://codereview.chromium.org/166753002/diff/220001/DEPS#newcode58 DEPS:58: #"webrtc_revision": "5521", should we remove this line?
https://codereview.chromium.org/166753002/diff/260002/DEPS File DEPS (right): https://codereview.chromium.org/166753002/diff/260002/DEPS#newcode58 DEPS:58: #"webrtc_revision": "5521", nit: remove this line https://codereview.chromium.org/166753002/diff/260002/content/browser/media/w... File content/browser/media/webrtc_browsertest.cc (right): https://codereview.chromium.org/166753002/diff/260002/content/browser/media/w... content/browser/media/webrtc_browsertest.cc:102: "callAndForwardRemoteStream({video: true, audio: false});"); why this change?
lgtm. thanks for the [offline] explanation.
lgtm https://codereview.chromium.org/166753002/diff/260002/content/browser/media/w... File content/browser/media/webrtc_browsertest.cc (right): https://codereview.chromium.org/166753002/diff/260002/content/browser/media/w... content/browser/media/webrtc_browsertest.cc:102: "callAndForwardRemoteStream({video: true, audio: false});"); On 2014/02/14 15:33:14, tommi wrote: > why this change? I'd like to know as well. Looks like you are working on remote audio forwarding? https://codereview.chromium.org/166753002/diff/260002/content/renderer/media/... File content/renderer/media/mock_peer_connection_impl.h (right): https://codereview.chromium.org/166753002/diff/260002/content/renderer/media/... content/renderer/media/mock_peer_connection_impl.h:46: return false; NOTIMPLEMENTED(); https://codereview.chromium.org/166753002/diff/260002/content/renderer/p2p/ip... File content/renderer/p2p/ipc_socket_factory.cc (right): https://codereview.chromium.org/166753002/diff/260002/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:54: // NOTREACHED(); looks like there's a new OPT_RTP_SENDTIME_EXTN_ID.
https://codereview.chromium.org/166753002/diff/260002/content/browser/media/w... File content/browser/media/webrtc_browsertest.cc (right): https://codereview.chromium.org/166753002/diff/260002/content/browser/media/w... content/browser/media/webrtc_browsertest.cc:102: "callAndForwardRemoteStream({video: true, audio: false});"); On 2014/02/14 16:37:00, Ronghua Wu wrote: > On 2014/02/14 15:33:14, tommi wrote: > > why this change? > > I'd like to know as well. Looks like you are working on remote audio forwarding? Shijing explained this to me offline. The reason is that audio tracks will now have a valid source object, whereas they didn't before. This has the effect that this test tries to forward remote audio tracks, which still isn't supported.
https://codereview.chromium.org/166753002/diff/260002/content/renderer/p2p/ip... File content/renderer/p2p/ipc_socket_factory.cc (right): https://codereview.chromium.org/166753002/diff/260002/content/renderer/p2p/ip... content/renderer/p2p/ipc_socket_factory.cc:54: // NOTREACHED(); Add OPT_RTP_SENDTIME_EXTN_ID below line 50. On 2014/02/14 16:37:00, Ronghua Wu wrote: > looks like there's a new OPT_RTP_SENDTIME_EXTN_ID.
On Fri, Feb 14, 2014 at 9:05 AM, <tommi@chromium.org> wrote: > > https://codereview.chromium.org/166753002/diff/260002/ > content/browser/media/webrtc_browsertest.cc > File content/browser/media/webrtc_browsertest.cc (right): > > https://codereview.chromium.org/166753002/diff/260002/ > content/browser/media/webrtc_browsertest.cc#newcode102 > content/browser/media/webrtc_browsertest.cc:102: > "callAndForwardRemoteStream({video: true, audio: false});"); > On 2014/02/14 16:37:00, Ronghua Wu wrote: > >> On 2014/02/14 15:33:14, tommi wrote: >> > why this change? >> > > I'd like to know as well. Looks like you are working on remote audio >> > forwarding? > > Shijing explained this to me offline. The reason is that audio tracks > will now have a valid source object, whereas they didn't before. This > has the effect that this test tries to forward remote audio tracks, > which still isn't supported. My concern is, what will happen if some developers do this? Will we end up a crash or just audio doesn't work? > > https://codereview.chromium.org/166753002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Good question, I don't have an answer to that but might be able to test over the weekend. On Fri, Feb 14, 2014 at 6:31 PM, Ronghua Wu <ronghuawu@chromium.org> wrote: > > > > On Fri, Feb 14, 2014 at 9:05 AM, <tommi@chromium.org> wrote: > >> >> https://codereview.chromium.org/166753002/diff/260002/ >> content/browser/media/webrtc_browsertest.cc >> File content/browser/media/webrtc_browsertest.cc (right): >> >> https://codereview.chromium.org/166753002/diff/260002/ >> content/browser/media/webrtc_browsertest.cc#newcode102 >> content/browser/media/webrtc_browsertest.cc:102: >> "callAndForwardRemoteStream({video: true, audio: false});"); >> On 2014/02/14 16:37:00, Ronghua Wu wrote: >> >>> On 2014/02/14 15:33:14, tommi wrote: >>> > why this change? >>> >> >> I'd like to know as well. Looks like you are working on remote audio >>> >> forwarding? >> >> Shijing explained this to me offline. The reason is that audio tracks >> will now have a valid source object, whereas they didn't before. This >> has the effect that this test tries to forward remote audio tracks, >> which still isn't supported. > > My concern is, what will happen if some developers do this? Will we end > up a crash or just audio doesn't work? > >> >> https://codereview.chromium.org/166753002/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I did not know the answe either since the content browser test just timed out. But even if it crashes the tag, probably it should not block us rolling the cl, we can easily make a fix in Chrome to prevent connecting the remote stream to pc. Wdyt? On Feb 14, 2014 6:36 PM, "Tommi" <tommi@chromium.org> wrote: > Good question, I don't have an answer to that but might be able to test > over the weekend. > > > On Fri, Feb 14, 2014 at 6:31 PM, Ronghua Wu <ronghuawu@chromium.org>wrote: > >> >> >> >> On Fri, Feb 14, 2014 at 9:05 AM, <tommi@chromium.org> wrote: >> >>> >>> https://codereview.chromium.org/166753002/diff/260002/ >>> content/browser/media/webrtc_browsertest.cc >>> File content/browser/media/webrtc_browsertest.cc (right): >>> >>> https://codereview.chromium.org/166753002/diff/260002/ >>> content/browser/media/webrtc_browsertest.cc#newcode102 >>> content/browser/media/webrtc_browsertest.cc:102: >>> "callAndForwardRemoteStream({video: true, audio: false});"); >>> On 2014/02/14 16:37:00, Ronghua Wu wrote: >>> >>>> On 2014/02/14 15:33:14, tommi wrote: >>>> > why this change? >>>> >>> >>> I'd like to know as well. Looks like you are working on remote audio >>>> >>> forwarding? >>> >>> Shijing explained this to me offline. The reason is that audio tracks >>> will now have a valid source object, whereas they didn't before. This >>> has the effect that this test tries to forward remote audio tracks, >>> which still isn't supported. >> >> My concern is, what will happen if some developers do this? Will we end >> up a crash or just audio doesn't work? >> >>> >>> https://codereview.chromium.org/166753002/ >>> >> >> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Fri, Feb 14, 2014 at 9:57 AM, Shijing Xian <xians@chromium.org> wrote: > > I did not know the answe either since the content browser test just timed > out. > > But even if it crashes the tag, probably it should not block us rolling > the cl, we can easily make a fix in Chrome to prevent connecting the remote > stream to pc. > > Wdyt? > I think there're developers using the remote video track forwarding, we can't just disable that. Or you meant you can make a fix to filter out audio? If so we should do it in this cl and revert the change to test. > On Feb 14, 2014 6:36 PM, "Tommi" <tommi@chromium.org> wrote: > >> Good question, I don't have an answer to that but might be able to test >> over the weekend. >> >> >> On Fri, Feb 14, 2014 at 6:31 PM, Ronghua Wu <ronghuawu@chromium.org>wrote: >> >>> >>> >>> >>> On Fri, Feb 14, 2014 at 9:05 AM, <tommi@chromium.org> wrote: >>> >>>> >>>> https://codereview.chromium.org/166753002/diff/260002/ >>>> content/browser/media/webrtc_browsertest.cc >>>> File content/browser/media/webrtc_browsertest.cc (right): >>>> >>>> https://codereview.chromium.org/166753002/diff/260002/ >>>> content/browser/media/webrtc_browsertest.cc#newcode102 >>>> content/browser/media/webrtc_browsertest.cc:102: >>>> "callAndForwardRemoteStream({video: true, audio: false});"); >>>> On 2014/02/14 16:37:00, Ronghua Wu wrote: >>>> >>>>> On 2014/02/14 15:33:14, tommi wrote: >>>>> > why this change? >>>>> >>>> >>>> I'd like to know as well. Looks like you are working on remote audio >>>>> >>>> forwarding? >>>> >>>> Shijing explained this to me offline. The reason is that audio tracks >>>> will now have a valid source object, whereas they didn't before. This >>>> has the effect that this test tries to forward remote audio tracks, >>>> which still isn't supported. >>> >>> My concern is, what will happen if some developers do this? Will we end >>> up a crash or just audio doesn't work? >>> >>>> >>>> https://codereview.chromium.org/166753002/ >>>> >>> >>> >> To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
This is only for audio right? On Feb 14, 2014 7:04 PM, "Ronghua Wu" <ronghuawu@chromium.org> wrote: > > > > On Fri, Feb 14, 2014 at 9:57 AM, Shijing Xian <xians@chromium.org> wrote: > >> >> I did not know the answe either since the content browser test just >> timed out. >> >> But even if it crashes the tag, probably it should not block us rolling >> the cl, we can easily make a fix in Chrome to prevent connecting the remote >> stream to pc. >> >> Wdyt? >> > I think there're developers using the remote video track forwarding, we > can't just disable that. Or you meant you can make a fix to filter out > audio? If so we should do it in this cl and revert the change to test. > >> On Feb 14, 2014 6:36 PM, "Tommi" <tommi@chromium.org> wrote: >> >>> Good question, I don't have an answer to that but might be able to test >>> over the weekend. >>> >>> >>> On Fri, Feb 14, 2014 at 6:31 PM, Ronghua Wu <ronghuawu@chromium.org>wrote: >>> >>>> >>>> >>>> >>>> On Fri, Feb 14, 2014 at 9:05 AM, <tommi@chromium.org> wrote: >>>> >>>>> >>>>> https://codereview.chromium.org/166753002/diff/260002/ >>>>> content/browser/media/webrtc_browsertest.cc >>>>> File content/browser/media/webrtc_browsertest.cc (right): >>>>> >>>>> https://codereview.chromium.org/166753002/diff/260002/ >>>>> content/browser/media/webrtc_browsertest.cc#newcode102 >>>>> content/browser/media/webrtc_browsertest.cc:102: >>>>> "callAndForwardRemoteStream({video: true, audio: false});"); >>>>> On 2014/02/14 16:37:00, Ronghua Wu wrote: >>>>> >>>>>> On 2014/02/14 15:33:14, tommi wrote: >>>>>> > why this change? >>>>>> >>>>> >>>>> I'd like to know as well. Looks like you are working on remote audio >>>>>> >>>>> forwarding? >>>>> >>>>> Shijing explained this to me offline. The reason is that audio tracks >>>>> will now have a valid source object, whereas they didn't before. This >>>>> has the effect that this test tries to forward remote audio tracks, >>>>> which still isn't supported. >>>> >>>> My concern is, what will happen if some developers do this? Will we >>>> end up a crash or just audio doesn't work? >>>> >>>>> >>>>> https://codereview.chromium.org/166753002/ >>>>> >>>> >>>> >>> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Fri, Feb 14, 2014 at 10:25 AM, Tommi <tommi@chromium.org> wrote: > This is only for audio right? > Sorry, not sure about the question. Can you explain? I tested the content_browser_test locally: 1) It seems be a timeout of the test code not chrome. 2) Having audio track in a stream will break video track forwarding, but I can't tell why. If we commit this, there might be developers who use it this way got breakage. We can ask them to walkaround this by removing the audio track from the stream. But obvious that's not the best. Before the change: RemoteStream(a,v) -> v RemoteStream(v) -> v After the change: RemoteStream(a, v) -> nothing RemoteStream(v) -> v Tommi, wdyt? Should we go for it? > On Feb 14, 2014 7:04 PM, "Ronghua Wu" <ronghuawu@chromium.org> wrote: > >> >> >> >> On Fri, Feb 14, 2014 at 9:57 AM, Shijing Xian <xians@chromium.org> wrote: >> >>> >>> I did not know the answe either since the content browser test just >>> timed out. >>> >>> But even if it crashes the tag, probably it should not block us rolling >>> the cl, we can easily make a fix in Chrome to prevent connecting the remote >>> stream to pc. >>> >>> Wdyt? >>> >> I think there're developers using the remote video track forwarding, we >> can't just disable that. Or you meant you can make a fix to filter out >> audio? If so we should do it in this cl and revert the change to test. >> >>> On Feb 14, 2014 6:36 PM, "Tommi" <tommi@chromium.org> wrote: >>> >>>> Good question, I don't have an answer to that but might be able to test >>>> over the weekend. >>>> >>>> >>>> On Fri, Feb 14, 2014 at 6:31 PM, Ronghua Wu <ronghuawu@chromium.org>wrote: >>>> >>>>> >>>>> >>>>> >>>>> On Fri, Feb 14, 2014 at 9:05 AM, <tommi@chromium.org> wrote: >>>>> >>>>>> >>>>>> https://codereview.chromium.org/166753002/diff/260002/ >>>>>> content/browser/media/webrtc_browsertest.cc >>>>>> File content/browser/media/webrtc_browsertest.cc (right): >>>>>> >>>>>> https://codereview.chromium.org/166753002/diff/260002/ >>>>>> content/browser/media/webrtc_browsertest.cc#newcode102 >>>>>> content/browser/media/webrtc_browsertest.cc:102: >>>>>> "callAndForwardRemoteStream({video: true, audio: false});"); >>>>>> On 2014/02/14 16:37:00, Ronghua Wu wrote: >>>>>> >>>>>>> On 2014/02/14 15:33:14, tommi wrote: >>>>>>> > why this change? >>>>>>> >>>>>> >>>>>> I'd like to know as well. Looks like you are working on remote audio >>>>>>> >>>>>> forwarding? >>>>>> >>>>>> Shijing explained this to me offline. The reason is that audio tracks >>>>>> will now have a valid source object, whereas they didn't before. This >>>>>> has the effect that this test tries to forward remote audio tracks, >>>>>> which still isn't supported. >>>>> >>>>> My concern is, what will happen if some developers do this? Will we >>>>> end up a crash or just audio doesn't work? >>>>> >>>>>> >>>>>> https://codereview.chromium.org/166753002/ >>>>>> >>>>> >>>>> >>>> >> To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I think so since it doesn't work as advertised. We should file a bug to fix this though. On Feb 14, 2014 7:42 PM, "Ronghua Wu" <ronghuawu@chromium.org> wrote: > > > > On Fri, Feb 14, 2014 at 10:25 AM, Tommi <tommi@chromium.org> wrote: > >> This is only for audio right? >> > Sorry, not sure about the question. Can you explain? > > > I tested the content_browser_test locally: > 1) It seems be a timeout of the test code not chrome. > 2) Having audio track in a stream will break video track forwarding, but I > can't tell why. > > If we commit this, there might be developers who use it this way got > breakage. We can ask them to walkaround this by removing the audio track > from the stream. But obvious that's not the best. > > Before the change: > RemoteStream(a,v) -> v > RemoteStream(v) -> v > > After the change: > RemoteStream(a, v) -> nothing > RemoteStream(v) -> v > > Tommi, wdyt? Should we go for it? > > >> On Feb 14, 2014 7:04 PM, "Ronghua Wu" <ronghuawu@chromium.org> wrote: >> >>> >>> >>> >>> On Fri, Feb 14, 2014 at 9:57 AM, Shijing Xian <xians@chromium.org>wrote: >>> >>>> >>>> I did not know the answe either since the content browser test just >>>> timed out. >>>> >>>> But even if it crashes the tag, probably it should not block us rolling >>>> the cl, we can easily make a fix in Chrome to prevent connecting the remote >>>> stream to pc. >>>> >>>> Wdyt? >>>> >>> I think there're developers using the remote video track forwarding, we >>> can't just disable that. Or you meant you can make a fix to filter out >>> audio? If so we should do it in this cl and revert the change to test. >>> >>>> On Feb 14, 2014 6:36 PM, "Tommi" <tommi@chromium.org> wrote: >>>> >>>>> Good question, I don't have an answer to that but might be able to >>>>> test over the weekend. >>>>> >>>>> >>>>> On Fri, Feb 14, 2014 at 6:31 PM, Ronghua Wu <ronghuawu@chromium.org>wrote: >>>>> >>>>>> >>>>>> >>>>>> >>>>>> On Fri, Feb 14, 2014 at 9:05 AM, <tommi@chromium.org> wrote: >>>>>> >>>>>>> >>>>>>> https://codereview.chromium.org/166753002/diff/260002/ >>>>>>> content/browser/media/webrtc_browsertest.cc >>>>>>> File content/browser/media/webrtc_browsertest.cc (right): >>>>>>> >>>>>>> https://codereview.chromium.org/166753002/diff/260002/ >>>>>>> content/browser/media/webrtc_browsertest.cc#newcode102 >>>>>>> content/browser/media/webrtc_browsertest.cc:102: >>>>>>> "callAndForwardRemoteStream({video: true, audio: false});"); >>>>>>> On 2014/02/14 16:37:00, Ronghua Wu wrote: >>>>>>> >>>>>>>> On 2014/02/14 15:33:14, tommi wrote: >>>>>>>> > why this change? >>>>>>>> >>>>>>> >>>>>>> I'd like to know as well. Looks like you are working on remote audio >>>>>>>> >>>>>>> forwarding? >>>>>>> >>>>>>> Shijing explained this to me offline. The reason is that audio tracks >>>>>>> will now have a valid source object, whereas they didn't before. >>>>>>> This >>>>>>> has the effect that this test tries to forward remote audio tracks, >>>>>>> which still isn't supported. >>>>>> >>>>>> My concern is, what will happen if some developers do this? Will we >>>>>> end up a crash or just audio doesn't work? >>>>>> >>>>>>> >>>>>>> https://codereview.chromium.org/166753002/ >>>>>>> >>>>>> >>>>>> >>>>> >>> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Fri, Feb 14, 2014 at 7:42 PM, Ronghua Wu <ronghuawu@chromium.org> wrote: > > > > On Fri, Feb 14, 2014 at 10:25 AM, Tommi <tommi@chromium.org> wrote: > >> This is only for audio right? >> > Sorry, not sure about the question. Can you explain? > > > I tested the content_browser_test locally: > 1) It seems be a timeout of the test code not chrome. > 2) Having audio track in a stream will break video track forwarding, but I > can't tell why. > > If a stream has both audio and video, what is expected if addStream fails on one of track? > If we commit this, there might be developers who use it this way got > breakage. We can ask them to walkaround this by removing the audio track > from the stream. But obvious that's not the best. > > Before the change: > RemoteStream(a,v) -> v > RemoteStream(v) -> v > > After the change: > RemoteStream(a, v) -> nothing > RemoteStream(v) -> v > > Tommi, wdyt? Should we go for it? > > > >> On Feb 14, 2014 7:04 PM, "Ronghua Wu" <ronghuawu@chromium.org> wrote: >> >>> >>> >>> >>> On Fri, Feb 14, 2014 at 9:57 AM, Shijing Xian <xians@chromium.org>wrote: >>> >>>> >>>> I did not know the answe either since the content browser test just >>>> timed out. >>>> >>>> But even if it crashes the tag, probably it should not block us rolling >>>> the cl, we can easily make a fix in Chrome to prevent connecting the remote >>>> stream to pc. >>>> >>>> Wdyt? >>>> >>> I think there're developers using the remote video track forwarding, we >>> can't just disable that. Or you meant you can make a fix to filter out >>> audio? If so we should do it in this cl and revert the change to test. >>> >>>> On Feb 14, 2014 6:36 PM, "Tommi" <tommi@chromium.org> wrote: >>>> >>>>> Good question, I don't have an answer to that but might be able to >>>>> test over the weekend. >>>>> >>>>> >>>>> On Fri, Feb 14, 2014 at 6:31 PM, Ronghua Wu <ronghuawu@chromium.org>wrote: >>>>> >>>>>> >>>>>> >>>>>> >>>>>> On Fri, Feb 14, 2014 at 9:05 AM, <tommi@chromium.org> wrote: >>>>>> >>>>>>> >>>>>>> https://codereview.chromium.org/166753002/diff/260002/ >>>>>>> content/browser/media/webrtc_browsertest.cc >>>>>>> File content/browser/media/webrtc_browsertest.cc (right): >>>>>>> >>>>>>> https://codereview.chromium.org/166753002/diff/260002/ >>>>>>> content/browser/media/webrtc_browsertest.cc#newcode102 >>>>>>> content/browser/media/webrtc_browsertest.cc:102: >>>>>>> "callAndForwardRemoteStream({video: true, audio: false});"); >>>>>>> On 2014/02/14 16:37:00, Ronghua Wu wrote: >>>>>>> >>>>>>>> On 2014/02/14 15:33:14, tommi wrote: >>>>>>>> > why this change? >>>>>>>> >>>>>>> >>>>>>> I'd like to know as well. Looks like you are working on remote audio >>>>>>>> >>>>>>> forwarding? >>>>>>> >>>>>>> Shijing explained this to me offline. The reason is that audio tracks >>>>>>> will now have a valid source object, whereas they didn't before. >>>>>>> This >>>>>>> has the effect that this test tries to forward remote audio tracks, >>>>>>> which still isn't supported. >>>>>> >>>>>> My concern is, what will happen if some developers do this? Will we >>>>>> end up a crash or just audio doesn't work? >>>>>> >>>>>>> >>>>>>> https://codereview.chromium.org/166753002/ >>>>>>> >>>>>> >>>>>> >>>>> >>> > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
