gdkpixbuf: Add support for loading image sequences

[Hiers]

A working patch implementing image sequences in the gdkpixbuf module.

I'm not familiar with GLib/GTK, so be extra careful with merging this as it might have some beginner bugs.

[wantehchang]

Hiers: Thank you for your contribution. We don't maintain the gdk-pixbuf code (which is why it's in the contrib/ directory). I will ask the author to review your pull request.

@novomesk Daniel: Could you take a look at this? Thanks.

[novomesk]

This is good feature enhancement. I was thinking to focus adding animation anyway.

However, I will need more time to understand, check and test the code. Maybe I will add some tweaks afterwards.

@wantehchang Is the new release planned soon? I don't like to add big changes close to the release.

[wantehchang]

Daniel: We don't have a plan to make a new libavif release soon, say in the next two weeks.

[Hiers]

I have an example here of a file that will eventually fail. It has no problems when ran through ffplay. It's the only file I've created from a video (webm) and the only avif image sequence that I've seen fail. It could be hitting an edge case that I'm not handling in my patch.

[novomesk]

What does it mean that the yipyipyipyip.avif fails?

[Hiers]

When I open that animation, after looping a few times it will either disappear for a moment with assertion 'G_IS_OBJECT (object)' failed and assertion 'GDK_IS_PIXBUF (pixbuf)' failed error messages before recovering, or more rarely just segfault.

[novomesk]

What application/version you are using to play animation, so I can try to reproduce the crashes?

[Hiers]

xviewer version 3.2.12

[novomesk]

I am able to reproduce the problem with EOG application and with different AVIF animation too.

(eog:27980): GLib-GObject-CRITICAL **: 14:53:29.510: g_object_ref: assertion 'G_IS_OBJECT (object)' failed

(eog:27980): GdkPixbuf-CRITICAL **: 14:53:29.510: gdk_pixbuf_get_width: assertion 'GDK_IS_PIXBUF (pixbuf)' failed

(eog:27980): GdkPixbuf-CRITICAL **: 14:53:29.510: gdk_pixbuf_get_height: assertion 'GDK_IS_PIXBUF (pixbuf)' failed

(eog:27980): Gdk-CRITICAL **: 14:53:29.510: gdk_cairo_surface_create_from_pixbuf: assertion 'GDK_IS_PIXBUF (pixbuf)' failed

(eog:27980): GLib-GObject-CRITICAL **: 14:53:29.564: g_object_unref: assertion 'G_IS_OBJECT (object)' failed

[novomesk]

In GdkPixbuf* set_pixbuf(AvifAnimation * context, GError ** error) there are several return FALSE;
I think that's old code. Now it should be return NULL;?

[Hiers]

I am able to reproduce the problem with EOG application and with different AVIF animation too.

(eog:27980): GLib-GObject-CRITICAL **: 14:53:29.510: g_object_ref: assertion 'G_IS_OBJECT (object)' failed

(eog:27980): GdkPixbuf-CRITICAL **: 14:53:29.510: gdk_pixbuf_get_width: assertion 'GDK_IS_PIXBUF (pixbuf)' failed

(eog:27980): GdkPixbuf-CRITICAL **: 14:53:29.510: gdk_pixbuf_get_height: assertion 'GDK_IS_PIXBUF (pixbuf)' failed

(eog:27980): Gdk-CRITICAL **: 14:53:29.510: gdk_cairo_surface_create_from_pixbuf: assertion 'GDK_IS_PIXBUF (pixbuf)' failed

(eog:27980): GLib-GObject-CRITICAL **: 14:53:29.564: g_object_unref: assertion 'G_IS_OBJECT (object)' failed

Is there anything in common with the files that cause this problem?

In GdkPixbuf* set_pixbuf(AvifAnimation * context, GError ** error) there are several return FALSE; I think that's old code. Now it should be return NULL;?

Yeah, good catch. Unfortunately, the old bug still persists after I make these changes.

[novomesk]

In avif_animation_iter_get_pixbuf you are returning .pixbuf from array.

avif_iter->current_frame used as index is out-of-bounds. Sometimes it has value 12. Valid numbers of frames in yipyipyipyip.avif are in range 0-11.

In avif_context_try_load you are loading all frames from animation into memory/array.

It would be fine to add error checking after each frame if frame.pixbuf is not NULL.

static GdkPixbufModulePattern signature[] can be extended to contain ftypavis in header too.

I see the approach used here is to load all frames into memory and then to return cached pixels. I used similar approach in my Qt plug-ins in the past. Problem was that large+long animations too noticeable long time at the beginning to decode and memory consumption grows quickly.
AVIF animation can be like Full HD movie with several hundreds of thousands of frames. Users would not be happy if we attempt to store all uncompressed frames into the memory.
What about some reasonable limit to avoid too high memory consumption?
Or would you like to change the behavior to decode frames when they are needed?

[Hiers]

You got me on the right path to fix that bug. The comment I left on whether the precision loss of me converting that double to an unsigned int was somewhat prescient, as that was what was causing problems. I've changed how the total time of the animation in milliseconds is used and it seems to have fixed it.

I was hoping the way I was loading everything at once wouldn't be a problem because I've never seen an avif/gif/etc animation that was more than a few seconds long, but I'll try a more sensible approach.

[novomesk]

I am sending you a large animation:
http://188.121.162.14/avif/japanese-village.avif

[Hiers]

My current issue with having a reasonable limit or decoding frames as they are needed is that any decoding done after the animation starts playing will affect it. Whether it has small constant stutters when decoding one frame, or fewer bigger stutters when decoding a larger set of frames.

I can only think of offloading the decoding work to a separate thread, but I didn't want to go that route if there is a better solution.

[novomesk]

I like the idea with decoder thread and cache of few future frames. Of course, it is most difficult to implement and test.

[Hiers]

I implemented the decoder thread. I have two issues, one of which is not knowing if using the GAsyncQueue is a good idea in this situation. The other is the forsaken size_func call in line 387 that will print a lot of errors and generally make the whole thing not work.

[Hiers]

Also, what do you think of the buffer size? It looks very small when used for low res animations, but will quickly grow if used with high quality 1080p animations.

Instead of the buffer being X frames, it might be better to make it be X MB instead. But maybe it's not a big deal.

[novomesk]

Buffer with 64 frames means more than 1 second of 60fps video. I think it is enough.

Animations with small dimensions decode quickly and typically it can play smoothly also without buffering.

Note: I observed that avifImageYUVToRGB call sometimes take relatively significant amount of time. In my software I switch to rgb.chromaUpsampling = AVIF_CHROMA_UPSAMPLING_FASTEST; when decoding animation to avoid lags on playback.

[Hiers]

Ok, if you're happy with the PR, I'm happy with it as it is.

[novomesk]

When I exit eog I see following message:

g_object_unref: assertion 'old_ref > 0' failed

Here is a testfile http://188.121.162.14/avif/repetition3.avif with Repeat Count 3 (plays 4 times totally in browsers). repetition3.avif plays infinitely in eog. Is it possible to make it to play only 4 times in eog?

[Hiers]

I've fixed it, but EOG and EOG forks have a bug with files that have a repeat count > 0.

Since once it stops repeating the pixbuf will never need to be updated and EOG is not expecting this, it will be stuck in a while loop and the GUI will stop responding.

But this is not something on the pixbuf module's end.

[novomesk]

Hmm, eog doesn't have problem when GIF has limited repeat count. I believe there must be a way how to do it without hitting 100% CPU utilization at the end.

Perhaps we can find inspiration in their sources? https://gitlab.gnome.org/GNOME/gdk-pixbuf/-/tree/master/gdk-pixbuf

[Hiers]

I've rummaged in the code a bit, and the reason EOG and its forks work with gif is because they never even run the code that will loop incessantly.

For some reason, a gif (or at least the gif you gave here as an example) has the delay time for the last frame of the last loop as a very very big number. It might hit the bug I've mentioned once that ridiculous timer runs out, but I didn't care to wait.

[Hiers]

In short, EOG does have a bug with animations that have a set number of loops, but gifs work in a way that hide that bug.

[novomesk]

Are you willing to explain the problem to the EOG developers?
https://gitlab.gnome.org/GNOME/eog/-/issues

In case they don't fix I suggest we always loop the animation infinitely.

[wantehchang]

@Hiers @novomesk Hi Hiers and Daniel,

I was just wondering if this pull request is ready to be merged. The last comment was on May 21, and the code review doesn't seem to be finished.

[novomesk]

I did not forget about this PR.

Lot of work was done and valuable experiences were gathered. We have valuable prototype.

But I would not merge yet. I intend to inspect the code more deeply when I have more time.

[Hiers]

The big problem we had with a bug in EOG and EOG forks that led to an animation using 100% of a cpu core when the animation finished has been fixed.

That was the last thing I was waiting on. As for me, this can get merged.

[Hiers]

@novomesk I refactored a function that was a bit too big to be easily understandable and also changed the get_delay function to always return a value following the documentation. If you've got time, check if this is ready to be merged.

[novomesk]

clang compiler doesn't like it:

libavif/contrib/gdk-pixbuf/loader.c:170:83: error: implicit conversion loses integer precision: 'size_t' (aka 'unsigned long') to 'guint' (aka 'unsigned int') [-Werror,-Wshorten-64-to-32]
            context->frames = g_array_remove_range(context->frames, 0, avif_iter->current_frame);
                              ~~~~~~~~~~~~~~~~~~~~                     ~~~~~~~~~~~^~~~~~~~~~~~~
libavif/contrib/gdk-pixbuf/loader.c:211:106: error: implicit conversion loses integer precision: 'uint64_t' (aka 'unsigned long') to 'int' [-Werror,-Wshorten-64-to-32]
        return g_array_index(avif_iter->animation->frames, AvifAnimationFrame, avif_iter->current_frame).duration_ms;
        ~~~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~

And in avif_animation_finalize can you first wait for the decoder thread to finish and allocate other things afterwards?

[Hiers]

Thanks, I never thought of catching errors by running clang on it.

[novomesk]

I am getting crashes in eog after playing AVIF animation for few seconds.

The first crash:

(eog:16000): GLib-CRITICAL **: 17:53:40.404: g_async_queue_pop: assertion 'queue' failed

(gdb) bt 
#0  0x00007ffff058e1b6 in decoder_thread () at /usr/lib64/gdk-pixbuf-2.0/2.10.0/loaders/libpixbufloader-avif.so
#1  0x00007ffff7c123cd in g_thread_proxy () at /usr/lib64/libglib-2.0.so.0
#2  0x00007ffff70ad2b9 in start_thread () at /lib64/libc.so.6
#3  0x00007ffff713030c in clone3 () at /lib64/libc.so.6

The second crash:

(eog:16312): GLib-GObject-CRITICAL **: 17:56:07.632: g_object_ref: assertion 'G_IS_OBJECT (object)' failed

(eog:16312): GLib-GObject-CRITICAL **: 17:56:07.632: g_object_ref: assertion 'G_IS_OBJECT (object)' failed

(eog:16312): GdkPixbuf-CRITICAL **: 17:56:07.632: gdk_pixbuf_get_width: assertion 'GDK_IS_PIXBUF (pixbuf)' failed

(eog:16312): GdkPixbuf-CRITICAL **: 17:56:07.632: gdk_pixbuf_get_height: assertion 'GDK_IS_PIXBUF (pixbuf)' failed

(eog:16312): Gdk-CRITICAL **: 17:56:07.632: gdk_cairo_surface_create_from_pixbuf: assertion 'GDK_IS_PIXBUF (pixbuf)' failed

(eog:16312): GdkPixbuf-CRITICAL **: 17:56:07.633: gdk_pixbuf_get_width: assertion 'GDK_IS_PIXBUF (pixbuf)' failed

(eog:16312): GdkPixbuf-CRITICAL **: 17:56:07.633: gdk_pixbuf_get_height: assertion 'GDK_IS_PIXBUF (pixbuf)' failed

(eog:16312): GdkPixbuf-CRITICAL **: 17:56:07.633: gdk_pixbuf_get_has_alpha: assertion 'GDK_IS_PIXBUF (pixbuf)' failed

Thread 1 "eog" received signal SIGSEGV, Segmentation fault.
0x00007ffff7d0bd7d in g_type_check_instance_is_fundamentally_a () from /usr/lib64/libgobject-2.0.so.0
(gdb) bt
#0  0x00007ffff7d0bd7d in g_type_check_instance_is_fundamentally_a () at /usr/lib64/libgobject-2.0.so.0
#1  0x00007ffff7cec685 in g_object_unref () at /usr/lib64/libgobject-2.0.so.0
#2  0x00007ffff7f89a82 in update_pixbuf () at /usr/lib64/eog/libeog.so
#3  0x00007ffff7f8bd27 in display_next_frame_cb () at /usr/lib64/eog/libeog.so
#4  0x00007ffff7ce76d0 in g_closure_invoke () at /usr/lib64/libgobject-2.0.so.0
#5  0x00007ffff7cfac8c in signal_emit_unlocked_R.isra.0 () at /usr/lib64/libgobject-2.0.so.0
#6  0x00007ffff7cfc4eb in signal_emit_valist_unlocked () at /usr/lib64/libgobject-2.0.so.0
#7  0x00007ffff7d01dc2 in g_signal_emit_valist () at /usr/lib64/libgobject-2.0.so.0
#8  0x00007ffff7d01e77 in g_signal_emit () at /usr/lib64/libgobject-2.0.so.0
#9  0x00007ffff7f77d7d in private_timeout () at /usr/lib64/eog/libeog.so
#10 0x00007ffff7be5242 in g_timeout_dispatch () at /usr/lib64/libglib-2.0.so.0
#11 0x00007ffff7be146a in g_main_dispatch () at /usr/lib64/libglib-2.0.so.0
#12 0x00007ffff7be45a7 in g_main_context_iterate_unlocked.constprop () at /usr/lib64/libglib-2.0.so.0
#13 0x00007ffff7be4b9c in g_main_context_iteration () at /usr/lib64/libglib-2.0.so.0
#14 0x00007ffff7e183cd in g_application_run () at /usr/lib64/libgio-2.0.so.0
#15 0x00005555555552f0 in main ()

[Hiers]

The first crash should be fixed, but I'm surprised a NULL pointer de-reference didn't cause a segfault unless it was ran in gdb...

I hope this commit fixes the second crash, because I've no idea what caused it and can't reproduce it. The backtrace doesn't even include anything specific to our .so.

[novomesk]

Now it doesn't crash but playback stops after few seconds. When the animation stops, these messages appear:

(eog:22326): GLib-GObject-CRITICAL **: 16:48:41.650: g_object_ref: assertion 'G_IS_OBJECT (object)' failed

(eog:22326): GLib-GObject-CRITICAL **: 16:48:41.650: g_object_ref: assertion 'G_IS_OBJECT (object)' failed

(eog:22326): GdkPixbuf-CRITICAL **: 16:48:41.650: gdk_pixbuf_get_width: assertion 'GDK_IS_PIXBUF (pixbuf)' failed

(eog:22326): GdkPixbuf-CRITICAL **: 16:48:41.650: gdk_pixbuf_get_height: assertion 'GDK_IS_PIXBUF (pixbuf)' failed

(eog:22326): Gdk-CRITICAL **: 16:48:41.650: gdk_cairo_surface_create_from_pixbuf: assertion 'GDK_IS_PIXBUF (pixbuf)' failed

(eog:22326): GdkPixbuf-CRITICAL **: 16:48:41.659: gdk_pixbuf_get_width: assertion 'GDK_IS_PIXBUF (pixbuf)' failed

(eog:22326): GdkPixbuf-CRITICAL **: 16:48:41.659: gdk_pixbuf_get_height: assertion 'GDK_IS_PIXBUF (pixbuf)' failed

(eog:22326): GdkPixbuf-CRITICAL **: 16:48:41.659: gdk_pixbuf_get_has_alpha: assertion 'GDK_IS_PIXBUF (pixbuf)' failed

[Hiers]

Well, I changed something, but I don't think it's going to fix this bug. I can't for the life of me replicate it.

If this didn't fix it for you, can you run it again and try to get some more debugging information? I'm at a loss at what is going wrong.

[novomesk]

I'll look into it a bit later when I have more time.

[Hiers]

Fixed what I think is the most likely culprit of the messages you were getting. It's related to a race condition between the thread displaying the image and the thread decoding the avif file; There's a potential situation where the first thread will go beyond what is in the buffer if the decoder lags behind.

Getting this merged will be a pain though, with #1907 touching so many lines.