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

canvas drawImage(videoElem...) cause memory leak via RequestFrameAnimation #582

Closed
3 tasks done
namello-zmtp opened this issue Oct 13, 2020 · 32 comments
Closed
3 tasks done

Comments

@namello-zmtp
Copy link

Please read first!

Please use Public Google Group (mailing list) for general technical discussions and questions.

  • I have used Google with the error message or bug in association with the library and Cordova words to make sure the issue I'm reporting is only related to iOSRTC.
  • I have provided steps to reproduce (e.g. sample code or updated extra/renderer-and-libwebrtc-tests.js file).
  • I have provided third party library name and version, ios, Xcode and plugin version and adapter.js version if used.

Note: If the checkboxes above are not checked (which you do after the issue is posted), the issue will be closed, removing this checkbox will result in automatic closed issue.

Versions affected

  • Cordova version (e.g 7.1.0): N/A
  • Cordova iOS version (e.g 5.1.0): N/A
  • Plugin version (e.g 6.0.12): Any
  • iOS version (e.g 10.2): Any
  • Xcode version (e.g 11.1 - 11A1027): Any
  • WebRTC-adapter version (e.g. 7.4.0): Any
  • WebRTC Framework version (e.g. JSSip 3.1.2): Any

Description

It appears that the plugin doesn't emit the necessary data for canvas drawImage(videoElem...) to function (capture the current frame basically) this prevents libraries like https://github.com/zxing-js from working.

Steps to reproduce

canvas: CanvasRenderingContext2D;
videoElem: HTMLVideoElement;

canvas.drawImage(videoElem, 0, 0); // Doesn't error but data isn't valid

Expected results

The frame should be copied over

Actual results

Nothing is copied over

@fimdomeio
Copy link

From what I see the only method to get a frame into canvas right now is use render.save() to an image element and then load that image into canvas. Works but sound a very bad idea. Also if I do this repeatedly I get flooded with THREAD WARNING: ['iosrtcPlugin'] took '85.138184' ms. Plugin should use a background thread. and it becomes impossible to debug the app.

If anyone knows of a better way to continuosly get camera frames into canvas in iOS I would love to know about it.

@calebboyd
Copy link
Contributor

calebboyd commented Oct 15, 2020

It's possible to capture the frame data from the existing buffer. But the image format is I420. It might be possible to send it to this canvas adapter.. https://github.com/brion/yuv-canvas

@fimdomeio
Copy link

Looks like this fork https://github.com/PeterXu/cordova-plugin-iosrtc has cordova.plugins.iosrtc.MediaStream() that can use a canvas. Did not had the oportunity to fully investigate this week, but will try ASAP.

@hthetiot
Copy link
Contributor

hthetiot commented Oct 16, 2020

@namello-zmtp This can already be done using existing iosRTC API, we could implement a SHAM/SHIM for canvas.drawimage but currently its not planned.

Example:

TLDR: only once video started playing via after "canplay" event access render property of the video tag and use save method with callback.

videoEl.render.save(function (data) {
    image.src = "data:image/jpg;base64," + data;
  });

@fimdomeio

Looks like this fork https://github.com/PeterXu/cordova-plugin-iosrtc has cordova.plugins.iosrtc.MediaStream() that can use canvas

It does and we may soon backport or colaborate with @PeterXu to put some of his work back in iosRTC, depending the time available from both contributors.

Notice that this fork does not contains all fixes we made since May 2019:

Also see related comment:

@hthetiot
Copy link
Contributor

Note: We can easily fix the warning "Plugin should use a background thread" for render.save.

@hthetiot
Copy link
Contributor

Its easy peasy in fact to add support for drawnimage if you understand how Javascript works.

Example:

// Apply CanvasRenderingContext2D.drawImage monkey patch
var drawImage = CanvasRenderingContext2D.prototype.drawImage;
CanvasRenderingContext2D.prototype.drawImage = function (arg) {
	var args = Array.prototype.slice.call(arguments);
	var context = this;
	if (arg instanceof HTMLVideoElement && arg.render) {
		arg.render.save(function (data) {
		    var img = new window.Image();
		    img.addEventListener("load", function () {
		    	args.splice(0, 1, img);
		        drawImage.apply(context, args);
		    });
		    img.setAttribute("src", "data:image/jpg;base64," + data);
	  	});
	} else {
		return drawImage.apply(context, args);
	}
};

I will make a PR for testing.

@hthetiot
Copy link
Contributor

@hthetiot hthetiot reopened this Oct 28, 2020
@hthetiot hthetiot modified the milestones: 6.0.15, 6.0.16 Oct 28, 2020
@hthetiot
Copy link
Contributor

Duplicate #116

@hthetiot
Copy link
Contributor

Fixed on master a98e6ad

@hthetiot
Copy link
Contributor

Related #594

@hthetiot hthetiot self-assigned this Oct 28, 2020
@hthetiot
Copy link
Contributor

document.body.innerHTML = "";

var canvas = document.createElement('canvas');
document.body.appendChild(canvas);

var video = document.createElement('video');
video.autoplay = true;
document.body.appendChild(video);

const onFrame = () => {
  // Resize canvas if video siwe change 
  if (
    canvas.width !== video.videoWidth ||
    canvas.height !== video.videoHeight
  ) {
    canvas.width = video.videoWidth;
    canvas.height = video.videoHeight;
  }
  const canvasContext = canvas.getContext('2d');
  canvasContext.drawImage(video, 0, 0);
  // setTimeout(onFrame, parseInt((1000/60)*25));
  window.requestAnimationFrame(onFrame);
};

// Refresh video tags position
if (window.cordova && window.cordova.plugins.iosrtc) {
  setInterval(function () {
    cordova.plugins.iosrtc.refreshVideos();
  }, 500); 
}

navigator.mediaDevices.getUserMedia({ 
  video: true, 
  audio: false 
}).then((stream) => {

  video.oncanplay = (event) => {
    window.requestAnimationFrame(onFrame);
  };

  video.srcObject = stream;

}).catch((error) => {
  console.log(error);
});

Tested and working on master.

@hthetiot
Copy link
Contributor

cc @gabrielenosso

@hthetiot
Copy link
Contributor

cordova plugin remove cordova-plugin-iosrtc --verbose
cordova plugin add https://github.com/cordova-rtc/cordova-plugin-iosrtc#master --verbose
cordova platform remove ios --no-save
cordova platform add ios --no-save

@gabrielenosso
Copy link

@hthetiot is this implementation performant?
I am asking cause you wrote yesterday that the current implementation was not.

If this is the case, this plugin is amazing and solves all problems on iOS, as you can have the video in a canvas and use it as a "common" DOM element.

@fimdomeio
Copy link

@gabrielenosso I did not try this, but had tried before the

videoEl.render.save(function (data) {
    image.src = "data:image/jpg;base64," + data;
  });

method. It did not seem performant at all. actually I could see the iphone killing the video for using too much resources.
I'm still going down the rabit hole with PeterXu fork which uses websockets to get the frames and send directly to a canvas. Seems to achieve a lot better performance.

@gabrielenosso
Copy link

@fimdomeio If you can give me some more input, I could try to go deeper.

@hthetiot
Copy link
Contributor

@fimdomeio If you can give me some more input, I could try to go deeper.

Try this tell us:

@hthetiot
Copy link
Contributor

hthetiot commented Oct 29, 2020

@gabrielenosso I did not try this, but had tried before the

videoEl.render.save(function (data) {
    image.src = "data:image/jpg;base64," + data;
  });

method. It did not seem performant at all. actually I could see the iphone killing the video for using too much resources.
I'm still going down the rabit hole with PeterXu fork which uses websockets to get the frames and send directly to a canvas. Seems to achieve a lot better performance.

Yes but we have SHIM for that that what we use for now.

const canvasContext = canvas.getContext('2d');
canvasContext.drawImage(video, 0, 0);

If new underlying implementation of the API change you should not care, the render.save may get more render methods to compare performance in future released

@fimdomeio
Copy link

@hthetiot, @gabrielenosso, Trying the example above appears to work quite well in terms of framerate. I see a continuous memory increase (until app crashes), that appear to be caused by canvasContext.drawImage(video, 0, 0);

@hthetiot
Copy link
Contributor

hthetiot commented Nov 6, 2020

See #594

I think there is a memory leaks...

@hthetiot
Copy link
Contributor

hthetiot commented Nov 8, 2020

@fimdomeio can you try this PR (see testing instruction), this should solve memory leak:

@hthetiot
Copy link
Contributor

hthetiot commented Nov 13, 2020

please confirm the fix #600 solve your issue @fimdomeio
I need this to release 6.0.16 we waiting for you.

See #582 (comment)
cc @namello-zmtp @gabrielenosso

@fimdomeio
Copy link

@hthetiot On it. Just stuck on xcode update and a not so fast internet connection. will give feedback ASAP.

@fimdomeio
Copy link

From my testing #600 does not fix the leak.
In the next few days I'll be posting a sample project so we can all be sure we're looking at the same code. It's pretty much the same as the example posted above just fixing some minor bugs.

@fimdomeio
Copy link

My sample test project is at https://github.com/fimdomeio/iosrtc-example

And the leak:
Captura de ecrã 2020-11-16, às 10 53 05

We got to the point where we realised that the memory leak disapears if in src/PluginMediaStreamRenderer.swift on the save function


we return "0"

We suspect the leak might be happening when the string is copied from swift to javascript

messageAs: based64

Are you able to reproduce the memory leak or need more detailed info?

@hthetiot
Copy link
Contributor

This help, keep you posted @fimdomeio

@hthetiot hthetiot modified the milestones: 6.0.16, 6.0.17 Nov 19, 2020
@hthetiot hthetiot changed the title Support canvas drawImage(videoElem...) canvas drawImage(videoElem...) cause memory leak via RequestFrameAnimation Nov 19, 2020
@hthetiot
Copy link
Contributor

I changed issue title cause 6.0.14 has drawImage(video) support, but its not usable via RequestFrameAnimation due memory leak. I need to release #605 first via 6.0.16

@hthetiot
Copy link
Contributor

memory leaks for video to canvas wil be addressed in 6.1.1

@hthetiot hthetiot modified the milestones: 6.1.1, 6.0.16 Nov 23, 2020
@hthetiot
Copy link
Contributor

Try master or wait for 6.0.17 see #609

@hthetiot hthetiot modified the milestones: 6.1.1, 6.0.17 Nov 27, 2020
@hthetiot hthetiot modified the milestones: 6.0.17, 6.0.x May 14, 2021
@hthetiot
Copy link
Contributor

Fix: #681
Related: #677

@hthetiot
Copy link
Contributor

@fimdomeio @namello-zmtp fixed in 6.0.21 and 8.0.0 via master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants