Listen for posterchange on tech #2339

eXon · 2015-07-10T22:37:20Z

When someone is using a YouTube video and doesn't have a poster, we need to put the YouTube default poster. However, this is an async process. We need to check if the highres version exists. If it doesn't we use the low res.

To make that possible, the player has to listen for posterchange on the tech and trigger it again on himself so that the poster can set himself correctly.

When someone is using a YouTube video and doesn't have a poster, we need to put the YouTube default poster. However, this is an async process. We need to check if the highres version exists. If it doesn't we use the low res. To make that possible, the player has to listen for posterchange on the tech and trigger it again on himself so that the poster can set himself correctly.

heff · 2015-07-14T01:46:38Z

I'm not understanding why the player needs to get involved in this process. If the youtube player is handling the poster, it seems like the process of deciding which poster to use should be in a black box as far as the player is concerned.

eXon · 2015-07-14T09:38:19Z

Even though poster() returns something in the tech, the getter is actually never called. How can the player know when to get the new poster to update it?

heff · 2015-07-14T22:57:40Z

Right now the poster attribute is really only used by the PosterImage component.

We assume:

The poster source is being supplied by the player user (not the tech)
The PosterImage component of the player is the primary display of the poster

While the poster is passed to the tech, it's more of a fallback, and I think it's rare that the tech's poster would actually show up whenever the user does supply a poster.

We currently don't ever assume the tech has the source of the poster, however we do still let the tech know, but again, it's more of a fallback.

The posterchange event is triggered so the PosterImage component can know when the user changes the poster, get the new source from player.poster(), and change the PosterImage source.

If the tech triggers a posterchange, the PosterImage will look for a new source and get whatever was previously stored in player.poster_, as set by the user.

Can the youtube tech provide a image source (jpeg, etc.) that the PosterImage component could read and display?

Do we need to treat the poster similarly to controls and tracks, where there's native (tech) vs custom (player) supplied versions of each, and you can choose between them some how?

eXon · 2015-07-15T10:02:37Z

If the user set a poster on VJS, we should use it no matter what of course. However, if none is provided, we need to set the default YouTube poster otherwise we will see a big red play button in the middle with it.

All the YouTube tech needs is a fallback poster that can be used if the user didn't provided one. Is there any way to do that right now?

heff · 2015-07-15T17:32:36Z

otherwise we will see a big red play button in the middle with it

Ok, so you're saying when you load a YouTube video with the YouTube player, it provides a URL to a poster image (jpeg or whatever) that we can use in the video.js player's PosterImage component, so that it covers up the YouTube play button. Is that right?

eXon · 2015-07-15T21:01:35Z

It will cover the whole iframe yep. Only way to get rid of that ugly button we don't want twice (VJS already got one).

heff · 2015-07-15T21:49:05Z

Ok cool, so this is specifically what I was looking to understand.
https://github.com/eXon/videojs-youtube/blob/358cd9666eb041f58f96b05bcee27c753b7acb05/src/Youtube.js#L490

The youtube player does have a poster url we could point to if the user doesn't provide one. So this is yet another twist in the player/tech relationship. It's kind of like the native/custom controls and native/custom tracks, except that in this case we would always be using the custom poster and the source would either be coming from the user or the tech.

I think the next step here would be to update the poster function to get the poster from the tech if there's no user supplied poster.

poster(src) {
    if (src === undefined) {
      return this.poster_ || this.techCall('poster') || '';
    }
  ...

Then ideally the posterchange from the tech would only be fired on the player if there was no user supplied poster. That seems to get a little more complicated.

Another note on this is that this means you can't ever disable the poster with this setup. I'm not sure why a user would want to do that though.

pam · 2015-09-20T19:59:54Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 1086989
Build details: https://travis-ci.org/pam/video.js/builds/81285962

(Please note that this is a fully automated comment.)

pam · 2015-09-20T20:11:30Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 57e53b0
Build details: https://travis-ci.org/pam/video.js/builds/81287330

(Please note that this is a fully automated comment.)

eXon · 2015-09-20T20:14:02Z

@heff This should fix the default poster for any tech.

When the tech start loading, it ask for the default poster.
When the tech is ready, it asks again for the default poster in case it changed (if their is a high res version it will)
When the source changes, it asks for the new default poster

None of this is done if their is a specified poster.

pam · 2015-09-20T20:21:24Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 647354a
Build details: https://travis-ci.org/pam/video.js/builds/81288809

(Please note that this is a fully automated comment.)

pam · 2015-09-20T20:56:30Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 2f341b1
Build details: https://travis-ci.org/pam/video.js/builds/81293406

(Please note that this is a fully automated comment.)

pam · 2015-09-20T21:13:55Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 810d619
Build details: https://travis-ci.org/pam/video.js/builds/81295303

(Please note that this is a fully automated comment.)

pam · 2015-09-20T21:24:01Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: e4a6713
Build details: https://travis-ci.org/pam/video.js/builds/81296925

(Please note that this is a fully automated comment.)

pam · 2015-09-20T21:36:11Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 48ca3a0
Build details: https://travis-ci.org/pam/video.js/builds/81298545

(Please note that this is a fully automated comment.)

eXon · 2015-09-20T21:41:05Z

Had to switch a few unit tests to use HTML5 because poster() on the Flash version isn't working without mocking a few things.

eXon · 2015-09-27T13:21:05Z

@heff What's the plan on the VJS5 release? This PR is critical for the YouTube tech to work correctly.

heff · 2015-09-28T16:10:11Z

@eXon Trying to get it out tomorrow. Reviewing this one now. If I see any changes I'll make a note but try to change them myself.

heff · 2015-09-28T16:31:41Z

test/unit/player.test.js

@@ -67,7 +67,7 @@ test('should accept options from multiple sources and override in correct order'
  videojs.options.attr = 1;

  let tag0 = TestHelpers.makeTag();
-  let player0 = new Player(tag0);
+  let player0 = new Player(tag0, { techOrder: ['html5'] });


We have a 'techFaker' tech that I think could be used here instead of html5. It would be technically safer on IE8.

heff · 2015-09-28T17:01:26Z

Made suggestions here: https://github.com/eXon/video.js/pull/1

Review exon patch 2

eXon · 2015-09-28T17:27:14Z

I think it's even better to be able to tell VJS that the default poster has changed. Thank you for the help! :)

pam · 2015-09-28T17:34:11Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 9d0453a
Build details: https://travis-ci.org/pam/video.js/builds/82594222

(Please note that this is a fully automated comment.)

eXon added 2 commits July 10, 2015 18:37

Added missing semicolom

8b044e5

heff mentioned this pull request Jul 24, 2015

Add poster to the options the tech are receiving #2338

Closed

heff mentioned this pull request Aug 31, 2015

VJS5: Allow the tech to set a default poster #2525

Closed

eXon added 3 commits September 20, 2015 10:54

Merge with recent master

a43ebcf

Removed posterchange event

46aa830

Ask the tech for default poster when none

1086989

Update default poster when changing source

57e53b0

Fix warning in unit test

647354a

Trying to fix unit test for IE8

2f341b1

Trying to fix IE8

810d619

Trying to fix IE8

e4a6713

Trying to fix IE8

48ca3a0

eXon mentioned this pull request Sep 27, 2015

How to setup videojs-youtube for VideoJs 5 videojs/videojs-youtube#310

Closed

heff reviewed Sep 28, 2015
View reviewed changes

heff added 2 commits September 28, 2015 09:33

Switched player tests to use techFaker instead of html5

2281a6a

Handle tech poster changes separately from the poster setter

fefb898

Merge pull request #1 from heff/review-eXon-patch-2

9d0453a

Review exon patch 2

heff closed this in 2de2892 Sep 28, 2015

eXon deleted the patch-2 branch September 28, 2015 19:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Listen for posterchange on tech #2339

Listen for posterchange on tech #2339

eXon commented Jul 10, 2015

heff commented Jul 14, 2015

eXon commented Jul 14, 2015

heff commented Jul 14, 2015

eXon commented Jul 15, 2015

heff commented Jul 15, 2015

eXon commented Jul 15, 2015

heff commented Jul 15, 2015

pam commented Sep 20, 2015

pam commented Sep 20, 2015

eXon commented Sep 20, 2015

pam commented Sep 20, 2015

pam commented Sep 20, 2015

pam commented Sep 20, 2015

pam commented Sep 20, 2015

pam commented Sep 20, 2015

eXon commented Sep 20, 2015

eXon commented Sep 27, 2015

heff commented Sep 28, 2015

heff Sep 28, 2015

heff commented Sep 28, 2015

eXon commented Sep 28, 2015

pam commented Sep 28, 2015

Listen for posterchange on tech #2339

Listen for posterchange on tech #2339

Conversation

eXon commented Jul 10, 2015

heff commented Jul 14, 2015

eXon commented Jul 14, 2015

heff commented Jul 14, 2015

eXon commented Jul 15, 2015

heff commented Jul 15, 2015

eXon commented Jul 15, 2015

heff commented Jul 15, 2015

pam commented Sep 20, 2015

pam commented Sep 20, 2015

eXon commented Sep 20, 2015

pam commented Sep 20, 2015

pam commented Sep 20, 2015

pam commented Sep 20, 2015

pam commented Sep 20, 2015

pam commented Sep 20, 2015

eXon commented Sep 20, 2015

eXon commented Sep 27, 2015

heff commented Sep 28, 2015

heff Sep 28, 2015

Choose a reason for hiding this comment

heff commented Sep 28, 2015

eXon commented Sep 28, 2015

pam commented Sep 28, 2015