Closed Bug 1039666 Opened 10 years ago Closed 10 years ago

Basic tests for Desktop & Window Screensharing

Categories

(Core :: WebRTC: Audio/Video, defect, P1)

Product:
Core
Component:
WebRTC: Audio/Video
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla34
Tracking Flags:
Tracking Status
firefox33 --- fixed
firefox34 --- fixed

People

(Reporter: jesup, Assigned: jesup)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 4 obsolete files)

basic tests for desktop screensharing
(deleted), patch
drno
: review+
Details | Diff | Splinter Review
Enable Screen/windowsharing tests for all but OSX 10.6 and XP
(deleted), patch
drno
: review+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review
Disable screen/windowsharing for OSX 10.6 and WinXP
(deleted), patch
cpearce
: review+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review
suppress 'nonqueued' IPC assertions (bug 1047842) in window/screensharing r=
(deleted), patch
jesup
: review+
Sylvestre
: approval-mozilla-aurora+
Details | Diff | Splinter Review 
This bug is for basic tests for desktop screensharing; likely we'll want more complex ones later.

This is not for the UI per se
Attached patch basic tests for desktop screensharing (obsolete) (deleted) — — Splinter Review 
WIP but seems to work in local tests
Attachment #8457130 - Flags: feedback?(drno)
Comment on attachment 8457130 [details] [diff] [review]
basic tests for desktop screensharing

Review of attachment 8457130 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good as a very basic smoke test.

We should have more detailed tests (in separate BZ ticket depending on the feature bugs):
- switch from camera to screenshare
- switch from screenshare back to camera
- add a screenshare video stream to an existing PC
- remove a screenshare video stream from an existing PC
- screenshare and camera video in two separate PCs
- test the different constraints for desktop, application, browser and tab

::: dom/media/tests/mochitest/mochitest.ini
@@ +24,5 @@
>  [test_getUserMedia_basicAudio.html]
>  skip-if = (toolkit == 'gonk' && debug) # debug-only failure
>  [test_getUserMedia_basicVideo.html]
> +skip-if = (toolkit == 'gonk') # no screenshare on b2g
> +[test_getUserMedia_basicScreenshare.html]

skip-if need to under the new test case [] line
And we need to skip this on Android as well, or?

@@ +57,5 @@
>  skip-if = toolkit == 'gonk' # b2g(Bug 960442, video support for WebRTC is disabled on b2g)
>  [test_peerConnection_basicVideo.html]
>  skip-if = toolkit == 'gonk' # b2g(Bug 960442, video support for WebRTC is disabled on b2g)
> +[test_peerConnection_basicScreenshare.html]
> +skip-if = toolkit == 'gonk' # b2g(Bug 960442, video support for WebRTC is disabled on b2g)

Please remove or update the comment.
Again I think we need to skip Android as well.

::: dom/media/tests/mochitest/test_getUserMedia_basicScreenshare.html
@@ +39,5 @@
> +    getUserMedia(constraints, function (aStream) {
> +      checkMediaStreamTracks(constraints, aStream);
> +
> +      var playback = new LocalMediaStreamPlayback(testVideo, aStream);
> +      playback.playMedia(false, function () {

Looks like this actually internally stops the playback already:

http://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/mediaStreamPlayback.js#43

which calls this:

http://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/mediaStreamPlayback.js#160

which calls .pause()

Maybe it makes more sense to call playMediaWithStreamStop():
http://dxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/mediaStreamPlayback.js#196
instead?

::: dom/media/tests/mochitest/test_peerConnection_basicScreenshare.html
@@ +13,5 @@
> +<pre id="test">
> +<script type="application/javascript">
> +  createHTML({
> +    bug: "796888",
> +    title: "Basic video-only peer connection"

Please update bug number and description.
Attachment #8457130 - Flags: feedback?(drno) → feedback+
Attached patch basic tests for desktop screensharing (obsolete) (deleted) — — Splinter Review 

        Attachment #8457130 -
        Attachment is obsolete: true

        Attachment #8457781 -
        Flags: review?(drno)
Blocks: 1040061

        Attachment #8457781 -
        Attachment is obsolete: true

        Attachment #8457781 -
        Flags: review?(drno)

    
    

    
  
Comment on attachment 8458061 [details] [diff] [review]
basic tests for desktop screensharing

Review of attachment 8458061 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM with small NITs.

::: dom/media/tests/mochitest/test_getUserMedia_basicScreenshare.html
@@ +40,5 @@
> +      checkMediaStreamTracks(constraints, aStream);
> +
> +      var playback = new LocalMediaStreamPlayback(testVideo, aStream);
> +      playback.playMediaWithStreamStop(false, function () {
> +        aStream.stop();

Sorry for not being precise enough in my last review: if you use playMediaWithStreamStop() I think you no longer need to call .stop() yourself. Just the .finish() call in the success callback should be enough.

::: dom/media/tests/mochitest/test_peerConnection_basicWindowshare.html
@@ +12,5 @@
> +<body>
> +<pre id="test">
> +<script type="application/javascript">
> +  createHTML({
> +    bug: "1038926",

Is there a reason why the PC tests refer to other bugs then the gUM tests?
I would prefer them to be aligned (with no preference for which number).

        Attachment #8458061 -
        Flags: review+

    
    

    
  
The latest patch has tests for Window sharing as well.  Needs to land with or after the Window sharing bug (Bug 1038926).
Summary: Basic tests for Desktop Screensharing → Basic tests for Desktop & Window Screensharing

    
    

    
  
https://hg.mozilla.org/integration/mozilla-inbound/rev/727bd16b29d3
FYI, I realized I didn't address the last nits here; I'll land that as a followup under the existing r+.

For the entire landing: https://tbpl.mozilla.org/?tree=Try&rev=c9bd8d240a61
Target Milestone: --- → mozilla33

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/727bd16b29d3
Assignee: nobody → rjesup
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED

    
    

    
  
Sorry for not spotting this at my review, but these tests actually do not test screen or window sharing. They simply just send a regular fake video stream, because the media constraints in these test are for Google Chrome.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug_1039666_fix_screen_share_test.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
This should hopefully fix the tests. It should have the correct media constraints and sets the permissions before running the test.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=24e7fd007e88

        Attachment #8459948 -
        Flags: review?(rjesup)

    
    

    
  
Comment on attachment 8459948 [details] [diff] [review]
bug_1039666_fix_screen_share_test.patch

Review of attachment 8459948 [details] [diff] [review]:
-----------------------------------------------------------------

With the same changes duplicated in to the getusermedia screen/window share tests, r+.  Thanks!

        Attachment #8459948 -
        Flags: review?(rjesup) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug_1039666_fix_screen_share_test.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
This patch also fixes the gUM screen share tests.

Carrying forward r+=jesup

Try run: https://tbpl.mozilla.org/?tree=Try&rev=52c85cfbbc49

        Attachment #8459948 -
        Attachment is obsolete: true
Whiteboard: [sceensharing-uplift]
Blocks: 1043808
Target Milestone: mozilla33 → mozilla34
Priority: -- → P1
Blocks: 1047121

        Attachment #8465856 -
        Flags: review?(drno)

    
    

    
  
Comment on attachment 8465857 [details] [diff] [review]
Disable screen/windowsharing for OSX 10.6 and WinXP

10.6 screen sharing crashes (window is ok).  WinXP window sharing works; screen sharing works for some people; but on tbpl the tests never exit (VM issues?)  Disabling for both for now.

        Attachment #8465857 -
        Flags: review?(cpearce)

        Attachment #8465857 -
        Flags: review?(cpearce) → review+

    
    

    
  
Comment on attachment 8465856 [details] [diff] [review]
Enable Screen/windowsharing tests for all but OSX 10.6 and XP

Review of attachment 8465856 [details] [diff] [review]:
-----------------------------------------------------------------

I don't quite understand why we need a new patch for this, as it seems to implement exactly the same functionality as attachment 8460064 [details] [diff] [review] did already.
And I'm not a big fan of enabling the screen sharing on the global level in head.js, as we now won't have any way to distinguish on the receiving side if we are receiving a screen share or the video feed.

But at least this tests the screen sharing properly now.

        Attachment #8465856 -
        Flags: review?(drno) → review+

    
    

    
  
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd4659598fe4
fix for suppressing on b2g with the wrong skip-if (toolkit instead of buildapp)
Blocks: 1048017

    
    

    
  
Unfortunately I had to revert https://hg.mozilla.org/integration/mozilla-inbound/rev/4c12508e3b69 since it conflicted with changes blassey landed on fx-team, that had merged to mozilla-central ahead of this (bug 1037424). This can reland after a rebase (this was one of several conflicts during today's merges; didn't seem worth the risk of me resolving in a hurry):

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a28657e4996
Keywords: leave-open
Depends on: 1048455
Keywords: leave-open
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago

          status-firefox34:
          affected → fixed
Resolution: --- → FIXED

    
    

    
  
Comment on attachment 8465856 [details] [diff] [review]
Enable Screen/windowsharing tests for all but OSX 10.6 and XP

Note: this applies to the full set of patches here (minus any that were in the 33 uplift from nightly)

Approval Request Comment
[Feature/regressing bug #]: screensharing

[User impact if declined]: no test coverage on Aurora

[Describe test coverage new/current, TBPL]: This is for the test coverage, plus disabling the feature on 10.6/XP which have problems.

[Risks and why]: Very low risk; only browser code changing is the disables.

[String/UUID change made/needed]: none

        Attachment #8465856 -
        Flags: approval-mozilla-aurora?

        Attachment #8460064 -
        Attachment is obsolete: true

        Attachment #8465857 -
        Flags: approval-mozilla-aurora?

        Attachment #8466664 -
        Flags: review+

        Attachment #8466664 -
        Flags: approval-mozilla-aurora?

        Attachment #8465856 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
  
Depends on: 1050247

        Attachment #8465857 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

        Attachment #8466664 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: