[Bug]: Video deserealization gives Error: fabric: Error loading but video is working

celioFagundes commented 2 months ago

CheckList

[X] I agree to follow this project's Code of Conduct
[X] I have read and followed the Contributing Guide
[X] I have read and followed the Issue Tracker Guide
[X] I have searched and referenced existing issues and discussions
[X] I am filing a BUG report.
[X] I have managed to reproduce the bug after upgrading to the latest version
[X] I have created an accurate and minimal reproduction

Version

6.0.2

In What environments are you experiencing the problem?

No response

Node Version (if applicable)

20.11.0

Link To Reproduction

https://codesandbox.io/p/sandbox/fabricjs-load-video-2752r8?file=%2Fsrc%2Ftest-json.ts

Steps To Reproduce

When using v5.3.0, i was using this method to deserealize a video element, which references are this issue https://github.com/fabricjs/fabric.js/issues/3697 and this post https://itnext.io/video-element-serialization-and-deserialization-of-canvas-fc5dbf47666d. It works on 5.3.0 when saving a canvas with a video element and loading it after, but in v6 it gives this error when loadingUncaught (in promise) Error: fabric: Error loading data:video/mp4;base64...,, even though if you close the error popup the video is working correctly. I created a codesandbox to demonstrate it on both version, the v6 takes some time to load and can crashes sometimes.

Codesandbox v6: https://codesandbox.io/p/sandbox/fabricjs-load-video-2752r8?file=%2Fsrc%2Ftest-json.ts CodesandBox v5.3.0: https://codesandbox.io/p/sandbox/fabric-load-video-5-3-0-2mcxrg?file=%2Fsrc%2FApp.tsx

This is the code to deserealize it in v6.0.2 , the 5.3.0 has only syntax differences

const getVideoElement = (url: string, height: number, width: number) => {
    const videoE = document.createElement("video");
    videoE.width = height;
    videoE.height = width;
    videoE.muted = true;
    videoE.src = url;
    videoE.crossOrigin = "anonymous";
    const source = document.createElement("source");
    source.src = url;
    source.type = "video/mp4";
    videoE.appendChild(source);
    return videoE;
  };

  const canvasLoaded = async (canvasJson: any) => {
    if (!canvas) return;
    const objs = canvasJson.objects;
    for (let i = 0; i < objs.length; i += 1) {
      const obj = objs[i];
      if (obj.data?.isVideo) {
        const videoE = getVideoElement(obj.src, obj.height, obj.width);
        const fab_video = new FabricImage(videoE, {
          objectCaching: true,
          left: obj.left,
          top: obj.top,
          height: obj.height,
          width: obj.width,
          scaleX: obj.scaleX,
          scaleY: obj.scaleY,
          data: obj.data,
        });

        const videoCanvasElement = fab_video.getElement() as HTMLVideoElement;
        videoCanvasElement.currentTime = 0.1;
        videoCanvasElement.play();
        videoCanvasElement.loop = true;

        canvas.add(fab_video);
        util.requestAnimFrame(function render() {
          canvas.renderAll();
          util.requestAnimFrame(render);
        });
      }
    }
  };

  const loadCanvas = async () => {
    if (!canvas) return;
    canvas.loadFromJSON(testJson, () => {
      canvasLoaded(testJson);
    });

    canvas.set("height", testJson.height);
    canvas.set("width", testJson.width);
    canvas.requestRenderAll();
  };

Expected Behavior

Video should loading without error

Actual Behavior

Fabric shows error even if the video is actually working

Error Message & Stack Trace

Uncaught (in promise) Error: fabric: Error loading data:video/mp4;base64...., at Bt.n.onerror (index.min.mjs:76:14943)

zhe-he commented 2 months ago

Visitors do not have permission to access this demo

celioFagundes commented 2 months ago

Visitors do not have permission to access this demo

Sorry, fixed the permission now

zhe-he commented 2 months ago

@celioFagundes

I found that the image loading part is different from the old version. In the old version, it would return even if it failed, but the new version does not return. I have rewritten the image loading method specifically for your business scenario. Here is an example. Regarding the new version, well... the perfect solution is that you should inherit from FabricImage to create a new object A, and then all videos should be this object A, rather than rewriting the Image object like in the old version

v6 fix demo

celioFagundes commented 2 months ago

I see, you mean creating custom Video Object inherinting from Image? I tested the fix and i found that the default images do not work, https://codesandbox.io/p/sandbox/fabricjs-load-video-forked-qxwdpk?file=%2Fsrc%2FApp.tsx%3A8%2C1-9%2C1 , changed the codesandbox to test it, if you removed the canvas with fix the other canvas with the image loads correctly, but if you render the canvas with fix it breaks the image. Also, this line of code gives this error Uncaught (in promise) DOMException: play() failed because the user didn't interact with the document first, if you load the canvas on render without the user clicking a button, i fixed it by removing this line and adding the current time of the video to 0.1 to show the first frame , without the video would be blank at start, i couldn't replicate this error on sandbox i believe it activates a user interaction somehow

  if ("play" in elementToDraw) (elementToDraw as HTMLVideoElement).play();
  elementToDraw &&
    ctx.drawImage(elementToDraw, sX, sY, sW, sH, x, y, maxDestW, maxDestH);

const loadImage = (url: string, { signal, crossOrigin = null }: util.LoadImageOptions = {}) =>
  new Promise<HTMLImageElement>(function (resolve, reject) {
    if (signal && signal.aborted) {
      return reject(new Error('loadImage'));
    }
    const isMp4 = url.endsWith('.mp4') || url.startsWith('data:video/mp4;base64,');
    const img = document.createElement(isMp4 ? 'video' : 'image') as HTMLImageElement;
    let abort: EventListenerOrEventListenerObject;
    if (signal) {
      abort = function (err: Event) {
        img.src = '';
        reject(err);
      };
      signal.addEventListener('abort', abort, { once: true });
    }
    const done = function () {
      img.onload = img.onerror = img.onloadeddata = null;
      abort && signal?.removeEventListener('abort', abort);
      resolve(img);
    };
    if (!url) {
      done();
      return;
    }
    img.onloadeddata = done;
    img.onload = done;
    img.onerror = function () {
      abort && signal?.removeEventListener('abort', abort);
      reject(new Error(`Error loading ${img.src}`));
    };
    crossOrigin && (img.crossOrigin = crossOrigin);
    img.src = url;
    img.currentTime = 0.1;
  });

zhe-he commented 2 months ago

Videos are not allowed to autoplay due to browser restrictions. Hmm... How about placing a play icon and adding a click event to the video object so that it plays when the user clicks on it? Regarding the image bug, the rewrite did not take the original image object into account. Use a new class to inherit from the image object, so that all requirements can be met.

tonyabracadabra commented 3 weeks ago

Is there any solution to this? I tried this video demo https://fabricjs.com/video-element on fabric v6 but it throws a similar error

zhe-he commented 3 weeks ago

@tonyabracadabra This is a template, not a perfect solution. The perfect solution is to inherit from FabricImage to create a new object A.

demo

fabricjs / fabric.js