1048688 - videos without an extension in the source URL aren't castable

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Screencasting, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: get_mime_type.patch

STR:
1) got to vimeo.com
2) click on a video
3) long press on a video

ER: "Cast to Screen" in the context menu
AR: "Cast to Screen" not in the menu

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

This also make embedded YouTube work

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8467528 [details] [diff] [review]
get_mime_type.patch

This looks sync and would block. I don't think we can block.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

deb, do you want to track a release for this?

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #2)
> Comment on attachment 8467528 [details] [diff] [review]
> get_mime_type.patch
> 
> This looks sync and would block. I don't think we can block.

This is called from the selector function of the NativeWindow.contextMenu, which looks to be sync by nature.

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 8467528 [details] [diff] [review]
get_mime_type.patch

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

I have no good solutions for you for this blocking call. Because of that (and because I want this) I say we let this through. I'll let mfinkle make the final call though.

::: mobile/android/chrome/content/CastingApps.js
@@ +289,5 @@
>    },
>  
> +  _getContentTypeForURI: function(aURI) {
> +    let channel = Services.io.newChannelFromURI(aURI);
> +    let stream = channel.open();

I guess we should flip this to asyncOpen. BUT, that means making the entire context menu system async. That will be pretty painful. I'm not sure of any good alternative.

@@ +296,5 @@
> +      case 301:
> +      case 302:
> +      case 303:
> +      stream.close();
> +      return this._getContentTypeForURI(this.makeURI(channel.getResponseHeader("Location")));

Indent this and the section below.

@@ +298,5 @@
> +      case 303:
> +      stream.close();
> +      return this._getContentTypeForURI(this.makeURI(channel.getResponseHeader("Location")));
> +      default:
> +      type = channel.contentType;

Declare type here. Or use the one above, set it in here, and do the stream.close(); return type; at the end of this function.

@@ +347,4 @@
>          return { element: aElement, source: sourceURI.spec, poster: posterURL, sourceURI: sourceURI, type: sourceNode.type };
>        }
> +
> +      let type = sourceNode.type ? sourceNode.type : this._getContentTypeForURI(sourceURI);

let type = sourceNode.type || this._getContentTypeForURI(sourceURI);

Comment 6

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: get_mime_type.patch

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8469544 [details] [diff] [review]
get_mime_type.patch

This looks somewhat complicated. It also has no tests. Both worry me a little.

Would we be able to simplify things if we only focused on <video> elements that were "playing"? We could trigger a simple async call to pull the mimetype. The callback could fire the <video> event to enable the casting button, which would also make the URLbar button become active.

Let's not mess around with the context menu.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

The only complicated part is the context menu. Unfortunately, the context menu is also the only way to cast a video on vimeo (they have their own video controls).

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #8)
> The only complicated part is the context menu. Unfortunately, the context
> menu is also the only way to cast a video on vimeo (they have their own
> video controls).

Not true. Once the video starts to play, you can tap on the video and see our controls. So the cast button would be visible.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: get_mime_type.patch

This patch actually works with vimeo

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8470103 [details] [diff] [review]
get_mime_type.patch

I'm wondering if the _videoMap should hold weak references. Wes?

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 8470103 [details] [diff] [review]
get_mime_type.patch

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

Not tested, but I'm worried about behavior if you open multiple tabs with videos or a single tab with multiple videos.

::: mobile/android/chrome/content/CastingApps.js
@@ +44,4 @@
>    _castMenuId: -1,
>    mirrorStartMenuId: -1,
>    mirrorStopMenuId: -1,
> +  _videoMap: new Map(),

Use a WeakMap to avoid accidentally holding onto a video element. Also, this needs to be per-tab and it needs to be cleared on location changes.

@@ +267,5 @@
>      let extensions = SimpleServiceDiscovery.getSupportedExtensions();
>      let types = SimpleServiceDiscovery.getSupportedMimeTypes();
>  
>      // Fast path: Is the given element a video element
> +    this._getVideo(aElement, types, extensions, callback);

you need to break here if _getVideo succeeds. Since its async, you probably need to pass a function in instead of the callback, and branch in there.

@@ +285,5 @@
> +      }
> +    } catch(e) {}
> +  },
> +
> +  getVideoSync: function(aElement, aX, aY, callback) {

The naming in here is abysmal. With this there are three "getVideo" functions in here that all do slightly different things.

1.) getVideo takes an element and a point, if the element is a video it returns it, otherwise, it checks at the point. It doesn't use the Map at all. Since it may do this network query to get the mimetype info for a url, its async with this patch.

2.) getVideoSync also takes an element and a point. It only uses the Map to determine what to check.

We should try to move these two closer together if we can. In fact, can we make them one function? i.e. In one case we can call the callback when we fire off the network lookup, and in the other we can call it after we do the lookup. We can pass a "flags" or "options" object to specify behavior (I'm not sure if I hate this idea or not)?

getVideo: function(callback, sync) {
    // try really hard synchronously
    // do an async lookup
    //     then if not sync, call callback
    // if sync, call callback with what we have now
}

From the matches function we'd have to do something like:

var vid;
this.getVideo{function(result) { vid = result; }, SYNCHRONOUS);
return vid != null;

3.) _getVideo actually takes an element, and if we decide its a video makes a nice little object with info about the video for us. We should rename it _getVideoInfo or something. It probably shouldn't bother to check the node type either (i.e. if you pass it a non-video, thats an error).

@@ +289,5 @@
> +  getVideoSync: function(aElement, aX, aY, callback) {
> +    if (this._videoMap.has(aElement)) {
> +      return this._videoMap.get(aElement);
> +    }
> +    // The context menu system will keep walking up the DOM giving us a chance

insert a line before this

@@ +300,5 @@
> +        // Look for a video element contained in the overlay bounds
> +        let rect = element.getBoundingClientRect();
> +        if (aY >= rect.top && aX >= rect.left && aY <= rect.bottom && aX <= rect.right) {
> +          if (this._videoMap.has(element)) {
> +            return this._videoMap.get(element);

No need for the 'has' line with a WeakMap

@@ +314,5 @@
> +    let channel = Services.io.newChannelFromURI(aURI);
> +    let listener = {
> +      onStartRequest: function(request, context) {
> +        switch (channel.responseStatus) {
> +        case 301:

You should indent these cases as well.

@@ +354,4 @@
>        // Use the file extension to guess the mime type
>        let sourceURI = this.makeURI(sourceURL, null, this.makeURI(aElement.baseURI));
>        if (this.allowableExtension(sourceURI, aExtensions)) {
> +        callback({ element: aElement, source: sourceURI.spec, poster: posterURL, sourceURI: sourceURI});

You need to return in here to avoid the check below.

@@ +373,5 @@
>  
>        // Using the type attribute is our ideal way to guess the mime type. Otherwise,
>        // fallback to using the file extension to guess the mime type
> +      if (this.allowableExtension(sourceURI, aExtensions)) {
> +        callback({ element: aElement, source: sourceURI.spec, poster: posterURL, sourceURI: sourceURI, type: sourceNode.type });

return;

@@ +537,4 @@
>  
>    openExternal: function(aElement, aX, aY) {
>      // Start a second screen media service
> +    let video = this.getVideo(aElement, aX, aY, this._openExternal.bind(this));

Remove the 'let video =' bit.

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Wesley Johnston (:wesj) from comment #12)
> Comment on attachment 8470103 [details] [diff] [review]
> get_mime_type.patch
> 
> Review of attachment 8470103 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not tested, but I'm worried about behavior if you open multiple tabs with
> videos or a single tab with multiple videos.
what are you worried about?

> ::: mobile/android/chrome/content/CastingApps.js
> @@ +44,4 @@
> >    _castMenuId: -1,
> >    mirrorStartMenuId: -1,
> >    mirrorStopMenuId: -1,
> > +  _videoMap: new Map(),
> 
> Use a WeakMap to avoid accidentally holding onto a video element. Also, this
> needs to be per-tab and it needs to be cleared on location changes.
Why does it need to be per-tab or cleared if it's a weak map?
 
> @@ +267,5 @@
> >      let extensions = SimpleServiceDiscovery.getSupportedExtensions();
> >      let types = SimpleServiceDiscovery.getSupportedMimeTypes();
> >  
> >      // Fast path: Is the given element a video element
> > +    this._getVideo(aElement, types, extensions, callback);
> 
> you need to break here if _getVideo succeeds. Since its async, you probably
> need to pass a function in instead of the callback, and branch in there.
why?

> @@ +285,5 @@
> > +      }
> > +    } catch(e) {}
> > +  },
> > +
> > +  getVideoSync: function(aElement, aX, aY, callback) {
> 
> The naming in here is abysmal. With this there are three "getVideo"
> functions in here that all do slightly different things.
> 
> 1.) getVideo takes an element and a point, if the element is a video it
> returns it, otherwise, it checks at the point. It doesn't use the Map at
> all. Since it may do this network query to get the mimetype info for a url,
> its async with this patch.
> 
> 2.) getVideoSync also takes an element and a point. It only uses the Map to
> determine what to check.
> 
> We should try to move these two closer together if we can. In fact, can we
> make them one function? i.e. In one case we can call the callback when we
> fire off the network lookup, and in the other we can call it after we do the
> lookup. We can pass a "flags" or "options" object to specify behavior (I'm
> not sure if I hate this idea or not)?
thought about doing this when I wrote it and decided it would be too ugly. I still think that

> 
> @@ +300,5 @@
> > +        // Look for a video element contained in the overlay bounds
> > +        let rect = element.getBoundingClientRect();
> > +        if (aY >= rect.top && aX >= rect.left && aY <= rect.bottom && aX <= rect.right) {
> > +          if (this._videoMap.has(element)) {
> > +            return this._videoMap.get(element);
> 
> No need for the 'has' line with a WeakMap
sure there is, we don't want to return if its null, we want to keep looping.
 
 
> @@ +354,4 @@
> >        // Use the file extension to guess the mime type
> >        let sourceURI = this.makeURI(sourceURL, null, this.makeURI(aElement.baseURI));
> >        if (this.allowableExtension(sourceURI, aExtensions)) {
> > +        callback({ element: aElement, source: sourceURI.spec, poster: posterURL, sourceURI: sourceURI});
> 
> You need to return in here to avoid the check below.
debatable. Especially when we have devices that support a different set of mime types, we should figure out all of the formats that are in the sources elements. This CastingApps doesn't handle that now (i.e. if you have an element with a webm src and an mp4 in the sources and there is a chromecast and a Roku on your network, the webm video will be offered to your Roku, which will fail). Doing it this way gets us part way there.

 
> @@ +373,5 @@
> >  
> >        // Using the type attribute is our ideal way to guess the mime type. Otherwise,
> >        // fallback to using the file extension to guess the mime type
> > +      if (this.allowableExtension(sourceURI, aExtensions)) {
> > +        callback({ element: aElement, source: sourceURI.spec, poster: posterURL, sourceURI: sourceURI, type: sourceNode.type });
> 
> return;
see above

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: get_mime_type.patch v0.2

I took a crack and refactoring this patch a bit:

1. I removed _videoMap and renamed getVideoSync to isVideoCastable. getVideoSync (and _videoMap) were only being used to drive the context menu, but I found we didn't need them at all. I changed filterCast.match a little so it matches the pageaction icon behavior: you're allowed to cast a video that is already being cast. This simplifies the code a bit and removes the need for the _videoMap.

2. isVideoCastable depends entirely on the code that sets video.mozAllowCastable, which is fired when a <video> binding is created/attached. Saves us holding extra state and saves CPU cycles.

3. I found a bug in the way getVideo(..., aCallback) was being implemented. We MUST return ASAP from getVideo and _getVideo. If we don't, the callback can be fired multiple times. This patch makes _getVideo(... aCallback) return true|false depending on whether we found a <video> element or not. If we did not, then we need to search for a hidden <video> in getVideo(..., aCallback).

I tested this patch on vimeo.com and ted.com; and both work fine where previously they did not.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: get_mime_type.patch v0.3

This patch also indents the switch/case and removes the unused "let video = "

Comment 16

[Wesley Johnston (:wesj)]

Comment on attachment 8476292 [details] [diff] [review]
get_mime_type.patch v0.3

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

I'm still a little worried about these return values.

::: mobile/android/chrome/content/CastingApps.js
@@ +242,1 @@
>        return;

Maybe splinter is making this confusing. Why the return here?

@@ +283,5 @@
>        for (let element of elements) {
>          // Look for a video element contained in the overlay bounds
>          let rect = element.getBoundingClientRect();
>          if (aY >= rect.top && aX >= rect.left && aY <= rect.bottom && aX <= rect.right) {
> +          this._getVideo(element, types, extensions, aCallback);

Since this has no escape hatch, it will check all the videos at this point?

@@ +343,5 @@
> +      // Do an async fetch to figure out the mimetype of the source video
> +      let checkType = function(aType) {
> +        if (this.allowableMimeType(aType, aTypes)) {
> +          aCallback({ element: aElement, source: sourceURI.spec, poster: posterURL, sourceURI: sourceURI, type: aType });
> +          return true;

These returns don't go anywhere, do they? I think maybe we need to encapsulate this into better tasks so that it can look like:

function findCastableUrl(urls, callback) {
  var index = 0;
  var url = urls[0];
  var testCallback = function(result) {
    if(result) {
      callback(result);
    } else if (urls.length > 0) {
      testUrl(urls.shift());
    } else {
      callback(null);
    }
  };
  testUrl(url, testCallback);
}

var urls = [list-of-urls-from-this-video];
this.findCastableUrl(urls, callback);

In reality, you might even want to split testUrl into a fast synchronous test and a slow async one. i.e. run through all the urls and do fast checks. If none of those pass, run through the list again and do slow checks. I think we may need something similar for videos. i.e.

1.) Give me all the videos at a point
2.) Grab the first and asynchronously check if its castable
3.) If it is, we're done!
4.) If its not, grab the next one. Go to step 2.

Having lots of videos stacked at a point seems like a rare thing to do, but I'd at least like a comment there explaining that its a known assumption.

@@ +347,5 @@
> +          return true;
> +        }
> +        return false;
> +      }.bind(this);
> +      this._getContentTypeForURI(sourceURI, checkType);

Insert a blank line before this.

@@ +369,5 @@
> +          aCallback({ element: aElement, source: sourceURI.spec, poster: posterURL, sourceURI: sourceURI, type: aType });
> +          return true;
> +        }
> +        return false;
> +      }.bind(this);

All this repeated code can be in testUrl()

@@ +379,4 @@
>        }
>      }
>  
> +    return true;

This should return false by default. i.e. We didn't find anything castable (yet), but the async callbacks are probably still running so we may in a bit. The return is just used to fast-path you out, and we can't fast-path in that case.

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8476292 [details] [diff] [review]
get_mime_type.patch v0.3

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

::: mobile/android/chrome/content/CastingApps.js
@@ +242,1 @@
>        return;

I don't know why it's there. Removing!

@@ +283,5 @@
>        for (let element of elements) {
>          // Look for a video element contained in the overlay bounds
>          let rect = element.getBoundingClientRect();
>          if (aY >= rect.top && aX >= rect.left && aY <= rect.bottom && aX <= rect.right) {
> +          this._getVideo(element, types, extensions, aCallback);

You're right. I removed the previous escape hatch, but I think we should keep it. No reason not to exit early if we find a video.

@@ +343,5 @@
> +      // Do an async fetch to figure out the mimetype of the source video
> +      let checkType = function(aType) {
> +        if (this.allowableMimeType(aType, aTypes)) {
> +          aCallback({ element: aElement, source: sourceURI.spec, poster: posterURL, sourceURI: sourceURI, type: aType });
> +          return true;

The returns are only really for sync discoveries (i.e. where we know from the URL that the format is OK), but we can't return early for the async discoveries.

I agree about the video sources searching. Searching for URLs first and then scanning that list for sync and async choices is a better approach. I might try that in this patch unless it gets too unwieldy and I do it separately.

I don't think I'm worried about doing the same thing for videos though.

OK. I started looking into this and it's a little bit more complicated. We also need to pass the element and posterURL in as well, so we can create the JS literal. I am trying something in-between.

@@ +379,4 @@
>        }
>      }
>  
> +    return true;

Makes sense

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: get_mime_type.patch v0.4

This addresses most of Wes' comments. I was not able to completely extract the "list of URLs we need to fetch" out into a standalone method, but I added it to the _getVideo call. Now, we give every opportunity to make a local/sync confirmation before needing to rely on the async networking checks.

Comment 19

[Wesley Johnston (:wesj)]

Comment on attachment 8501960 [details] [diff] [review]
get_mime_type.patch v0.4

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

One worry, but seems good. Task.jsm would be nice here (hint hint).

::: mobile/android/chrome/content/CastingApps.js
@@ +121,5 @@
>          callback: function() {
>            function callbackFunc(aService) {
>              let app = SimpleServiceDiscovery.findAppForService(aService);
> +            if (app) {
> +              app.mirror(function() {});

Is this callback just a "we started mirroring" thing we want to ignore? Can we not just pass undefined?

@@ +248,5 @@
>      if (SimpleServiceDiscovery.services.length == 0) {
>        return;
>      }
>  
> +    let callback = function(aVideo) {

Put this inline if its only used once.

@@ +280,5 @@
>    },
>  
> +  allowableExtension: function(aURI, aExtensions) {
> +    if (aURI && aURI instanceof Ci.nsIURL) {
> +      for (let x in aExtensions) {

Not your code, but can we use some array functions here? for...in/of always scare me to be honest.

return aExtensions.some((ext) => aURI.fileExtension === ext)

@@ +288,5 @@
> +    return false;
> +  },
> +
> +  allowableMimeType: function(aType, aTypes) {
> +    for (let x in aTypes) {

Same here.

@@ +352,4 @@
>  
> +  // Because this method uses a callback, make sure we return ASAP if we know
> +  // we have a castable video source.
> +  _getVideo: function(aElement, aTypes, aExtensions, aCallback) {

Because I'm the naming police, two getVideo functions in here is a bit confusing.

@@ +371,5 @@
>        // Use the file extension to guess the mime type
>        let sourceURI = this.makeURI(sourceURL, null, this.makeURI(aElement.baseURI));
>        if (this.allowableExtension(sourceURI, aExtensions)) {
> +        aCallback({ element: aElement, source: sourceURI.spec, poster: posterURL, sourceURI: sourceURI});
> +        return true;

You don't need/use this return value anymore.

@@ +412,5 @@
> +      let checkType = function(aType) {
> +        if (this.allowableMimeType(aType, aTypes)) {
> +          aCallback({ element: aElement, source: sourceURI.spec, poster: posterURL, sourceURI: sourceURI, type: aType });
> +        }
> +      }.bind(this);

Just put this inline.

@@ +414,5 @@
> +          aCallback({ element: aElement, source: sourceURI.spec, poster: posterURL, sourceURI: sourceURI, type: aType });
> +        }
> +      }.bind(this);
> +
> +      this._getContentTypeForURI(sourceURI, checkType);

This will kick off all the requests at once right? It will also potentially call the callback multiple times. I wouldn't care if you just stored a boolean to make sure we only fire the callback once. heck, maybe even

callback.fired = false
let checkType = function(type) {
  if (callback.fired) return;
  if (typeisGood) {
    callback.fired = true;
    callback();
  }
}

@@ +443,5 @@
> +          // Use the flag set when the <video> binding was created as the check
> +          return element.mozAllowCasting;
> +        }
> +      }
> +    } catch(e) {}

We really should move this to a generator or something. Different bug.

@@ +603,5 @@
>        return;
>      }
>  
> +    function filterFunc(aService) {
> +      return this.allowableExtension(aVideo.sourceURI, aService.extensions) || this.allowableMimeType(aVideo.type, aService.types);

since these methods return booleans, I'd rather they started with 'is'. i.e. isAllowableExtension or isExtensionCastable

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Wesley Johnston (:wesj) from comment #19)

> > +    let callback = function(aVideo) {
> 
> Put this inline if its only used once.

Done

> > +  allowableExtension: function(aURI, aExtensions) {
> > +    if (aURI && aURI instanceof Ci.nsIURL) {
> > +      for (let x in aExtensions) {
> 
> Not your code, but can we use some array functions here? for...in/of always
> scare me to be honest.
> 
> return aExtensions.some((ext) => aURI.fileExtension === ext)

Done

> This will kick off all the requests at once right? It will also potentially
> call the callback multiple times. I wouldn't care if you just stored a
> boolean to make sure we only fire the callback once. heck, maybe even
> 
> callback.fired = false
> let checkType = function(type) {
>   if (callback.fired) return;
>   if (typeisGood) {
>     callback.fired = true;
>     callback();
>   }
> }

I used this pattern

I also had to update the testVideoDiscovery test

https://hg.mozilla.org/integration/fx-team/rev/a97b95030a94

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a97b95030a94