These were disabled as part of Bug 1250356.

I'm not sure there's a pressing need to enable this test before 57 lands. Please feel free to cancel review if you'd prefer that I wait. Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=31da544c72b108f0bc036ee015a39fc330f5498b

Comment on attachment 8855380 [details] Bug 1328169 - Enable mediaconduit_unittests again; https://reviewboard.mozilla.org/r/127196/#review143548 Let's wait... the 57 changes will break this in lots of ways

Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a360f92067e7602d341531061d8841588fa0d36c

Comment on attachment 8884811 [details] [diff] [review] Enable mediaconduit_unittests Review of attachment 8884811 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp @@ +1286,5 @@ > configuredH264 = true; > } > > + // webrtc.org does not want to see multiple encoders for the same type > + if (codec_types_seen.find(codec_config->mType) != codec_types_seen.end()) { First, we're in configureRecv, not Send - so decoders, not encoders. Second, this sounds like a separate bug (though they could land together). Third, what about cases like H264 where you can have multiple H264 payload values in SDP, differentiated by fmtp data (packetization-mode, profile-level-id, etc)? What does the webrtc.org code do with that? @@ +1651,4 @@ > unsigned short height, > webrtc::VideoFrame* frame) // may be null > { > + //mCodecMutex.AssertCurrentThreadOwns(); Either remove it, or comment why it should remain here commented out @@ +1785,4 @@ > unsigned short height, > webrtc::VideoFrame* frame) > { > + //mCodecMutex.AssertCurrentThreadOwns(); ditto @@ +2400,5 @@ > + unsigned short &result_width, > + unsigned short &result_height) > +{ > + MutexAutoLock lock(mCodecMutex); > + SelectSendResolution(frame_width, frame_height, nullptr); Note: this is somewhat inconsistent with a "GetFoo()" API expectation - this changes the sending resolution. At minimum, it should be renamed Set...(). Even more complicating: it can fail.

(In reply to Randell Jesup [:jesup] from comment #6) > Comment on attachment 8884811 [details] [diff] [review] > Enable mediaconduit_unittests > > Review of attachment 8884811 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp > @@ +1286,5 @@ > > configuredH264 = true; > > } > > > > + // webrtc.org does not want to see multiple encoders for the same type > > + if (codec_types_seen.find(codec_config->mType) != codec_types_seen.end()) { > > First, we're in configureRecv, not Send - so decoders, not encoders. > Second, this sounds like a separate bug (though they could land together). > Third, what about cases like H264 where you can have multiple H264 payload > values in SDP, differentiated by fmtp data (packetization-mode, > profile-level-id, etc)? What does the webrtc.org code do with that? > Good point. I think it makes more sense to disable the relevant test code for now rather than changing the behaviour of the code under test, as the test is quite possibly testing old expectations of the webrtc.org code. With the current code, different codecs for the same payload type will trigger an assertion in video_receive_stream.cc. Please let me know if I should file a follow on bug to prevent that situation from happening. > @@ +1651,4 @@ > > unsigned short height, > > webrtc::VideoFrame* frame) // may be null > > { > > + //mCodecMutex.AssertCurrentThreadOwns(); > > Either remove it, or comment why it should remain here commented out > > @@ +1785,4 @@ > > unsigned short height, > > webrtc::VideoFrame* frame) > > { > > + //mCodecMutex.AssertCurrentThreadOwns(); > > ditto > Sorry, these were commented to see if SelectSendResolution could work outside of SendVideoFrame and I missed reverting the changes. > @@ +2400,5 @@ > > + unsigned short &result_width, > > + unsigned short &result_height) > > +{ > > + MutexAutoLock lock(mCodecMutex); > > + SelectSendResolution(frame_width, frame_height, nullptr); > > Note: this is somewhat inconsistent with a "GetFoo()" API expectation - this > changes the sending resolution. At minimum, it should be renamed Set...(). > > Even more complicating: it can fail. I'll rename this to SetSendingWidthAndHeight. I'll also initialize the result_width and result_height to zero to ensure we catch failures in the tests. (Confusingly, at least to me, the bool returned by SelectSendResolution indicates whether or not it has taken over ownership of the frame, not success or failure).

Comment on attachment 8885368 [details] [diff] [review] Enable mediaconduit_unittests Review of attachment 8885368 [details] [diff] [review]: ----------------------------------------------------------------- ::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp @@ +2405,5 @@ > +{ > + MutexAutoLock lock(mCodecMutex); > + result_width = 0; > + result_height = 0; > + SelectSendResolution(frame_width, frame_height, nullptr); perhaps (void) or Unused << Not important

Pushed by dminor@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/301c6426fe5a Enable mediaconduit_unittests again; r=jesup