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

Text gets dropped on some frames intermittently. #686

Closed
bmatherly opened this Issue Dec 30, 2018 · 12 comments

Comments

Projects
None yet
2 participants
@bmatherly
Copy link
Member

bmatherly commented Dec 30, 2018

Original Report:
https://forum.shotcut.org/t/text-flickering-to-left-after-export/8910

@ddennedy I might need your help with this one. I can only reproduce this problem on Windows using the released package. Also, I only reproduce the problem on export. I do not see the problem in the preview window nor in Linux under any circumstances. I think there might be an intermittent race condition.

To reproduce:

  1. Set video mode to 1080p30 (I do not know if this is necessary, but it is what I do)
  2. Open a clip
  3. Add the text filter (the default settings put a timecode on the bottom of the image)
  4. Export using default settings
  5. Open the exported file and watch.
    The text will intermittently flicker. If you step by frames in Shotcut, you can see that every few seconds a video frame will not have the text at all.

I downloaded the 18.12.23 Windows SDK. When I run shotcut from QT creator I am not able to reproduce the flickering on export.

I added a debug message here:
https://github.com/mltframework/mlt/blob/master/src/modules/qt/filter_qtext.cpp#L289
But as I mentioned, I never see the problem in the preview window. I haven't taken the time to figure out how to compile qmelt. But as I mentioned, the problem does not occur when I run from QT.

Can you try to reproduce the problem on some of your systems? Hopefully, between the two of us, we can find an environment that reproduces the problem more reliably.

@ddennedy

This comment has been minimized.

Copy link
Member

ddennedy commented Jan 2, 2019

So far I have not reproduced it. I do not think you need to re-compile qmelt. You can easily try to build MLT from the SDK using the same flags (minus --enable-debug) as the release build. I run tail -2 config.log and then re-run configure by converting the paths to my locations (C:\Projects\Shotcut). Then, you can run make distclean, followed by reconfigure, make -j all, make install. If you can reproduce with the SDK shotcut.exe after that then you can tinker with placement of mlt_service_lock()/mlt_service_unlock() calls placed in qtext to see if the problem goes away. I will continue to try to reproduce it off-and-on. (I also tried on macOS with no reproduction.)

@ddennedy

This comment has been minimized.

Copy link
Member

ddennedy commented Jan 2, 2019

I reproduced the text alignment bug here, and it is probably the same race condition:
https://forum.shotcut.org/t/text-alignment-after-rendering/8963/

A big problem is that qtext is inspecting filter properties inside its get_image routine while the dynamictext filter is setting them in its process routine. There is a technique to create a unique properties object that I will lookup and try to employ.

@ddennedy ddennedy added this to the v19.01 milestone Jan 3, 2019

@ddennedy

This comment has been minimized.

Copy link
Member

ddennedy commented Jan 3, 2019

I think this is fixed in mltframework/mlt@0e2044f
At least what I could reproduce with horizontal alignment was fixed. You can test a release build tomorrow for the text dropouts.

@bmatherly

This comment has been minimized.

Copy link
Member

bmatherly commented Jan 3, 2019

I see what you did there. I think there could still be a microscopic probability of a race condition if the thread context changes after the function returns but before the pointer is dereferenced. I will try it tomorrow and report back.

If we still need more protection, I think we could put a lock around everything except for drawPath since that is where all the heavy lifting takes place.
https://github.com/mltframework/mlt/blob/master/src/modules/qt/filter_qtext.cpp#L225

@ddennedy

This comment has been minimized.

Copy link
Member

ddennedy commented Jan 3, 2019

I already put a big lock around the majority of the body of qtext get_image, but it did not work. Some lock in there may need to be combined with a lock on the text filter inside dynamictext's process.

ddennedy added a commit to mltframework/mlt that referenced this issue Jan 3, 2019

@ddennedy

This comment has been minimized.

Copy link
Member

ddennedy commented Jan 3, 2019

I was not able to get service locks to work, but I did get mlt_frame_unique_properties() to work. Please review the following branch:
mltframework/mlt@2183778

I removed the encoding property because it is not available on the unique properties and the MLT convention is that all strings are UTF-8 and should be converted to that before setting or converted from that after getting.

@bmatherly

This comment has been minimized.

Copy link
Member

bmatherly commented Jan 4, 2019

How did I never know about mlt_frame_unique_properties()? This is the superior solution. There are probably many other services that would benefit from using this technique. In fact, we should almost require it and forbid services from accessing properties directly from their image/audio callbacks.

@bmatherly

This comment has been minimized.

Copy link
Member

bmatherly commented Jan 4, 2019

I will test tomorrow after the nightly build. But I feel confident this will resolve the problem.

@ddennedy

This comment has been minimized.

Copy link
Member

ddennedy commented Jan 4, 2019

I tested it by reverting the previous commit where I can easily reproduce it.

As for applying this to other filters, yes it is a good way to remove locks. I think I should add a mlt_frame_get_unique_properties().

@bmatherly

This comment has been minimized.

Copy link
Member

bmatherly commented Jan 4, 2019

I think I should add a mlt_frame_get_unique_properties().

I agree. I assume that would return NULL if the properties do not exist.

@ddennedy

This comment has been minimized.

Copy link
Member

ddennedy commented Jan 5, 2019

64-bit Windows build is fixed now if you want to test this.

@bmatherly

This comment has been minimized.

Copy link
Member

bmatherly commented Jan 6, 2019

Confirmed. No more problems.

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