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

SaveFrames() misses some frames #5685

Closed
2 of 17 tasks
endurance21 opened this issue Jun 4, 2022 · 4 comments · Fixed by #5694
Closed
2 of 17 tasks

SaveFrames() misses some frames #5685

endurance21 opened this issue Jun 4, 2022 · 4 comments · Fixed by #5694
Labels
Milestone

Comments

@endurance21
Copy link
Contributor

Most appropriate sub-area of p5.js?

  • Accessibility (Web Accessibility)
  • Build tools and processes
  • Color
  • Core/Environment/Rendering
  • Data
  • DOM
  • Events
  • Friendly error system
  • Image
  • IO (Input/Output)
  • Localization
  • Math
  • Unit Testing
  • Typography
  • Utilities
  • WebGL
  • Other (specify if possible)

p5.js version

v1.4.1

Web browser and version

Google Chrome 102.0.5005.61

Operating System

Mac OS

Steps to reproduce this

Steps:

  1. visit https://p5js.org/reference/#/p5/saveFrames
  2. open the console and you will find that the total frames obtained are 22, it should be 25 though, if fps is provided as 25 and duration is 1 sec then the total frames obtained should be 25.

Snippet:

function draw() {
 background(mouseX);
 }

 function mousePressed() {
 saveFrames('out', 'png', 1, 25, data => {
   print(data);
 });
 }
@endurance21 endurance21 added the Bug label Jun 4, 2022
@endurance21
Copy link
Contributor Author

endurance21 commented Jun 4, 2022

My diagnosis and Understanding :

In the implementation of saveFrame API here

p5.prototype.saveFrames = function(fName, ext, _duration, _fps, callback) {

Let's focus on key arguments here, that is

  1. duration
  2. fps

if the duration is 1 sec and fps is 25 that essentially means 1*25 frames should be recorded.

scrolling down, we can encounter the following two parts

PART 1

  const frameFactory = setInterval(() => {
    frames.push(makeFrame(fName + count, ext, cnv));
    count++;
  }, 1000 / fps);

PART 2

  setTimeout(() => {
    clearInterval(frameFactory);
    if (callback) {
      callback(frames);
    } else {
      for (const f of frames) {
        p5.prototype.downloadFile(f.imageData, f.filename, f.ext);
      }
    }
    frames = []; // clear frames
  }, duration + 0.01);
};

I think the problem lies in PART 2, setTimeout stops the setInterval exactly after duration which is not the correct way, because the work of PART 1 may not necessarily get completed after the given duration because making frame every time requires some work and hence some time which may not be essentially instant.

I think the implementation should be like this

 totalRequiredFrames = fps*duration; // new code line

  const frameFactory = setInterval(() => {
    frames.push(makeFrame(fName + count, ext, cnv));
    count++;

   // new code line
    if(count > totalRequiredFrames){
      clearInterval(frameFactory);
     } 

  }, 1000 / fps);

@jesi-rgb
Copy link
Member

jesi-rgb commented Jun 7, 2022

I get the exact same behaviour. I would love to know from the team what are the exact intentions behind this function: if it was actually meant to save an arbitrary number of frames for a given animation and that means this is a bug, or if, on the other hand, there was a deliberate decision on capping the number of frames being processed so the sketch does not get blocked if there is a lot to process in a particularly busy sketch.

Can you help or guide us, @stalgiag? Or know someone that can?

@limzykenneth
Copy link
Member

@endurance21 What's happening is actually much simpler than that, p5.js limits the maximum frame rate to 22 and the maximum duration to 15, any values outside of this range will be clamped back down to 22 and 15. It isn't made very clear in the documentation and I think we should clarify it better.

@jesi-rgb As alluded to in the reference, saveFrames() is not meant to be used to save an arbitrary number of frames and if that's your intention you should look for other solutions such as ccapture.js. The main reason is that with sufficiently large canvas, saveFrames() can very easily blow through the available memory for the process, and throw out large numbers of save file dialogs, crashing even the browser.

@jesi-rgb
Copy link
Member

@limzykenneth gotcha! everything clear! we can actually change that real quick in the docs!

@Qianqianye Qianqianye added this to the 1.4.2 milestone Jul 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants