t-mullen / video-stream-merger

Merge multiple video MediaStreams into one composite.
https://t-mullen.github.io/video-stream-merger/
MIT License
357 stars 81 forks source link

When trying to addStream() yPosition not working #73

Closed Iulian33 closed 3 years ago

Iulian33 commented 3 years ago
 const merger = new VideoStreamMerger();
    merger.addStream(mediaStream, {
      x: 200,
      y: 410,
      width: 400,
      height: 225,
      mute: true
    });

not working y:410 in version @4.0.0

hthetiot commented 3 years ago

I will check, thx @Iulian33

hthetiot commented 3 years ago

I did not saw the difference between V3 implementation, initially then saw my typo "stream.Y" vs "stream.y". Sorry will make PR to fix ASAP.

      // default draw function
      const width = stream.width == null ? this.width : stream.width
      const height = stream.height == null ? this.height : stream.height
      this._ctx.drawImage(stream.element, stream.x, stream.y, width, height)

Source: https://github.com/t-mullen/video-stream-merger/blob/0cfc83c382ab39ca3ae81f18a11424ce4c650af0/src/index.js#L338

    const canvasHeight = this.height;
    const canvasWidth = this.width;

    const height = stream.height || canvasHeight;
    const width = stream.width || canvasWidth;

    let positionX = stream.x || 0;
    let positionY = stream.Y || 0;

    try {
        this._ctx?.drawImage(element, positionX, positionY, width, height);
    } catch (err) {
      // Ignore error possible "IndexSizeError (DOM Exception 1): The index is not in the allowed range." due Safari bug.
      console.error(err);
    }

Source: https://github.com/t-mullen/video-stream-merger/blob/9edd4057d8f6564011d821e30bde4a7b4db67e37/src/index.ts#L535

Note: the V4 is a bit different cause had KeepRatio implementation in initial PR, will make separate PR for stream.keepRatio

t-mullen commented 3 years ago

Fixed in 4.0.1

Iulian33 commented 3 years ago

Thx a lot ! appreciate your effort