After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 796876 - deinterlace: Closed caption pass-through
deinterlace: Closed caption pass-through
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-good
unspecified
Other All
: Normal normal
: 1.15.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2018-07-25 21:06 UTC by Vivia Nikolaidou
Modified: 2018-07-27 11:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
deinterlace: Closed caption pass-through (14.58 KB, patch)
2018-07-25 21:07 UTC, Vivia Nikolaidou
none Details | Review
deinterlace: Closed caption pass-through (14.67 KB, patch)
2018-07-27 08:34 UTC, Vivia Nikolaidou
committed Details | Review

Description Vivia Nikolaidou 2018-07-25 21:06:56 UTC
See commit message
Comment 1 Vivia Nikolaidou 2018-07-25 21:07:01 UTC
Created attachment 373167 [details] [review]
deinterlace: Closed caption pass-through

Pass through closed caption data when deinterlacing. When two
deinterlaced frames are created for the same interlaced frame (e.g.
fields=all), the second of the two frames will have no closed caption
data.

Also fixed memory leaks related to timecode meta pass-through.
Comment 2 Sebastian Dröge (slomo) 2018-07-26 11:15:39 UTC
Review of attachment 373167 [details] [review]:

::: gst/deinterlace/gstdeinterlace.c
@@ +810,3 @@
+      if (self->field_history[idx].caption->data) {
+        /* If it was passed to a frame, this is set to NULL */
+        g_free (self->field_history[idx].caption->data);

g_free() is NULL-safe

@@ +829,3 @@
+  gst_deinterlace_delete_meta_at (self, idx);
+
+  gst_video_frame_unmap_and_free (gst_deinterlace_pop_history (self));

For a later time (add a FIXME comment!) it would be good if pop_history() would fill an out-parameter with the frame and everything else. Currently there's pop_history() that works with the indizes and then you do it again manually a few lines above here.

Also generally the history handling should be refactored a bit to have meaningful functions instead of code arbitrarily adjusting the history in similar ways in multiple places.
Comment 3 Vivia Nikolaidou 2018-07-27 08:34:24 UTC
Created attachment 373177 [details] [review]
deinterlace: Closed caption pass-through

Pass through closed caption data when deinterlacing. When two
deinterlaced frames are created for the same interlaced frame (e.g.
fields=all), the second of the two frames will have no closed caption
data.

Also fixed memory leaks related to timecode meta pass-through.
Comment 4 Vivia Nikolaidou 2018-07-27 11:37:35 UTC
Review of attachment 373177 [details] [review]:

commit f2c2560db27b97ae9fa00e3a06a485b8e1cd79aa (HEAD -> master, origin/master, origin/HEAD)
Author: Vivia Nikolaidou <vivia@ahiru.eu>
Date:   Wed Jul 25 17:15:53 2018 +0300

    deinterlace: Closed caption pass-through

    Pass through closed caption data when deinterlacing. When two
    deinterlaced frames are created for the same interlaced frame (e.g.
    fields=all), the second of the two frames will have no closed caption
    data.

    Also fixed memory leaks related to timecode meta pass-through.

    https://bugzilla.gnome.org/show_bug.cgi?id=796876