jhiesey / videostream

Play html5 video when from a file-like object
MIT License
243 stars 74 forks source link

Problems when streaming with preload="none" #22

Closed yciabaud closed 8 years ago

yciabaud commented 8 years ago

I can't stream to a video element with preload="none" since the rewrite, I had a look at the code and it seems that the event sourceopen is not fired because of the lack of preload and then I have an error when playing video:

Uncaught Error: The argument to MediaElementWrapper.createWriteStream must be a string or a previous stream returned from that function

In fact it is a previous stream object but the buffer has not been created yet.

Second problem in this configuration is that the stream begins to be pumped before the video is actually played and this is the preloading I was trying to avoid.

What do you think about this?

yciabaud commented 8 years ago

The first part of the issue is probably located in the mediasource module.

feross commented 8 years ago

@yciabaud So this worked fine in videostream 1.x?

yciabaud commented 8 years ago

Yes it was, the stream I wrote between webtorrent and videostream could not be well designed but previously it had the behavior I wanted.

Maybe I should extract a simpler test without my custom stream to be sure.

yciabaud commented 8 years ago

OK I checked with a little test case and without preload it works in videostream 1.2.3 and not in 2.1.2 .

Can you take a look at it?

I used one of my files but the test case is the same as the one in the repo:

var videostream = require('videostream')
var https = require('https')
var MultiStream = require('multistream')
var stream = require('stream')

var video = document.createElement('video')
video.preload = "none"
video.controls = true
document.body.appendChild(video)

var REQUEST_SIZE = 1000000
var exampleFile = {
    createReadStream: function (opts) {
    var self = this
    opts = opts || {}
    var start = opts.start || 0
    var fileSize = -1

    var req = null
    var multi = new MultiStream(function (cb) {
      var end = opts.end ? (opts.end + 1) : fileSize

      var reqStart = start
      var reqEnd = start + REQUEST_SIZE
      if (end >= 0 && reqEnd > end) {
        reqEnd = end
      }
      if (reqStart >= reqEnd) {
        req = null
        return cb(null, null)
      }

      req = https.request({
        protocol: 'https:',
        hostname: 's3-eu-west-1.amazonaws.com',
        port: 443,
        path: '/wetube-pseudostreaming/720p/videos/tKrPDjSvh4gJGRob8/1456258872200_SampleVideo_1080x720_2mb.mp4',
        headers: {
          range: 'bytes=' + reqStart + '-' + (reqEnd - 1)
        }
      }, function (res) {
        var contentRange = res.headers['content-range']
        if (contentRange) {
          fileSize = parseInt(contentRange.split('/')[1], 10)
        }
        cb(null, res)
      })
      req.on('error', function(e) {
        console.log('problem with request:' + e.message);
      })

      // write data to request body
      req.end();

      // For the next request
      start = reqEnd
    })
    var destroy = multi.destroy
    multi.destroy = function () {
      if (req) {
        req.destroy()
      }
      destroy.call(multi)
    }
    return multi
  }
}

videostream(exampleFile, video)
jhiesey commented 8 years ago

I'll take a look at this tonight

jhiesey commented 8 years ago

There are two separate issues here:

  1. The mediasource module doesn't handle multiple calls to createWriteStream before the mediasource opens. This was causing the exceptions. I have a fix that I need to test more but should work.
  2. videostream completely fails and/or starts downloading video data too early. This is fixed in v2.2.0, which also should not trigger the error in mediasource (even without my fix for that).

@yciabaud could you try version 2.2.0 and see if it fixes your problem?

yciabaud commented 8 years ago

Yes it does, good work! All is working as I need. Can't wait to deploy it, thank you for the quick fixing.

feross commented 8 years ago

As usual, @jhiesey gets :100: / :100: