Page MenuHome

Fix T65656 - Crash on hard-cut audio strip
Needs ReviewPublic

Authored by Richard Antalik (ISS) on Jun 10 2019, 4:25 AM.

Details

Summary

Broken by bbaa1bffe9db.

@Sergey Sharybin (sergey) Not sure If I can have 2 handles initialized (probably not) or even if this is design violation.

That hard-cut code seems quite hacky, so I could do this without sound handle. But I can imagine situation, where I would need to use it from sequencer

Diff Detail

Event Timeline

Harbormaster completed remote builds in B3857: Diff 15862.
Sergey Sharybin (sergey) requested changes to this revision.Jun 10 2019, 10:39 AM

Audio handles do not exist on original sequences.

In the areas where sequencer really needs to access those handles it is allowed to load them using BKE_sound_load_audio() on the original sequence, and free them soon after that using BKE_sound_free_audio(). This is how add audio strip works for example.

Would imagine something similar to this will work here as well.

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

This will always evaluate to truth, since the playback_handle does not exist on original sequences.

This revision now requires changes to proceed.Jun 10 2019, 10:39 AM
source/blender/blenkernel/intern/sequencer.c
1050

This is strange, provided, that crash happens at line 1053 in this file how did this ever worked?
Also I remember using playback_handle in 2.79 to get audio length, without need to initialize the handle.

Sound and playback handles are treated as evaluated data, similar to modifier stack, i.e. This means they are only valid when accessed from evaluated speaker, sound, scene and sequencer strip.

Operators work on an original data, and tag it for update after they are done with modifications.

This allows to have same scene and objects being evaluated into different states in different contexts (i.e. viewport vs. final render) at the same time.

Blender 2.79 allowed you access to playback_handle from anywhere is because Blender was always reading sound files. But what is most annoying it was always registering sound in the scene. The latter we can not do on load because this step is only to be performed during scene evaluation.

Ideal solution would be to add BKE_sound_get_info() which will give information about sound you gave to it. The tricky part about this is that you'd need to wrap result of AUD_getInfo() into struct which can be safely used in BKE headers.

Second to ideal solution is to temporarily load sound from operations which needs access information using functions i've mentioned earlier.
This allows to follow clear original/evaluated state separation, and also allows to easily find all usages and replace with BKE_sound_get_info() later on.

Third to ideal solution is to keep handles in the original sound. This minimizes amount of changes in the operators, but introduces following problems:

  • Opens files even if they are not needed (speaker, sound, etc) is not on visible view layer. Doesn't sound too bad, for until you start to deal with assets stored on a shared network storage.
  • Makes ownership way less clear and confusing.

I don't think there so many places where access to playback handle is needed, so would really prefer to have ownership clear, and pick up one for the first two ways to move forward.

Thanks, for explanation.

If I understand you correctly: bSound type should own copy AUD_SoundInfo returned by AUD_getInfo(), but packed to custom struct.

Now should BKE_sound_get_info() return AUD_SoundInfo directly, or it should be custom type? For VSE it probably doesn't matter, but not sure about other uses.
I am not sure about who will issue this data? From what you said, it will be either operator, or depsgraph. My best bet would be sound_verify_evaluated_id()?

Now should BKE_sound_get_info() return AUD_SoundInfo directly, or it should be custom type?

That's the tricky part. Blender might be compiled with WITH_AUDASPACE=OFF, meaning AUD_SoundInfo will never be available. And having a type in a header which depends on compiler flags is quite fragile. Additionally, having custom type allows to close audio handles inside of BKE_sound_get_info().
So to me it seems having custom type is easier and safer.

I am not sure about who will issue this data? From what you said, it will be either operator, or depsgraph. My best bet would be sound_verify_evaluated_id()?

BKE_sound_get_info () will be used by operators, such as "Add Sound Strip", "Cut" and other which needs sound info. Since operators acts on an original data sound_verify_evaluated_id() is not needed in BKE_sound_get_info().

This is untested update.

I am storing pointer to SoundInfo in DNA, so, BKE_sound_get_info in not truly compatible with AUD_getInfo. Not sure if this is problem or not, but SoundInfo may change it's size in future. I can wrap it even more though so it could be compatible...

Also I am not sure if I should copy SoundInfo in BKE_sound_copy_data.

And I forgot to set sound->info to null in readfile.c.

This approach wouldn't work because audio handles are never opened for sound from bmain, they are only opened for evaluated sounds which are coming via depsgraph and copy-on-write.

I am about to submit a patchset which implements BKE_sound_get_info() which can be used on both original and copy-on-written Sounds.