fregante / iphone-inline-video

📱 Make videos playable inline on the iPhone (prevents automatic fullscreen)
https://npm.im/iphone-inline-video
MIT License
2.05k stars 297 forks source link

Reset source on currentSrc change #45

Closed timdp closed 8 years ago

timdp commented 8 years ago

This is somewhat related to the currentSrc-related PR I made earlier. If I call play() on a video element without a source, then change its src, and then call play() again, the source won't be synced with the audio element.

fregante commented 8 years ago

Can you verify that the latest version on GitHub works for you?

I was wondering what would happen normally in your situation (.play() without any src) and it seems like the playback state would be kept (.paused === false) until a source is added, that is: it will automatically start playing once the source is added. My commit seems to work in that situation, let me know if it works for you please so I'll push to npm.

timdp commented 8 years ago

Haven't tested yet, but the update function happens asynchronously, so what happens if you change the video source and call play immediately?

fregante commented 8 years ago

.play() is skipped if the video is already paused === false

https://github.com/bfred-it/iphone-inline-video/blob/gh-pages/index.js#L73-L86

timdp commented 8 years ago

Yeah, but how does that help if play gets called only before the interval fires? I think you want to call update on an emptied event or something. Again, I haven't tested yet because I don't have an iPhone handy here, so sorry if I'm not making sense.

fregante commented 8 years ago

No worries, it's good to discuss these things to find any issues I might have missed.

The play call is irrelevant either way since the video is already in a playing state so:

.play(); // starts onUpdate
.src = '...';
.play(); // does nothing

--- onUpdate
.load(); // triggers 'emptied'

--- onEmptied
audio.src = video.currentSrc // which is essentially what your commit does
timdp commented 8 years ago

Hmm, I'm still not fully seeing it. Let me get back to you when I can actually test and debug. :-)

fregante commented 8 years ago

Are you talking about a situation where you change the src and then play for the first time?

fregante commented 8 years ago

I pulled the previous commit and pushed a new one to another branch. Test that, it seems to work better and it takes your suggestion :)

timdp commented 8 years ago

Okay, I tested in Chrome with my scenario, passing true, false to force loading, so these results won't be fully accurate. However:

I'll try to explain what I'm doing without giving away too many of our company secrets. :-) We have a video element without a source, which we arm as soon as possible by calling play() upon user interaction. Then, a while later, when we've decided which source we want to play, we set the src attribute and call play() again. That's basically it.

fregante commented 8 years ago

You can't test in Chrome, the playback has always been problematic in Chrome and I get issues regularly. Test on an iPhone

I tested many variations of the playback in src-change-support:

All with and without audio (it's a different behavior internally)

fregante commented 8 years ago

I tried in Chrome just to see the issue and apparently for some reason the network goes idle every other frame, so a new load is triggered. I tried replicating the behavior on the iPhone with a super-slow loading video or a super-slow playback, but in neither case I saw that happening.

Therefore this doesn't seem to apply, but if/when it does it's rather problematic (the flickering is caused by the video re-starting to download)

That extra .load() call is necessary to keep the video playing when switching from an empty source, I'm trying to match the behavior on desktop.

I tried extending the condition for the load() that's causing the flickering from

player.video.networkState === player.video.NETWORK_IDLE

to:

player.video.networkState === player.video.NETWORK_IDLE && !player.video.buffered.length

But (in Chrome) it causes the video not to load at all until to call load() manually.

Let me know if you can think of another way to check whether the video is not loading, but should.

timdp commented 8 years ago

Can you just elaborate on why my original fix didn't work for you?

fregante commented 8 years ago

Third paragraph. Your fix is now in there, I just extended it.

timdp commented 8 years ago

Ah, sorry, got confused there. I hope to be able to test on an iPhone tomorrow.

timdp commented 8 years ago

Finally got around to testing. My patch itself didn't work that great; emptied happens before play(), so the source update doesn't always happen. It looks like your additions did fix it though. It's still broken on Chrome desktop, but I guess that doesn't matter.

fregante commented 8 years ago

This is now in 1.9.0!

I applied the condition described above so it won't trigger a reload every other frame in Chrome, but I think you'll have to run .load() in Chrome after adding the .src, as I said. Which is a minor inconvenience compared to the risk of constant reloading :)

Thanks for your time/report/pr!