createImageFromImageBitmap should accept valid image sources other than ImageBitmap

[jezell]

Steps to reproduce

Use createImageFromImageBitmap with a video element.

Expected results

Don't throw exception when passed valid data.

https://developer.mozilla.org/en-US/docs/Web/API/createImageBitmap

This function supports all sorts of sources, not just ImageBitmaps, but the dart code has an unneeded instanceof check inserted. This should just be deleted, or it should be corrected to properly check against the valid types

 if (!domInstanceOfString(imageSource, 'ImageBitmap')) {
    throw ArgumentError('Image source $imageSource is not an ImageBitmap.', 'imageSource');
  }

HTMLImageElement
SVGImageElement
HTMLVideoElement
HTMLCanvasElement
Blob
ImageData
ImageBitmap
OffscreenCanvas

This will be blocking #144591 unless the video_player package manually calls the browser function and then passes the result forward:

    final JSAny tmp =
        await promiseToFuture<JSAny>(web.window.createImageBitmap(widget.element));
    final ui.Image img = await ui_web.createImageFromImageBitmap(tmp);
    

Actual results

Throws exception

Code sample

<video src='somesource' id='sample' />

import 'dart:web';
import 'dart:ui_web' as ui_web;

createImageFromImageBitmap(document.getElementById('sample')); // throws

Screenshots or Video

https://developer.mozilla.org/en-US/docs/Web/API/createImageBitmap

Logs

No response

Flutter Doctor output

n/a

[huycozy]

Could you please share a complete sample code so that we can verify this?

[jezell]

@huycozy

flutter/packages#6286

Implementing this pull request was the source of the bug. This code:

  final newSource =
        await promiseToFuture(web.window.createImageBitmap(widget.element))
            as web.ImageBitmap;
    final ui.Image img = await ui_web.createImageFromImageBitmap(newSource!);

  
    

Should be simply:

 final ui.Image = await ui_web.createImageFromImageBitmap(widget.element);
 

If you pull down the commit for the pull request, change your pubspec.yaml to reference the video player from pull request package instead of the pub.dev version, and make that change in video_player_web.dart:240 to remove the workaround, you'll see the crash with this sample:

import 'dart:async';

import 'package:flutter/material.dart';
import 'package:video_player/video_player.dart';
import 'package:video_player_web/video_player_web.dart';

void main() {
  runApp(const VideoPlayerApp());
}

class VideoPlayerApp extends StatelessWidget {
 const VideoPlayerApp({super.key});

 @override
 Widget build(BuildContext context) {
   return const MaterialApp(
     title: 'Video Player Demo',
     home: VideoPlayerScreen(),
   );
 }
}

class VideoPlayerScreen extends StatefulWidget {
 const VideoPlayerScreen({super.key});

 @override
 State<VideoPlayerScreen> createState() => _VideoPlayerScreenState();
}

class _VideoPlayerScreenState extends State<VideoPlayerScreen> {
 late VideoPlayerController _controller;
 late Future<void> _initializeVideoPlayerFuture;

 @override
 void initState() {
   super.initState();

   // Create and store the VideoPlayerController. The VideoPlayerController
   // offers several different constructors to play videos from assets, files,
   // or the internet.
   _controller = VideoPlayerController.networkUrl(
     Uri.parse(
       'https://flutter.github.io/assets-for-api-docs/assets/videos/butterfly.mp4',
     ),
   );

   // Initialize the controller and store the Future for later use.
   _initializeVideoPlayerFuture = _controller.initialize();

   // Use the controller to loop the video.
   _controller.setLooping(true);
 }

 @override
 void dispose() {
   // Ensure disposing of the VideoPlayerController to free up resources.
   _controller.dispose();

   super.dispose();
 }

 @override
 Widget build(BuildContext context) {
   return Scaffold(
     appBar: AppBar(
       title: const Text('Butterfly Video'),
     ),
     // Use a FutureBuilder to display a loading spinner while waiting for the
     // VideoPlayerController to finish initializing.
     body: FutureBuilder(
       future: _initializeVideoPlayerFuture,
       builder: (context, snapshot) {
         if (snapshot.connectionState == ConnectionState.done) {
           // If the VideoPlayerController has finished initialization, use
           // the data it provides to limit the aspect ratio of the video.
           return AspectRatio(
             aspectRatio: _controller.value.aspectRatio,
             // Use the VideoPlayer widget to display the video.
             child: VideoPlayer(_controller),
           );
         } else {
           // If the VideoPlayerController is still initializing, show a
           // loading spinner.
           return const Center(
             child: CircularProgressIndicator(),
           );
         }
       },
     ),
     floatingActionButton: FloatingActionButton(
       onPressed: () {
         // Wrap the play or pause in a call to `setState`. This ensures the
         // correct icon is shown.
         setState(() {
           // If the video is playing, pause it.
           if (_controller.value.isPlaying) {
             _controller.pause();
           } else {
             // If the video is paused, play it.
             _controller.play();
           }
         });
       },
       // Display the correct icon depending on the state of the player.
       child: Icon(
         _controller.value.isPlaying ? Icons.pause : Icons.play_arrow,
       ),
     ),
   );
 }
}

[eyebrowsoffire]

FYI, the ui_web API is specifically called createImagefromImageBitmap because it only actually accepts ImageBitmap objects, not other image sources. This was my first pass at this API and I kept it intentionally constrained for a couple of reasons:

• ImageBitmap objects are essentially immutable, as opposed to objects like HTMLVideoElement. With immutable objects like ImageBitmap, the rendering behavior won't change based on when we actually use the resource, but for mutable objects we may have subtle behavior changes if some of our rendering code defers use of the object, especially across an async boundary.

• ImageBitmap objects are transferable, but not all image sources are. This is particularly important for the experimental skwasm renderer which does its rendering on a web worker.

• createImageFromImageBitmap takes ownership of the image bitmap itself, and therefore will explicitly dispose of it when the engine is done with it. That is probably less likely to be the case for something like a HTMLVideoElement where the calling code probably retains ownership of the object after it has made the API call.

This was sort of a simplification decision for the first pass at this API. I could imagine ways that this contract could be more flexible, and the engine could do its own copying to an ImageBitmap object internally if it needed. But for the first pass, I decided to basically pass those concerns onto the calling code instead. Definitely open to revisiting this to make a richer API but since it involved some more nuanced design decisions I decided a smaller scoped change would be a good first step.

[jezell]

@eyebrowsoffire I think the issue is that no one needs to call this API with an ImageBitmap, so while it may make sense to constrain the API, people need to be able to get a VideoFrame, a VideoElement, a native SVG, etc. into flutter. Right now, that requires a double copy and calling the browser function on your own with js interop for every scenario that needs to use this API that I'm aware of (SVG, WebRTC, Video). Are there some other places where someone is attempting to use this API? Ok, it's doable, but it's wasteful and it results in extra code and dealing manually with disposal of a temporary ImageBitmap anyway.

While unfortunately you can't transfer DOM elements, for cases like media streaming there are solutions which let you use transferable objects like VideoFrame/WebCodecs where the API gives you a VideoFrame directly which can then be bound directly to a WebGL texture. VideoFrames are transferable resources and you really don't want to go to ImageBitmap, because the videos are often in a different color space like YUV and you want to get them to the GPU or over to the worker without a bunch of extra mutation and GC. Maybe #139271 could make this more efficient, but if you have to create image bitmaps for every frame just to get it into the texture, you're already potentially starting out in a bad spot from a GC perspective in a lot of browsers.

Right now, there's no pathway for anything that's not an ImageBitmap, which prevents doing these types of things as efficiently as they could be done. Certainly one approach of using a VideoFrame is createImageBitmap from the VideoFrame, but that often requires YUV->RGB conversion and extra memory being used so it's probably not the ideal path if it can be avoided.

Ideal end state would be a zero copy way to use the types of things I can get into a WebGL texture with zero copies into a flutter WebGL texture with zero copies. I get there are some constraints because of the decision to move the renderer into web workers, but it's a lot of extra hoops to jump through right now and extra bad in non chrome browsers that don't have as efficient pathways to move these resources around.

[eyebrowsoffire]

@jezell yep I agree with everything you're saying. The way the API is set up right now does end up causing extra unnecessary copies in some cases. Supporting VideoFrame itself should be straightforward and have the same semantics as ImageBitmap (immutable and transferable), so that might be a good place to start. But a separate API that doesn't transfer ownership might be needed for HTML elements.

[jezell]

@eyebrowsoffire wonder if exposing CanvasKit.MakeImageFromCanvasImageSource as an alternative would make it a little easier to avoid this and provide a more generic version?

https://github.com/google/skia/blob/main/modules/canvaskit/future_apis/ImageDecoder.md

If you always have to create an ImageBitmap to use this specific API, maybe it should accept ImageBitmap instead of JSAny in the signature to get rid of the confusion rather than doing a runtime check? Would also make the docs a little nicer since they could link straight to the ImageBitmap + createImageBitmap in the web package.

[huycozy]

@jezell Hmm, I'm following the given steps above, and running the example on Chrome but can't see the crash. I'm sure that I'm on video_player_web: 2.4.0 which is from your PR above.

[github-actions]

Without additional information, we are unfortunately not sure how to resolve this issue. We are therefore reluctantly going to close this bug for now.
If you find this problem please file a new issue with the same description, what happens, logs and the output of 'flutter doctor -v'. All system setups can be slightly different so it's always better to open new issues and reference the related ones.
Thanks for your contribution.

[eyebrowsoffire]

I don't think we should close this. This issue is a valid request for loosening the restrictions on createImageFromImageBitmap. I believe @jezell has worked around the issue in the package, hence why you are not reproducing the issue using the original repro steps. We don't need a repro case for this issue, it's actionable as-is.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.