GNOME Bugzilla – Bug 796818
deinterlace: Timecode pass-through
Last modified: 2018-07-17 13:30:59 UTC
See commit message
Created attachment 373063 [details] [review] deinterlace: Timecode pass-through for fields=top or bottom When it is trivial to pass-through a timecode, by only removing the "interlaced" flag, do pass-through.
Created attachment 373064 [details] [review] deinterlace: Timecode pass-through for fields=top or bottom When it is trivial to pass-through a timecode, by only removing the "interlaced" flag, do pass-through.
Created attachment 373065 [details] [review] deinterlace: Timecode pass-through When it is trivial to pass-through a timecode, by only removing the "interlaced" flag, do pass-through. Otherwise, double the fps_n and adjust the "frames" field.
Review of attachment 373064 [details] [review]: Patch below contains a more complete solution.
Created attachment 373066 [details] [review] deinterlace: Timecode pass-through When it is trivial to pass-through a timecode, by only removing the "interlaced" flag, do pass-through. Otherwise, double the fps_n and adjust the "frames" field.
Review of attachment 373066 [details] [review]: ::: gst/deinterlace/gstdeinterlace.c @@ +1127,3 @@ + self->field_history[i].tc = self->field_history[i - fields_to_push].tc; + self->field_history[i].has_tc = + self->field_history[i - fields_to_push].has_tc; The old timecodes have to be cleared here @@ +1165,3 @@ GST_DEBUG_OBJECT (self, "Two fields"); self->field_history[1].frame = field1; self->field_history[1].flags = field1_flags; You probably also want to add the timecode here @@ +1171,3 @@ + + if (meta) { + self->field_history[0].tc = meta->tc; And here need to take ownership of the data inside the timecode, or it might go away together with the buffer @@ +1768,3 @@ + &self->field_history[index].tc); + } else { + GstVideoTimeCodeMeta *meta = gst_buffer_get_video_time_code_meta (buf); This part is unneeded if you put the timecode into index 0 and index 1 above where things are put into the field history @@ +1777,3 @@ + if (meta) { + meta->tc.config.fps_n = 2 * meta->tc.config.fps_n; + meta->tc.frames = 2 * meta->tc.frames; These calculations are probably wrong for telecine but I don't know how they should be. They are probably also wrong if fields like ONEFIELD and RFF are in the stream (duplicated timecodes and timecode gaps will happen then). And I believe also without the field=all mode, ONEFIELD/RFF could cause non-contiguous timecodes here
(In reply to Sebastian Dröge (slomo) from comment #6) > Review of attachment 373066 [details] [review] [review]: > > ::: gst/deinterlace/gstdeinterlace.c > @@ +1127,3 @@ > + self->field_history[i].tc = self->field_history[i - fields_to_push].tc; > + self->field_history[i].has_tc = > + self->field_history[i - fields_to_push].has_tc; > > The old timecodes have to be cleared here > > @@ +1171,3 @@ > + > + if (meta) { > + self->field_history[0].tc = meta->tc; > > And here need to take ownership of the data inside the timecode, or it might > go away together with the buffer I actually noticed that this is fine. The buffer is kept alive all the time until you stop using the timecode. Please add a comment but otherwise keep it :) > @@ +1165,3 @@ > GST_DEBUG_OBJECT (self, "Two fields"); > self->field_history[1].frame = field1; > self->field_history[1].flags = field1_flags; > > You probably also want to add the timecode here This is still valid > @@ +1768,3 @@ > + &self->field_history[index].tc); > + } else { > + GstVideoTimeCodeMeta *meta = gst_buffer_get_video_time_code_meta > (buf); > > This part is unneeded if you put the timecode into index 0 and index 1 above > where things are put into the field history And this > @@ +1777,3 @@ > + if (meta) { > + meta->tc.config.fps_n = 2 * meta->tc.config.fps_n; > + meta->tc.frames = 2 * meta->tc.frames; > > These calculations are probably wrong for telecine but I don't know how they > should be. They are probably also wrong if fields like ONEFIELD and RFF are > in the stream (duplicated timecodes and timecode gaps will happen then). > > And I believe also without the field=all mode, ONEFIELD/RFF could cause > non-contiguous timecodes here For these cases, we discussed and couldn't come up with a reasonable way of doing timecodes in those situations. What happens here right now is probably as good as any other alternative. Should probably just get a GST_FIXME just in case... but only once, not per frame ;)
Created attachment 373074 [details] [review] deinterlace: Timecode pass-through When it is trivial to pass-through a timecode, by only removing the "interlaced" flag, do pass-through. Otherwise, double the fps_n and adjust the "frames" field.
Thanks for the comments. Please review again.
Review of attachment 373074 [details] [review]: Looks good except for ::: gst/deinterlace/gstdeinterlace.c @@ +1130,3 @@ self->field_history[i].flags = self->field_history[i - fields_to_push].flags; + self->field_history[i].tc = self->field_history[i - fields_to_push].tc; You're overriding old timecodes here without freeing them
Created attachment 373075 [details] [review] deinterlace: Timecode pass-through When it is trivial to pass-through a timecode, by only removing the "interlaced" flag, do pass-through. Otherwise, double the fps_n and adjust the "frames" field.
Review of attachment 373075 [details] [review]: One more thing, please fix and just push then ::: gst/deinterlace/gstdeinterlace.c @@ +1132,3 @@ + if (self->field_history[i].tc) + gst_video_time_code_free (self->field_history[i].tc); + if (self->field_history[i - fields_to_push].tc) Need to set to NULL in the else case
Created attachment 373076 [details] [review] deinterlace: Timecode pass-through When it is trivial to pass-through a timecode, by only removing the "interlaced" flag, do pass-through. Otherwise, double the fps_n and adjust the "frames" field.
Review of attachment 373076 [details] [review]: Will commit as discussed.
Review of attachment 373076 [details] [review]: commit ceac1b51b18a2316f8b721cdeaab3ae9f03ad5fd (HEAD -> master, dev/master) Author: Vivia Nikolaidou <vivia@ahiru.eu> Date: Mon Jul 16 19:03:39 2018 +0300 deinterlace: Timecode pass-through When it is trivial to pass-through a timecode, by only removing the "interlaced" flag, do pass-through. Otherwise, double the fps_n and adjust the "frames" field. https://bugzilla.gnome.org/show_bug.cgi?id=796818