PierfrancescoSoffritti / android-youtube-player

YouTube Player library for Android and Chromecast, stable and customizable.
https://pierfrancescosoffritti.github.io/android-youtube-player/
MIT License
3.44k stars 765 forks source link

Add videoId to IFramePlayerOptions to avoid initialization errors #1166

Closed talklittle closed 2 months ago

talklittle commented 2 months ago

The API can report initialization errors if no videoId is provided initially, when constructing the YT.Player in JS. This error seems to happen less if using loadVideo() to autoplay. But when using cueVideo(), or doing nothing after initialization, the error happens with error value VIDEO_NOT_PLAYABLE_IN_EMBEDDED_PLAYER.

Passing videoId in IFramePlayerOptions seems to prevent the error.

PierfrancescoSoffritti commented 2 months ago

Hey, thanks for the PR.

Could you provide an example that I can use to reproduce the exception?

talklittle commented 2 months ago

Thanks for the reply. The error is not reproducing at the moment. Same video ID as earlier today (actually it was all video IDs I tried, various types of media, like presentation videos and music videos), same code without videoId. I saw in other issues (#202) that similar things happened to other devs: the onError was firing with VIDEO_NOT_PLAYABLE_IN_EMBEDDED_PLAYER but then the problem "went away" by itself.

Earlier today when I added the videoId in IFramePlayerOptions the problem went away, and I did verify the problem came back again when I removed the videoId. But tonight the problem isn't happening at all anymore, with or without that parameter. Confusing!

PierfrancescoSoffritti commented 2 months ago

ugh confusing indeed. I suggest we hold on to this PR until we have a clearer idea of what's happening, what do you think?

talklittle commented 2 months ago

(Forgot to mention: The original error did not actually prevent videos from playing. Despite onError firing after initialization, I was able to play videos fine. The onError seemed to be associated with the "undefined" video ID that was passed to the YT.Player constructor.)


I suggest we hold on to this PR until we have a clearer idea of what's happening, what do you think?

Well, pushing back on that, I actually think it would be good to merge the PR. Considering the same thing apparently happened to multiple devs historically—When people randomly encounter the error, they will have one additional avenue available to them to resolve the issue. Worst case, it ends up being an unneeded addition that devs can ignore.

Also note that the IFrame API reference suggests that videoId is generally required in the YT.Player constructor. It's only optional when providing a full YouTube URL in an <iframe> element—but this library uses an empty div, not an iframe with URL, so that exemption doesn't apply.

PierfrancescoSoffritti commented 2 months ago

Forgot to mention

So the only drawback of this error is that onError is called, but the video plays fine? I think in #202 they are complaining about the video not being playable at all. So it seems like these are separate issues?

IFrame API reference suggests that videoId is generally required in the YT.Player constructor

I couldn't find mentions of it being required, am I missing something? To verify this it should be enough to create a player without videoId and add a delay between the creation and calling cueVideo/loadVideo. If the videoId is actually required I would expect the error to be reported all the time with this delay.

When people randomly encounter the error, they will have one additional avenue available to them to resolve the issue. Worst case, it ends up being an unneeded addition that devs can ignore.

I am hesitant about merging code that we don't know will actually fix the issue. As it could just end up polluting the API and docs, doing more harm than good by creating confusion.

They way forward is to find a way to replicate the problem.

talklittle commented 2 months ago

My take on what appears to be happening is that passing no videoId results in the API loading a "placeholder" blank video, presumably owned by YouTube. The placeholder was probably undergoing issues earlier today, like maybe they deleted it or changed its visibility by accident.

Rather than say "pass videoId to avoid the API throwing errors" I was considering changing the javadoc to say "pass videoId to ensure your target video is loaded initially; otherwise a blank placeholder video will be loaded by the API until loadVideo or cueVideo are called."

Edit: My evidence for the "placeholder" video theory is that earlier today when I removed my cueVideo() call entirely, after initialization it showed the YouTube "sad face" and gray background and "This video is unavailable".

PierfrancescoSoffritti commented 2 months ago

You might be onto something here, but this hypothesis also needs to verified.

Verifying that the videoId is actually always required might be a good place to start.

talklittle commented 2 months ago

I take that back: videoId is not required. Omitting it means a placeholder video —I'm pretty sure of the placeholder video after giving today's evidence a second thought, thanks to this conversation.

If I change the javadoc to clarify videoId is not to prevent any error, but an optional choice to skip the initial placeholder video—would that be sufficient to get the PR merged?

PierfrancescoSoffritti commented 2 months ago

I have tried to replicate the case you are talking about by creating a YouTubePlayerView and never calling cueVideo/loadVideo.

What happens seems to make sense to me:

  1. The YouTube logo shows up.
  2. When I click on it, an error is shown.
  3. If I load a video it starts playing.

See attached video.

https://github.com/user-attachments/assets/b1ba64a0-557c-4b7f-a782-52ace34cd76b

talklittle commented 2 months ago

Thank you very much for taking the time to try it out. Since the placeholder video is working again tonight, the screen recording does show the correct expected behavior. The broken behavior was from earlier today when the placeholder was broken, and did not have the same error message. In your recording "An error occurred. Please try again later." While in my case it was "This video is unavailable".

Right now I'm pushing the proposed clarification to the javadoc/Readme and will also update this PR title accordingly.

talklittle commented 2 months ago

Thank you for your attention and the conversation. I will take extra time to look at this again and verify the behaviors. I want to be sure passing a video ID will change the initial loading behavior away from a blank screen. Closing PR until I fully investigate and re-test.

PierfrancescoSoffritti commented 2 months ago

Thanks to you for taking the time to make a PR and to help improving the library :)

By the way, I wonder if what you need could be achieved with the videoId attribute.

talklittle commented 2 months ago

Last comment before I sleep :) Thanks for pointing out the XML attribute app:videoId. I was aware of that, but it does not fit my current use case which needs dynamic loading. When I redo my PR I will remember to forward that value to the JS constructor, starting from the YouTubePlayerView init block. (And I do need to redo the PR; I actually messed up how I was using videoId, so it's a very good thing you got me to double-check! So far my 2nd try is working well: the API is now loading an initial video cover image, even with cueVideo() commented out.)

PierfrancescoSoffritti commented 2 months ago

Yeah it is unfortunate that videoId can only be used from the XML.

Sounds good, essentially the scope of the new PR would be: "allow setting initial videoId programmatically". Without making assumptions about undocumented behavior of the IFrame player.