Closed Bug 1072442 Opened 5 years ago Closed 4 years ago

Context menu for WebRTC stream includes non-functional "Play Speed"

Categories

(Toolkit :: Video/Audio Controls, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: mbrubeck, Assigned: nejmeddine.douma)

Details

(Keywords: polish, useless-UI)

Attachments

(1 file, 1 obsolete file)

Steps to reproduce:
1) Open a page that displays a WebRTC video stream (using https://apprtc.appspot.com/ or Loop/Hello, for example).
2) Right-click on the streaming video.
3) Choose "Play Speed: High Speed" from the context menu.

Expected results: "Play Speed" should not appear in the context menu for streaming video.

Actual results: "Play Speed" options appear in the context menu, but have no effect on playback.
This information may be available via HTMLMediaElement.mozSrcObject which can then get a MediaStreamTrack as it seems MediaStreamTrack.remote may indicate whether we're using a WebRTC stream but I'm just guessing by skimming MDN. I'm not sure how to handle this for other media live streams.
I wonder if HTMLMediaElement.seekable is also relevant? Are there cases where media isn't seekable but the playback rate can be adjusted?
Flags: needinfo?(cpearce)
Flags: firefox-backlog?
(In reply to Matthew N. [:MattN] from comment #2)
> I wonder if HTMLMediaElement.seekable is also relevant? Are there cases
> where media isn't seekable but the playback rate can be adjusted?

Potentially a "live" stream. In practice we cache the stream, so we can partially seek in those.
Flags: needinfo?(cpearce)
Flags: firefox-backlog? → firefox-backlog+
Flags: qe-verify?
The spec says that the if the duration is not known, for example a stream, the duration is set to positive Infinity.

In /browser/base/content/nsContextMenu.js, we can add a check to
> this.showItem("context-media-playbackrate", onMedia);

to only show the item if it is onMedia as well as if the target.duration is not equal to Infinity.
(In reply to nejmeddine.douma from comment #8)
> Created attachment 8756365 [details] [diff] [review]
> bug-1072442-fix.patch
> 
> I updated the patch. Thank you for the review.
> I'm working with 4 more students on contruibing to Firefox for the next two
> weeks full-time. We would like to work on 787881. Could we start working on
> 787881 or do you have other bugs that you think it is better for us to work
> on before starting bug 787881?

I will check around and comment in that bug to keep all discussion there.
Flags: needinfo?(jaws)
Keywords: checkin-needed
Attached patch bug-1072442-fix.patch (obsolete) — Splinter Review
Hi Jared,

I made a patch for the bug. Could you please review it?

Thanks
Nejm
Attachment #8756295 - Flags: review?(jaws)
Comment on attachment 8756295 [details] [diff] [review]
bug-1072442-fix.patch

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

I'll mark this as r+, but please upload a new version of the patch with the parenthesis removed and change the commit message to "Bug 1072442 - The Play Speed contextmenu should be hidden for streaming media. r=jaws"

::: browser/base/content/nsContextMenu.js
@@ +468,5 @@
>      this.showItem("context-media-play",  onMedia && (this.target.paused || this.target.ended));
>      this.showItem("context-media-pause", onMedia && !this.target.paused && !this.target.ended);
>      this.showItem("context-media-mute",   onMedia && !this.target.muted);
>      this.showItem("context-media-unmute", onMedia && this.target.muted);
> +    this.showItem("context-media-playbackrate", onMedia && (this.target.duration != Number.POSITIVE_INFINITY));

Looks good, but you can drop the extra set of parenthesis because operator precedence says that `&&` has the lower precedence than an equality comparison and thus will be evaluated last.

You can read https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Operator_Precedence to learn more.
Attachment #8756295 - Flags: review?(jaws) → review+
This is the section of the spec that says duration should be positive Infinity for streaming media, https://dev.w3.org/html5/spec-preview/media-elements.html#offsets-into-the-media-resource
Assignee: nobody → nejmeddine.douma
Status: NEW → ASSIGNED
I updated the patch. Thank you for the review.
I'm working with 4 more students on contruibing to Firefox for the next two weeks full-time. We would like to work on 787881. Could we start working on 787881 or do you have other bugs that you think it is better for us to work on before starting bug 787881?
Thanks
Nejm
Attachment #8756295 - Attachment is obsolete: true
Flags: needinfo?(jaws)
https://hg.mozilla.org/mozilla-central/rev/6a39cf939f60
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1276349
No longer depends on: 1276349
You need to log in before you can comment on or make changes to this bug.