fixed-wrong-capture-size-and-freeze-issue

[Prateek93a]

Resolves #4535

Changes:

First, I updated the check at this line, to use defW and defH if they are not undefined ( when width and height are set by size() method). This fixes the image not in center issue which was caused due to image() using full initial width and height of the video element instead of the one set by size() method.

Second, I added the changes here, to ensure that for p5.MediaElement, new pixels are loaded before image is drawn, hence the video doesn't freeze now which was caused because of pixels not updating on the canvas after calling get() method.

I tested the following code:

let capture;
let img;

function setup() {
  createCanvas(400, 400);
  capture = createCapture(VIDEO);
  capture.size(320, 240);
  capture.hide(); // hide raw camera
  imageMode(CENTER);
}

function draw() {
  background(0);

  image(capture, mouseX, mouseY);

  if (img) {
    // draw the img to test that get() is working correctly
    image(img, 0, 0);
  }
}

function mousePressed() {
  img = capture.get();
}

Screenshots of the change:

Since it involves use of webcam, I did not post any screenshots or gifs, but I have provided the code for testing.

PR Checklist

• npm run lint passes
• Inline documentation is included / updated
• Unit tests are included / updated

[micuat]

ok now looking at series of issues around loadPixels and wrong dimensions I think it's somewhat urgent to merge this PR. However I'm actually not sure calling loadPixels every time will cause performance issue? or is it ok? because context.drawImage in fact doesn't need loading pixels, the issue is simply because of the wrong dimensions

[Prateek93a]

Actually, for the video freezing issue, what caused the problem was that when get() was called, the canvas property was set on the element with the pixel values at that point of time. Now in the image function, cnv got set to img.canvas instead of img.elt after calling get(). Because of this, the same old pixel values got rendered instead of updated values. This problem is not specific to get(). Any function that sets canvas property can cause the this issue because that canvas property does not get updated.

One solution could be to only load pixels on MediaElement if canvas property is set. So I can put if (p5.MediaElement && img instanceof p5.MediaElement && img.canvas) and then load pixels. That could give better performance in several cases where canvas is not set. But then we need to add extra check inside the if(this._tint) block to call loadPixel in case the element is MediaElement. Shall I do this?

[micuat]

I see... so if I understand correctly

• loadPixels is necessary for pixel operation (e.g. get) but you need to create a canvas for it, load pixels on it which can drag the performance
• for image and copy we actually don't need a canvas, so in fact we can skip loadPixels

and the current implementation is to 1) "make sure" pixels are always loaded - which costs the performance (sometimes really badly) and 2) there are mixups of parameters like in this case.

I would say, if the current PR fixes the issues and not breaking other stuff, we should merge it. And then we should open an issue (if not opened yet) about potentially cleaning up the back buffer canvas - whether they should be always loaded or only create a canvas when necessary. What do you think @outofambit ?

[outofambit]

hey @micuat this is the part of the library i'm least familiar with, but i can look into this soon. in the meantime, @stalgiag @aferriss do you happen to have any thoughts here?

[micuat]

thanks a lot, I just tested the build and confirm that fixes #4535 and #5200

I cannot verify if this change can potentially break anything - so further comments are appreciated!

[Qianqianye]

Thanks @Prateek93a and @micuat for working on it.
Hey @stalgiag @aferriss, could you share some thoughts on this? Thank you!

[stalgiag]

This solves the related issues and does not cause issues with any examples (from website or manual tests) that I could find. As far as I can tell this is safe to merge and very welcome. Thank you @Prateek93a!

[Qianqianye]

Thanks @Prateek93a. Could you please resolve the conflicts so we can merge this PR?

[davepagurek]

I see one of the merge conflicts is around the call to loadPixels after I edited that section. It now calls _ensureCanvas, as of #5871. Here's a sketch built off of the main branch to test out calling get, it seems to no longer freeze: https://editor.p5js.org/davepagurek/sketches/_Bw6LpjMN

I think maybe _ensureCanvas is sufficient instead of calling loadPixels, as it keeps the media element's canvas up-to-date but avoids copying all the numbers into the pixels array, which no longer needs to be read for tinting? Let me know if that also seems right to you! If that also solves the same problem, then when we resolve the conflicts, we can go with the version on main for this one. We'll still need your change for defW and defH though!

[Qianqianye]

Thanks @davepagurek! Since we haven't heard back from @Prateek93a, please feel free to resolve the conflict about defW and defH.

I think maybe _ensureCanvas is sufficient instead of calling loadPixels, as it keeps the media element's canvas up-to-date but avoids copying all the numbers into the pixels array, which no longer needs to be read for tinting? Let me know if that also seems right to you! If that also solves the same problem, then when we resolve the conflicts, we can go with the version on main for this one.

@limzykenneth @aferriss, does it sound right to you to go with version on main to resolve the conflict? Thanks!

[limzykenneth]

Sure, @davepagurek if you like do go ahead with resolving the conflicts and merging this. Thanks.