opentok / accelerator-annotation-js

MIT License
2 stars 8 forks source link

Annotation Capture (Subscriber) in 'covers' mode is based on incorrect dimensions #22

Open DavyDeDurpel opened 6 years ago

DavyDeDurpel commented 6 years ago

New issue checklist

General information

Bug report

Expected behavior

A screenshot with annotation should look identical to what is visible in the live stream.

...

Actual behavior

The screenshot is in the incorrect format and annotations are not in the correct location. The calculations in 'cover' mode does not support all use cases. This causes incorrect dimensions of the images and also incorrect offset of the annotations.

...

Steps to reproduce

Start a Subrscriber with option: fitMode: 'cover'. Make sure that your canvas dimensions are bigger than your video dimensions (ex. full screen web browser) Do some annotations and make a snapshot.

This is what I believe should be the correct calculation to cover all cases:

if (scaledToFill) {
    var xScale = canvas.width / width;
    var yScale = canvas.height / height;

    if (xScale > yScale) {
      scale = xScale;
      width = canvas.width;
      height = height * scale;
      offsetY = (canvas.height - height) / 2 * yScale > 1 ? -1 : 1;
    } else {
      scale = yScale;
      height = canvas.height;
      width = width * scale;
      offsetX = (canvas.width - width) / 2 * xScale > 1 ? -1 : 1;
    }
  }

...

Crash log? Screenshots? Videos? Sample project?

...

Question or Feature Request

...

DavyDeDurpel commented 6 years ago

It seems that my offset calculation correction was not needed. This code works for me:

     if (scaledToFill) {
        var xScale = canvas.width / width;
        var yScale = canvas.height / height;

        if (xScale > yScale) {
          scale = xScale;
          width = canvas.width;
          height = height * scale;
          offsetY = (canvas.height - height) / 2;
        } else {
          scale = yScale;
          height = canvas.height;
          width = width * scale;
          offsetX = (canvas.width - width) / 2;
        }
      }
maikthomas commented 6 years ago

Hi @DavyDeDurpel, Thanks identifying this bug and proposing a solution! We aren't able to add this is in ourselves just yet, but if you'd like to submit a Pull Request we will be happy to review it. Otherwise we'll get to this when we can. Thanks, Mike