Page MenuHome

Made export_subtitles take the Scene start frame into account
Needs RevisionPublic

Authored by bassam kurdali (bassamk) on Jan 9 2016, 10:19 PM.

Details

Summary

exported .srt files would be wrong if scene didn't start at 0 - also could export negative time subtitles

Diff Detail

Event Timeline

bassam kurdali (bassamk) retitled this revision from to Made export_subtitles take the Scene start frame into account.
bassam kurdali (bassamk) updated this object.
bassam kurdali (bassamk) removed rB Blender as the repository for this revision.

I realized during making the bug report that in the corner case where a text strip starts before frame start but ends after it we would probably want a subtitle (starting from time zero), this patch should achieve that.

Bastien Montagne (mont29) requested changes to this revision.Jan 10 2016, 10:03 AM

Yeah, Blender being frame-one-based, we should probably always subtract 1 frame to get correct absolute timecode…

Beside that, making exported timecodes relative to start time of render is indeed useful, but imho it should remain an option - I can think of cases where you want absolute time code while only exporting a subpart of the project...

Also, you should probably check for end renderframe as well, in this case?

Finally, your second patch is made against first one, which makes it not so usable, please always diff against master. ;)

This revision now requires changes to proceed.Jan 10 2016, 10:03 AM

Hi Mont!
I didn't intend to make my second patch this way, might need some help making a proper one; this is the command I used:
 git format-patch HEAD...origin/master
where I had previously git commit ed both those changes locally. How do I tell git make a 'complete' diff?

as for the logic:

frame_start is set to zero if it is negative
frame_end is calculated via subtraction without modification

I then check if frame end is negative: if it is, the subtitle track is before the video even plays and will not show, nothing is outputted. If it is positive, then you get a subtitle from frame_start (previously calculated) and frame end.

It does not make sense to set frame end to zero since then you will just have a subtitle that flashes for one millisecond at the beginning of the video.

btw, I don't mind making it optional; but the default should be that the tracks align. (video/audio/srt)

seq_end should obviously not be set ot 0.0, but to (end_frame - start_frame) (provided seq_end is above that value of course). If you want to offset and clamp by start of rendering range, then you also have to clamp by end of same range.

We can make the option (maybe named 'Offset Timing'?) default ON, yes. And even when OFF, we have to apply -1 frame offset anyway (but without any clamping or whatever), since Blender 'zero time' is on frame 1 (grrrrr).

Regarding git, such dev should be done in local temp branch, to avoid 'polluting' your master one until you are ready to commit. But a command like git diff origin/master.. should still give you complete diff between 'official' master and yours?

ah I see, you want to clamp by the end of the scene frame range! makes sense, I'll do that, makes perfect sense.
what are your current thoughts on making it an option but keeping the default in sync with audio and video output? I actually don't have/see a use case where a user would want an out of sync srt file, and it seems more logical to just set the scene frame start/end frame instead. Perhaps I could ask around on BA? having too many options also clutters interface, especially if they are unneeded.

I'll figure out my git diff business too, I've some friends who are pretty badass with git that will help me figure out this- I'm still really have difficulties with 'what the right thing to do' is with git.

For instance, I already committed (locally) my two changes so far, without branching. It seems like I should be able to branch from that, and have the new branch instead? I'm guessing having done that I could switch back to master and somehow reset those two commits so it tracks origin again.

bassam kurdali (bassamk) edited edge metadata.

ok, now it is clipping the end frame as well as the start frame. I think the patch is now correct (commulative) too.

Patch looks good now, but you still need to control it with an option (be it ON by default).

Bastien Montagne (mont29) requested changes to this revision.Jan 11 2016, 10:26 AM
Bastien Montagne (mont29) edited edge metadata.
This revision now requires changes to proceed.Jan 11 2016, 10:26 AM

I'm happy to do it but first I would like to know why, namely:

why have an option to export subtitles that is out-off-sync by an arbitrary (yes frame 1 is arbitrary) amount from the scene start and end frame (thus becoming out of sync with audio and video output)?
What is the use case for this? who wants it?
'compatibility with a previous behavior' seems tautological - as I think previous behavior was undesirable, and the only reason people didn't notice is that the default 42 ms offset was not noticeable and people either didn't encounter the bug at a higher offset, or didn't report it (yet)

Once again, I'm not opposed to adding the option, but I would like to see a reason for it. I'm open to discussing this on irc and with other stakeholders (editors and coders)

This is more or less same reason as you want it to be offset by start render frame - imagine someone who has a big project, reanders everything, then has to tweak and re-render a small subset of it, so (s)he sets a tiny render range. Then subtiltles timing is adjusted in same range, and exported again, you likely want your whole project's subtitles to be re-exported, no only those inside render range.

Point is: just like you can want to start a project on frame 100, you can want to set render range on different timings than whole project, in which case exporting subtitles would be garbage with new system.

And I already said in previous comment that indeed we had to apply a -1 frame offset to get actual absolute timing (since Blender's zero time is on frame 1) - that is actually the only real bug here. ;)

Regarding how to do this, in fact I would add an 'offset' field to the operator, with an enum to choose between 'Render' range, 'Absolute' timing, and 'Custom' timing (the third would be only one allowing edition of offset numbutton).

And add below a 'Clamp by render range' option - think this has to be decoupled from offset one.

PS: number of options is not really an issue here, currently that op has none!

Ok cool, I was thinking you wanted a cryptic 'keep old behavior' or 'legacy' option which would be confusing. This sounds ok.
Two (really 5) new options:

1- an enum "offset" with the options:
-scene render range (obvious)
-absolute - should this start at zero?
-custom

1.5 - greyed out by default, enabled if custom is selected, two fields for :
-start frame / end frame

2- clip to range a boolean,
defaulted at being on.

  • if scene render range is selected, clips to scene range
  • if absolute is selected, no clipping is performed (as there is no range) so it is greyed out
  • if custom is selected, clips to custom frame range.

Sounds OK. For me main motivation would be to practice adding properties to operators in C ;)

BTW I'm still not convinced with the counter-argument we have for this is that there is no such option for video and audio export - Those both clip and timecode based on the start and end frame range:

  1. when you set frame start and frame end to a temporary custom value, you'd expect to set it back to normal for audio and subtitles which don't have a single frame format.
  2. (and more important) even if we implement the option 'absolute' as the old behavior, if you set a temporary frame range to fix a sequence, that does not mean that you want to export subtitles starting from frame zero. this will not sync with your full project if the full project doesn't start on frame zero - exposing the same problem again. So the current behavior you mention is still wrong if the project doesn't start at frame zero - though the custom option is potentially fine if set to the project bounds.

btw, I hope you don't mind the discussion, I'm desirous of reaching an optimal result, for this there must be a clear design. As mentioned earlier, I don't mind adding options, but I'm still not completely won over to them ;)

Final comment: should we :
add an option to use the scene preview range?

bassam kurdali (bassamk) edited edge metadata.

Hi , this is my first stab at adding properties to the operator as initially discussed. Some comments/questions:

Is it possible to grey out or hide some properties when they don't apply in the file selector? can you point me to an operator that does this?

The reason is:

  • clipping is ignored when absolute is chosen as the range: should be greyed out or hidden.
  • custom start and end is ignored unless custom range is chosen, should be greyed out or hidden otherwise.
  1. Should we add an option to restrict export only to selected text sequences?
  2. I'm still dubious about the options I've added, especially the absolute one, which only gives 'good' results in very special cases, not at all clear.

hrmz the numbering on my comment above is wrong, it reads as 1, 1, 2 should be 1, 2, 3 sorry bout that.

Bastien Montagne (mont29) requested changes to this revision.Jan 18 2016, 12:33 PM
Bastien Montagne (mont29) edited edge metadata.

Regarding UI: wmOperatorType has an ui callback, where typically you just call uiDefAutoButsRNA with a custom callback that is used for each property in the operator: if it returns true, the property is drawn, otherwise it is hidden. See e.g. MESH_OT_sort_elements in editors/mesh/editmesh_tools.c.

Regarding options, I would only use one number (offset) instead of start_frame/end_frame.

Regarding clipping, imho we should always clip to zero (though I found nothing about negative values being forbidden by srt specifications, I doubt it would be correctly handled by any app out there). And option should only control whether we also clip by end render frame (i.e. if we limit length of exported subtitles to current render range).

As for exporting only selected text strips, I’d let that for another patch - let’s first try to get this one ready for master :)

source/blender/editors/space_sequencer/sequencer_edit.c
3842–3844

Never use magic numbers like that, please always use defined values, or even better, anonymous enums:

enum {
    SEQ_SUBTITLE_EXPORT_OFFSET_RENDER    = 1,
    SEQ_SUBTITLE_EXPORT_OFFSET_ABSOLUTE  = 2,
    SEQ_SUBTITLE_EXPORT_OFFSET_CUSTOM    = 3,
};
3927

Again, should be frame 1 (since zero time in Blender is frame one)…

3940

!= 2 eeek! Never use magic numbers like that, please always use defined values (see above).

3941–3942

picky code style point - always a space before and after C operators:

(seq_start > 0.0) ? seq_start : 0.0
3959

tsst… no empty lines of spaces changes in patches please ;)

3989

nice catch.

This revision now requires changes to proceed.Jan 18 2016, 12:33 PM

Here's what I would like to do:

-scene range (as now)

-custom range: show the start and end frame. I don't agree with the offset; I wouldn't know what to do with it myself as a user.

-absolute: export all sequences with no clipping, starting from timestamp zero, regardless of where the first clip lies in the timeline.

remove the clipping boolean option; imo one should always clip in the first two cases, but never in the third, that way we never get negative timestamps guaranteed, and you don't get unpredictable behavior if user has put strips on negative frames but disabled clipping. (what should blender do? it's not obvious, it could clip just the negative strips out, or 'slide' the whole thing introducing an offset)

I'll make the changes for code style (using enum) instead of magic numbers.

Final comment: I still don't see a use case for either of the last options :) It no longer does what the old code did (which I still insist is just a bug) and adds some weird new options instead, to export subtitles that don't sync up with the video audio. I don't see why scene frame start/ frame end are 'good enough' for video and audio export, but not for subtitles.