Page MenuHome

Add font selection to VSE text strips
ClosedPublic

Authored by Richard Antalik (ISS) on Aug 17 2018, 10:37 PM.

Diff Detail

Repository
rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
source/blender/blenfont/intern/blf_internal_types.h
167

Maybe reference_count is a more standard name for this.

source/blender/blenkernel/intern/seqeffects.c
3456

It's not clear to me that datablock reference counting in these functions is safe?

I suggest to look for example at all places where seq->scene_camera is accessed and ensure the font reference counting and datablock management is done in all the same places.

It might be ok, but since no other effects strips do it, it's not clear.

3527

Add a #define .._NOT_LOAD -2 rather than a magic number.

This revision now requires changes to proceed.Dec 14 2018, 4:54 PM
Richard Antalik (ISS) marked 10 inline comments as done.

This will now apply only to 2.8 branch

I added a bit more code context. Seems to mess up inline comments. Sorry

source/blender/blenfont/intern/blf.c
249

I would rather keep this consistent. Flag would be more appropriate, but seems not to be needed.

Intention of the code is not absolutely clear - does it mean that we can load one font multiple times, but we have to assign it always new "slot" in font array?
If that is the case, then it is OK.

As for thread safety, I would like to be able to use one font with multiple threads.
Duplication is not the best approach. Even less with limit of 16 fonts.

I can do threaded rendering, so I can make this safe now. But I would rather do it in separate patch as it is slightly out of scope and not so easy to test, as chance of collision is random.

source/blender/blenkernel/intern/seqeffects.c
3456

I think it was OK in 2.7
in 2.8 COW update generated more users.

I have changed the signature of text FX functions however. Something tells me, that it's not quite right way to do but to be fair, I don't really know

3527

Used a more consistent name here

source/blender/windowmanager/intern/wm_event_system.c
1345–1358 ↗(On Diff #11616)

I looked into this a little bit more, it is supposed to change area of operators executed from headers.

OP in question is executed from panel, so I added check for this case, that way it should be more compatible, but even when commented I didn't see any buggy behavior. And I consider this patch quite tested.

source/blender/windowmanager/intern/wm_event_system.c
1520 ↗(On Diff #12926)

I looked into this a little bit more, it is supposed to change area of operators executed from headers.

OP in question is executed from panel, so I added check for this case, that way it should be more compatible, but even when commented I didn't see any buggy behavior. And I consider this patch quite tested.

Brecht Van Lommel (brecht) requested changes to this revision.Dec 16 2018, 3:04 AM
Brecht Van Lommel (brecht) added inline comments.
source/blender/blenfont/intern/blf.c
249

The existence of these unique functions is a poor workaround for fonts not being thread safe.

Ideally there should be a separation between the font itself, and the drawing state. But since there isn't the same font is loaded multiple times with this function, so the render thread and main thread each have their own.

In terms of reference counting, I don't think this function needs special handling, and it is ok as is.

source/blender/blenkernel/intern/seqeffects.c
3411

BLF_load must be run on the main thread, is it guaranteed?

The other thing is that only one thread can be using a font at a time, though I think any potential issues in that regard are not new to this patch.

If that is to be solved, maybe a solution is to put the font ID and a mutex lock in the VFont datablock, and use BLF_load_unique to ensure you get a font purely used by that datablock.

3456

Even in 2.7x there would be cases where this fails. Really it should be carefully ensure that ID handling happens in all the same places as other datablocks used from sequencer strips. Otherwise library linking, datablock deletion and copying, COW, and other things will fail.

Right now it's at least missing in BKE_library_foreach_ID_link, KE_sequence_clipboard_pointers_* and expand_scene.

There are some systems like object modifiers where has some per modifier type callbacks for these things, the sequencer could have a similar system. But if that is added it can be done as a separate refactor, and needs to be hooked up in all the same places.

3460

Remove magic number comment.

source/blender/blenkernel/intern/sequencer.c
5513–5516

How can this work, calling the same function with different number of arguments? Only in c++ could there be arguments with default values.

If the function needs a new argument, it should be added to the functions for all sequencer effect types.

source/blender/makesdna/DNA_sequence_types.h
354

Add SEQ_ prefix.

source/blender/makesrna/intern/rna_sequencer.c
467

This code could move to seqeffects.c and be deduplicated to some extent.

484

Still using magic value here.

source/blender/windowmanager/intern/wm_event_system.c
1520 ↗(On Diff #12926)

This is fixing an issue in the wrong place. It is intentional that operators executed from a button in the UI panels region can run in the main window region, it is not just for headers. Not doing this will likely break operators in other editors.

I'm not sure what this is intended to exactly, but I'm guessing it can be solved by setting layout.operator_context to some value in the UI code.

This revision now requires changes to proceed.Dec 16 2018, 3:04 AM
Richard Antalik (ISS) marked 13 inline comments as done.
Richard Antalik (ISS) added inline comments.
source/blender/blenkernel/intern/seqeffects.c
3411

Right now text fx runs in main thread.

I would have to look closer at that library to see what exactly needs to be locked.
That's why I would rather do it in separate patch.

3456

If I understand you correctly,
There is "library" of operations on top of ID datablocks, which I should use / update on VFont. I should use this library instead of custom functions. BKE_sequencer_text_font_load / BKE_sequencer_text_font_unload.

We can handle VFont refcount in BKE_sequence_clipboard_pointers_* but copy_text_effect seems more appropriate - it copies effect vars only.

I am not very familiar with these datablocks. I would say, that it should be documented or better unified, else it can be pretty good headscratcher :)

For possible fail scenario:
Sequencer handles font usercount in context of text effects. Loading and free is safe in VSE.
Depsgraph did cause problems. If it behaves consistently (after load there is always free) this has been "sanitized".

Is there currently possibility to use font of text effect in a way, that is not safe? If there is, and list of such possibilities do exist, we can "sanitize" that. If such list for some reason cannot exist, we have a problem.
If there is not such possibility, we have no problem. A feature should be responsible for correct resource handling, not resource itself.

I will have to look closely, what BKE_library_foreach_ID_link and expand_scene does, and belongs to this list of problematic functions.

I am thinking, that maybe packing font may cause problems, if there is such option for VFonts.

source/blender/blenkernel/intern/sequencer.c
5513–5516

It works, because sh.copy is pointer to effect specific function.
I wasn't sure if this was legal thing to do.

source/blender/windowmanager/intern/wm_event_system.c
1520 ↗(On Diff #12926)

I tried to do this, but context was the same regardless. But, you pushed me in the right direction.

Found the definition of font open OP, that had context set to WM_OP_INVOKE_DEFAULT
Unlink OP has context set to WM_OP_INVOKE_REGION_WIN.

Since both operators are doing basically the same thing I modified unlink OP to use WM_OP_INVOKE_DEFAULT also.

Compile warnings, struct TextVars; should be declared in this header file.

source/blender/blenkernel/BKE_sequencer.h:308:44: warning: declaration of 'struct TextVars' will not be visible outside of this function [-Wvisibility]
void BKE_sequencer_text_font_unload(struct TextVars *data, const bool do_id_user);
                                           ^
source/blender/blenkernel/BKE_sequencer.h:309:42: warning: declaration of 'struct TextVars' will not be visible outside of this function [-Wvisibility]
void BKE_sequencer_text_font_load(struct TextVars *data, const bool do_id_user);                                         ^
source/blender/blenkernel/intern/seqeffects.c
3411

Ok, sounds good.

3456

A datablock (scene) referencing another datablock (font) requires handling in a few functions. Ideally this would all be localized, but it's not currently.

For now the best solution is to look at similar code and do exactly the same thing. So please do that, for example checking all the places where seq->scene_camera is used.

For example if a font datablock is deleted (maybe from the Python API), all references to it should be set to NULL to avoid invalid pointers. The datablock deletion function uses BKE_library_foreach_ID_link to find all the places that link to that font.

expand_scene is used when appending/linking a scene, to pull in other datablocks used by that scene. In this case the font datablock should come along with the scene.

source/blender/blenkernel/intern/sequencer.c
5513–5516

As far as I can tell it would have given a compile error. Only if the function pointer was casted to something else would it work, but that's rather ugly and somewhat defeats the purpose of having an API that abstracts away the different types.

Brecht Van Lommel (brecht) requested changes to this revision.Dec 16 2018, 10:47 PM
This revision now requires changes to proceed.Dec 16 2018, 10:47 PM
Richard Antalik (ISS) marked an inline comment as done.Dec 17 2018, 2:52 PM
Richard Antalik (ISS) added inline comments.
source/blender/blenkernel/intern/seqeffects.c
3456

OK, but this means, that currently VFont can not be deleted from python and can not be linked from another scene?
Or this can be done indirectly by deleting / linking another different datablock, that uses VFont?

Because this patch does not introduce Vfont - it has been there before.
So I can make patch to fix VFont linking, maybe combine it with mutex for BLF. Then this patch would depend on fixing BLF / VFont.

Also this seq->scene_camera - this member has type of Object * so should I look at that? because seq->scene_camera itself does not exist at lot of places other than BKE_sequence_clipboard_pointers

  • Fix ID reference handling of text datablock to be more standard and correct.
  • Fix wrong usage of blenkernel in blenfont, UI code should not be affected by .blend paths.
source/blender/blenkernel/intern/seqeffects.c
3456

The problem is not that for example deleting won't work, but that it will crash due to pointing to invalid memory.

The problem is not in Vfont, but in the linked from the sequencer strip to `Vfont.

Anyway, I've updated the patch now with the fix for this.

Brecht Van Lommel (brecht) added inline comments.
release/scripts/startup/bl_ui/space_sequencer.py
790

Don't leave commented out code here.

This revision is now accepted and ready to land.Dec 17 2018, 5:37 PM

Fix left out commented line

Richard Antalik (ISS) marked 8 inline comments as done.Dec 17 2018, 11:30 PM
Richard Antalik (ISS) added inline comments.
source/blender/blenkernel/intern/seqeffects.c
3456

Thanks, I think you would have to guide me anyway in this case.

3848

Why did you remove usercount handling?
Now it doesn't work correctly

source/blender/blenkernel/intern/seqeffects.c
3848

The user count is increased by using newlibadr_us instead of newlibadr in readfile.c.

What exactly is not working correctly now?

Richard Antalik (ISS) marked 2 inline comments as done.Dec 18 2018, 1:59 PM
Richard Antalik (ISS) added inline comments.
source/blender/blenkernel/intern/seqeffects.c
3848

rval.load will be pointer to load_text_effect

This is called on .blend load, on strip add, font change, and by sce_copy

add more text strips with font and delete them -> crash

source/blender/blenkernel/intern/seqeffects.c
3848

BKE_sequence_get_blend is called while rendering the strip. Increasing the user count should not happen at that point, it has to happen immediately on file load or when the pointer is changed. Doing it as part of a kind of lazy initialization is not predictable enough.

I couldn't get it to crash here, what are the exact steps?

Richard Antalik (ISS) marked an inline comment as done.Dec 18 2018, 2:39 PM
Richard Antalik (ISS) added inline comments.
source/blender/blenkernel/intern/seqeffects.c
3848

lazy init was requested by campbell, https://developer.blender.org/D2424#inline-27302

Sorry it's the lower one rval.load(seq); @ 3852

I am currently at work, cannot make reproducible case
But I could see, that even when adding strips I got red font field (presumably no users, but referenced error)

source/blender/blenkernel/intern/seqeffects.c
3848

As I understand it Campbell wanted lazy initialization of the BLF font and its associated id. Reference counting of datablocks (confusing also named ID) should not be done lazily, however.

I can't reproduce any red font field with the current code. It's not quite clear to me if you are talking about the latest version of the code or an earlier version?

fix clipboard pointer handling

Richard Antalik (ISS) marked 5 inline comments as done.Dec 18 2018, 11:01 PM
Richard Antalik (ISS) added inline comments.
source/blender/blenkernel/intern/sequencer.c
321

First problem:
On text strip copy, previously stored VFont ID was freed, but usercount wasn't lowered.

BKE_sequencer_base_clipboard_pointers_free freed datablock without handling usercount
BKE_sequencer_text_font_unload would handle usercount, but ID was freed

344

Second problem:
(I think)Duplicated ID was not linked to Main.

If I understand this properly, MEM_dupallocN(ID_PT); would create ID copy, that would be used by sequence TextVars. Usercount in this ID was increased, but ID was not linked to Main.

bmain was NULL so I modified this, to pass actual bmain so we can use it.

I think, that not duplicating ID is safe, because usercount has been increased, and clipboard now owns this datablock.

Brecht Van Lommel (brecht) requested changes to this revision.Dec 19 2018, 1:19 PM

I think the better fix is to ensure the clipboard does not affect the user counts at all.

When copying onto the clipboard, the LIB_ID_CREATE_NO_USER_REFCOUNT flag can be passed. Freeing of the clipboard should also not decrement the user count then.

source/blender/blenkernel/intern/sequencer.c
321

The seqbase_clipboard list is empty at the point BKE_sequencer_base_clipboard_pointers_free is called, so it does nothing now?

344

The clipboard contents can not point to any data in Main, and MEM_dupallocN is needed.

The reason is that the clipboard does not track if any data it points to has been deleted. It may also happen that another .blend file is opened and the current one closed.

This revision now requires changes to proceed.Dec 19 2018, 1:19 PM

Modified clipboard copy operator, so it does not affect ID usercount
ID usercount is increased in seqclipboard_ptr_restore

Tested with sound strip

  • behaves as unpatched 2.8
  • usercount increased on paste
  • multiple instances however can exist with usercount of 1

scene strip:

  • on scene strip add usercount is not increased (same as unpatched)
  • usercount increased on paste (unpatched does not increase usercount)
  • scene with usercount of 1 on unlink will refer to another scene
  • scene with usercount > 1 on unlink crashes

Did we open can of worms? :)

Campbell Barton (campbellbarton) requested changes to this revision.Dec 19 2018, 10:48 PM
Campbell Barton (campbellbarton) added inline comments.
source/blender/blenfont/intern/blf.c
346–369

Add BLI_assert(font->reference_count != 0); before decrement since reference counting bugs can easily slip by unnoticed.

Also, we normally do font->reference_count--; unless there is a reason to do the reverse.

source/blender/blenkernel/intern/seqeffects.c
3411

Issues like this are a hassle to track down, so assert it's true.

BLI_assert(BLI_thread_is_main());
source/blender/blenkernel/intern/sequencer.c
368

case should be indented.

397

Style (space between struct and *)

5727

This change seems unrelated to fonts, it should be a separate patch.

This revision now requires changes to proceed.Dec 19 2018, 10:48 PM
Richard Antalik (ISS) updated this revision to Diff 13011.EditedDec 21 2018, 1:07 AM
Richard Antalik (ISS) marked 7 inline comments as done.
  • Fix style
  • add assert check, did refcnt > 0 rather then refcnt != 0
  • fix dirty worktree (oops)
  • TODO fix referencing of other ID's in this patch?

I committed the clipboard refactoring separately, with a fix that should make the user counting correct.

Rebase on master, remove some unnecessary #includes.

This is ready to commit now as far as I'm concerned.

This revision was not accepted when it landed; it landed in state Needs Review.Jan 14 2019, 7:01 AM
This revision was automatically updated to reflect the committed changes.