Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

p5.MediaElement size() #1079

Closed
shiffman opened this issue Nov 6, 2015 · 10 comments
Closed

p5.MediaElement size() #1079

shiffman opened this issue Nov 6, 2015 · 10 comments
Assignees

Comments

@shiffman
Copy link
Member

shiffman commented Nov 6, 2015

Filing an issue @lmccart and I just talked about related to #1022. We adjusted the associated canvas size when calling loadPixels() so that the pixel array would match the DOM element size. I'm noticing users confused even when just calling image() to draw the image to the canvas that the size doesn't match what is set with size().

@lmccart lmccart closed this as completed in 317fdd1 Nov 8, 2015
@shiffman
Copy link
Member Author

I'm not sure how this happened but I'm looking at p5.dom.js 0.2.7 and the code change from 317fdd1 no longer seems to be in there. This is causing the pixels not to be resized appropriately when calling size() of a p5.MediaElement.

I believe here line 1679:

if (this.canvas.width !== this.elt.width) {
  this.canvas.width = this.elt.videoWidth;
  this.canvas.height = this.elt.videoHeight;
  this.width = this.canvas.width;
  this.height = this.canvas.height;
}

should be:

if (this.canvas.width !== this.elt.width) {
  this.canvas.width = this.elt.width;
  this.canvas.height = this.elt.height;
  this.width = this.canvas.width;
  this.height = this.canvas.height;
}

Or maybe something should be happening when size() is called that makes this work with the existing code? I'm not sure but it is broken again. Happy to pull request the above, but perhaps some other things changed and I'm missing something.

@shiffman shiffman reopened this Jan 13, 2016
@lmccart
Copy link
Member

lmccart commented Jan 13, 2016

thanks for catching this! i think there was something tricky with this one... let me look into it tomorrow. what would be super helpful is a unit test for this. i think it could just check the size of elements or a test a few pixels in the pixels array.

@lmccart
Copy link
Member

lmccart commented Jan 16, 2016

very weird, i can't tell how this got lost but you're right it should def be the second version. i just made the change. thanks!

@lmccart lmccart closed this as completed Jan 16, 2016
shiffman added a commit that referenced this issue Feb 4, 2016
This is an additional fix for when you want to draw a resized video into your canvas, but have not yet called loadPixels().
lmccart pushed a commit that referenced this issue Feb 4, 2016
Additional fix #1079 drawing p5.MediaElement after size()
@shiffman
Copy link
Member Author

This particular fix was reverted due to issue #1274 and the pixels array length does not match the specified size() anymore. You can see how the commit 8022760 changes this.elt.width back to this.elt.videoWidth. We seem to be in an endless cycle of changing this code back and forth. How do we best resolve both of these issues?

I am making a video about this question and when I upload that video I will post a link here.

Just to be clear again, the code that resolves this issue is:

if (this.canvas.width !== this.elt.width) {
  this.canvas.width = this.elt.width;
  this.canvas.height = this.elt.height;
  this.width = this.canvas.width;
  this.height = this.canvas.height;
}

The code that resolves #1274 is:

if (this.canvas.width !== this.elt.width) {
  this.canvas.width = this.elt.videoWidth;
  this.canvas.height = this.elt.videoHeight;
  this.width = this.canvas.width;
  this.height = this.canvas.height;
}

What code would resolve both?

@shiffman shiffman reopened this Mar 29, 2016
@shiffman
Copy link
Member Author

Here is the video where I filed this issue.

https://youtu.be/6t9m1ePK_-8

@lmccart
Copy link
Member

lmccart commented Mar 30, 2016

ahhh thank you for tracking down why this part always seems to be broken! and the video is amazing! i don't think this should be too hard to fix, i think the problem previously was just that we didn't realize there were two conflicting cases we kept switching back and forth between. i'll take a look at this in the next few days.

@lmccart
Copy link
Member

lmccart commented Apr 12, 2016

@Craigson are you still on this one, or @shiffman mentioned you were shifting over to processing work for a while? I can take this one over if you are busy right now, just let me know.

@Craigson
Copy link

Hi Lauren,

Sorry this mail slipped by me.. I haven't found a solution yet that solves
both issues, everything I've tried causes the other to break. I plan to
help Dan out with some stuff, but I've had to delay it to try make up
ground on thesis. I was going to try and get back to it this week, but if
it's time sensitive I'm happy to let it go.

On Tue, Apr 12, 2016 at 10:26 AM, Lauren McCarthy notifications@github.com
wrote:

@Craigson https://github.com/Craigson are you still on this one, or
@shiffman https://github.com/shiffman mentioned you were shifting over
to processing work for a while? I can take this one over if you are busy
right now, just let me know.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1079 (comment)

Craig Pickard
M.P.S Candidate in the Interactive Telecommunications Program, NYU
{ http://craigpickard.net }
{ 929 240 4478 }

@lmccart
Copy link
Member

lmccart commented Apr 19, 2016

Ok, I think I've fixed this. The fix was to update createCapture() to make sure that this.elt.width and this.elt.height is properly set when the stream is loaded. This allows us to use the correct code inside loadPixels():

if (this.canvas.width !== this.elt.width) {
  this.canvas.width = this.elt.width;
  this.canvas.height = this.elt.height;
  this.width = this.canvas.width;
  this.height = this.canvas.height;
}

Both this issue and #1274 should work properly now.

These were the two test cases I was using:

for issue #1079:

var capture;

function setup() {
  createCanvas(640, 480);
  capture = createCapture(VIDEO);
  capture.size(320, 240);
}

function mousePressed() {
  image(capture, 0, 0);
}

and for issue #1274:

var capture;

function setup() {
  createCanvas(640, 480);
  capture = createCapture(VIDEO);
}

function mousePressed() {
  var img = capture.get(0, 0, width, height);
  image(img, 0, 0, width, height);
} 

please let me know if i missed anything!

@shiffman
Copy link
Member Author

Hooray! I promised to make a follow-up video about this so will do so and test!

indefinit added a commit to indefinit/p5.js that referenced this issue Apr 22, 2016
* 'master' of https://github.com/processing/p5.js: (52 commits)
  fix for vertically centering multiline text (issue processing#1316), updated documentation for textAlign() function
  fixes processing#1079 fixes processing#1274
  Fix sort() example
  first pass xml done
  xml in progress
  xml in progress
  in progress xml
  fixing push pop around redraw closes processing#1058
  adding documentation for createRadio() closes processing#1315
  adding documentation for saveFarmes() closes processing#1324
  adding example for drop
  Don't trigger warnings on preload(). Fixes processing#1350.
  fix processing#1349
  extending timeout to 5s closes processing#1344
  Add reference.css to style docs in this repository.
  Preprocess docs descriptions as Markdown.
  Delete progress.txt
  Fix trailing whitespace in random.js
  Improve documentation for random()
  clarify rationale for catching exceptions thrown by defineProperty.
  ...
shiffman added a commit to shiffman/p5.js that referenced this issue May 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants