Continuation of https://developer.blender.org/D2424
Allows users to select a font for their text strips in the video sequence editor.
Details
Details
- Reviewers
Aaron Carlisle (Blendify) Brecht Van Lommel (brecht) Campbell Barton (campbellbarton) - Commits
- rBSd753726ce712: Add font selection to VSE text strips
rBSb3dbe17658fe: Add font selection to VSE text strips
rBd753726ce712: Add font selection to VSE text strips
rBb3dbe17658fe: Add font selection to VSE text strips
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- arcpatch-D3621_3 (branched from master)
- Build Status
Buildable 2681 Build 2681: arc lint + arc unit
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| source/blender/blenkernel/intern/seqeffects.c | ||
|---|---|---|
| 3527 | Add a #define .._NOT_LOAD -2 rather than a magic number. | |
Comment Actions
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? As for thread safety, I would like to be able to use one font with multiple threads. 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 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. |
| 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 | ||
| 307 | 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. |
| 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. | |
| 3456 | If I understand you correctly, 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: 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. 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. | |
| 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 Since both operators are doing basically the same thing I modified unlink OP to use WM_OP_INVOKE_DEFAULT also. |
Comment Actions
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. | |
| 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? Because this patch does not introduce Vfont - it has been there before. 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 | |
Comment Actions
- 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. | |
| release/scripts/startup/bl_ui/space_sequencer.py | ||
|---|---|---|
| 787 | Don't leave commented out code here. | |
| 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? | |
| 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? | |
| 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 | |
| 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? | |
| source/blender/blenkernel/intern/sequencer.c | ||
|---|---|---|
| 321 | First problem: BKE_sequencer_base_clipboard_pointers_free freed datablock without handling usercount | |
| 344 | Second problem: 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. | |
Comment Actions
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. | |
Comment Actions
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? :)
| 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. | |
Comment Actions
- 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?
Comment Actions
I committed the clipboard refactoring separately, with a fix that should make the user counting correct.