gaplessPlayback broken: Bad state: Cannot clone a disposed image.

[fryette]

🔙 Regression

@renefloor

Minimal reproducible sample with OctoImage(checked iOS only)

On the latest beta channel. What is more interesting on 1.22.0-12.1.pre everything was fine.

import 'dart:async';

import 'package:flutter/material.dart';
import 'package:octo_image/octo_image.dart';

void main() {
  runApp(MyApp());
}

class MyApp extends StatelessWidget {
  @override
  Widget build(BuildContext context) {
    return MaterialApp(
      title: 'Flutter Demo',
      home: MyHomePage(),
    );
  }
}

class MyHomePage extends StatefulWidget {
  @override
  _MyHomePageState createState() => _MyHomePageState();
}

class _MyHomePageState extends State<MyHomePage> {
  int index = 0;

  @override
  void initState() {
    super.initState();
    Timer.periodic(
      Duration(seconds: 2),
      (timer) {
        setState(() {
          ++index;
        });
      },
    );
  }

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: OctoImage(
        image:
            NetworkImage('https://via.placeholder.com/300.png/09f/fff?index=$index'),
        imageBuilder: (_, image) => image,
        placeholderBuilder: (_) => Container(color: Colors.red),
        gaplessPlayback: true,
      ),
    );
  }
}

[✓] Flutter (Channel beta, 1.26.0-17.6.pre, on macOS 11.2.1 20D75 darwin-x64, locale en) • Flutter version 1.26.0-17.6.pre at /Users/yauhen_sampir/fvm/versions/beta • Framework revision a29104a69b (9 days ago), 2021-02-16 09:26:56 -0800 • Engine revision 21fa8bb99e • Dart version 2.12.0 (build 2.12.0-259.12.beta)

[✓] Android toolchain - develop for Android devices (Android SDK version 29.0.3) • Android SDK at /Users/yauhen_sampir/Library/Android/sdk • Platform android-30, build-tools 29.0.3 • Java binary at: /Applications/Android Studio.app/Contents/jre/jdk/Contents/Home/bin/java • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6915495) • All Android licenses accepted.

[✓] Xcode - develop for iOS and macOS • Xcode at /Applications/Xcode.app/Contents/Developer • Xcode 12.4, Build version 12D4e • CocoaPods version 1.10.1

[✓] Chrome - develop for the web • Chrome at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome

[✓] Android Studio (version 4.1) • Android Studio at /Applications/Android Studio.app/Contents • Flutter plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/9212-flutter • Dart plugin can be installed from: 🔨 https://plugins.jetbrains.com/plugin/6351-dart • Java version OpenJDK Runtime Environment (build 1.8.0_242-release-1644-b3-6915495)

[✓] VS Code (version 1.53.2) • VS Code at /Applications/Visual Studio Code.app/Contents • Flutter extension version 3.9.0

Also can be reproduced using cached network image package https://github.com/Baseflow/flutter_cached_network_image/issues/513

[renefloor]

Thanks. Something is going wrong by the gaplessPlayback, setting that to false or removing the parameter results in the error disappearing. So it has something to do with the widget trying to show the old image again while loading the new one.

[renefloor]

@dnfield it looks like it broke because of this commit https://github.com/flutter/flutter/commit/2cdec25877dec1361c83d59510215687036b304e

The exception is this:

======== Exception caught by widgets library =======================================================
The following StateError was thrown building FadeWidget(dependencies: [_EffectiveTickerMode], state: _FadeWidgetState#40d18(ticker active)):
Bad state: Cannot clone a disposed image.
The clone() method of a previously-disposed Image was called. Once an Image object has been disposed, it can no longer be used to create handles, as the underlying data may have been released.

The relevant error-causing widget was: 
  OctoImage file:///Users/renefloor/Documents/Repos/textfield/lib/main.dart:44:13
When the exception was thrown, this was the stack: 
#0      Image.clone (dart:ui/painting.dart:1758:7)
#1      RawImage.createRenderObject (package:flutter/src/widgets/basic.dart:5760:21)
#2      RenderObjectElement.mount (package:flutter/src/widgets/framework.dart:5439:28)
...     Normal element mounting (15 frames)
#17     Element.inflateWidget (package:flutter/src/widgets/framework.dart:3623:14)
...
====================================================================================================

The OctoImage widget calls the Flutter Image widget with an ImageProvider. When the image widget indicates it is done loading it allows the OctoImage user to modify that widget using the ImageBuilder and that result is shown.

Using gaplessPlayback that first widget is shown while a new image is being loaded. However, that widget reuses the image which is not possible anymore. If I just store the first ImageProvider instead of the first Widget everything works just fine, however I lose the information of possible transformations that were applied on the Image widget as I build a new widget. This is the potential fix: https://github.com/baseflow/octo_image/commit/7f815b8e1f17ef49e482b865f558dddd70f104fd

However, that fix introduces a new problem.

@dnfield do you have any idea how I can reuse the widget here? Maybe I can store both the ImageProvider as the ImageBuilder function so the widget can be recreated as well, but I'm not sure if that is a good way as we might call a function on a widget that is already disposed.

[renefloor]

As I already feared, it is broken on Flutter 2.0 stable :(

[dnfield]

If you want to refer to an image, you have to make sure you've cloned it and then disposed of it when you're done. Otherwise, you might be using a disposed reference and find that it is no longer valid.

Using image providers directly is probably the right way to go, but if you need the reference to the image to stay alive you'll have to manage your own listening on that provider and make sure you clone/dispose the image appropriately.

[renefloor]

Thanks for the response @dnfield. I think I mis-worded maybe some sentences. I prefer to not keep track of the image itself, but the widget that contains the image. I put the old ImageWidget on a stack and fade it out, I put the new ImageWidget on that stack and fade in. It is practically the widget returned by the FrameBuilder:

 Image(
        image: widget.image,
        frameBuilder: frameBuilder,
      ),

I use that in a stack like this:

  Widget _stack(Widget revealing, Widget disappearing) {
    return Stack(
      fit: StackFit.passthrough,
      alignment: Alignment.center,
      children: [
        FadeWidget(
          child: revealing,
          duration: widget.fadeInDuration,
          curve: widget.fadeInCurve,
        ),
        FadeWidget(
          child: disappearing,
          duration: widget.fadeOutDuration,
          curve: widget.fadeOutCurve,
          direction: AnimationDirection.reverse,
        )
      ],
    );
  }

The Image widget is first added as 'revealing', but when you load a different image the widget we had gotten from framebuilder is now used as 'disapearing'. I guess I need to use the main Image widget instead of the widget I get in 'frameBuilder'. Would that help?

Edit: nvm, that last suggestion breaks the whole flutter tree of course.. :(

Build functions must never return their BuildContext parameter's widget or a child that contains
"context.widget". Doing so introduces a loop in the widget tree that can cause the app to crash.

Edit2: using that Image widget directly not in the builder, but as the placeholder does work and not break, so maybe that is promising.

Edit3: Doing this indeed results in applying the new imageBuilder to the old image again. This example shows the index on top of the image:

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      body: OctoImage(
        image:
        NetworkImage('https://via.placeholder.com/30$index.png/09f/fff'),
        imageBuilder: (url, image) {
          return Stack(
            alignment: Alignment.bottomCenter,
            children: [
              image,
              Text(index.toString()),
            ],
          );
        },
        placeholderBuilder: (_) => Container(color: Colors.red),
        gaplessPlayback: true,
      ),
    );
  }

This result in:

At least this is better than being broken, but still not optimal.

This is the proposed change: http://github.com/baseflow/octo_image/commit/389b6b46d16a4df207dbe5ec56d8c38912e6954d

[dnfield]

Pulling the image provider out of the widget isn't really safe.

[renefloor]

What do you mean with pulling it out of the widget? The ImageProvider is given to OctoImage by the user of the library. OctoImage then creates an image widget using that ImageProvider. So we already have the ImageProvider.

[dnfield]

Sorry, I think I was replying to an earlier comment. Also, sorry, but I'm dealing with a newborn and not sleeping much so I'm not sure I'm fully coherent here haha.

The basic change is that if you want to paint an image (or give it to something else that will paint it), you need to make sure that you're holding on to a clone of it. Otherwise, someone else can dispose the native data out from under you. Fishing an image provider out of an Image widget is probably not safe, since you don't know if that widget's state has been disposed and thus it disposed the underlying image.

If you're giving it the image provider, you shoudn't need to use widget.image in your code.

If OctoImage needs to keep the image provider's image around, it should be a listener on the provider and making sure that it creates and disposes of clones appropriately.

[fryette]

@renefloor do you have any ideas on how to fix it?

[renefloor]

We just can't use the result from the imageBuilder (from the Flutter Image widget) as the Image Widget itself might be disposed. So I first thought of just keeping the old image widget or rebuilding that with the OctoImage imagebuilder, but that means the caller has to check whether it tries to build the new or old url. These are both not ideal.

I think we need to manage the loading of the imageprovider ourselves, practically forking the Flutter Image widget. I have to dive into this a bit better, but I've had a really busy week. I'll hope to find time for this soon, but it's no easy fix.

[renefloor]

Add @fryette I think I fixed it in PR #18. Can you verify this PR? I'm going to write tests for it now.

[fryette]

@renefloor Will do it and let you know. Today/tommorow

Thanks!

[fryette]

@renefloor Can confirm that issue completely fixed