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 776900 - decklinkvideosrc: Do not append a zero timecode if none is found on the source
decklinkvideosrc: Do not append a zero timecode if none is found on the source
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.11.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-01-05 13:57 UTC by Vivia Nikolaidou
Modified: 2017-01-09 16:38 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
0001-decklinkvideosrc-Do-not-append-a-zero-timecode-if-no.patch (5.88 KB, patch)
2017-01-05 13:57 UTC, Vivia Nikolaidou
none Details | Review
0001-decklinkvideosrc-Do-not-append-a-zero-timecode-if-no.patch (7.02 KB, patch)
2017-01-05 17:57 UTC, Vivia Nikolaidou
committed Details | Review

Description Vivia Nikolaidou 2017-01-05 13:57:00 UTC
If the source doesn't give us timecode information, do not append a zero timecode to the frames.
Comment 1 Vivia Nikolaidou 2017-01-05 13:57:52 UTC
Created attachment 342954 [details] [review]
0001-decklinkvideosrc-Do-not-append-a-zero-timecode-if-no.patch
Comment 2 Sebastian Dröge (slomo) 2017-01-05 15:59:25 UTC
Review of attachment 342954 [details] [review]:

::: sys/decklink/gstdecklink.cpp
@@ +853,1 @@
           }

This code here seems to leak the IDeckLinkTimecode. You need to call ::Release() on it, like on the frames/packets (done inside got_video_frame / got_audio_packet at some point).

::: sys/decklink/gstdecklink.h
@@ +211,2 @@
   /* Set by the video source */
+  void (*got_video_frame) (GstElement *videosrc, IDeckLinkVideoInputFrame * frame, GstDecklinkModeEnum mode, GstClockTime capture_time, GstClockTime stream_time, GstClockTime stream_duration, guint hours, guint minutes, guint seconds, guint frames, BMDTimecodeFlags bflags, gboolean no_signal, gboolean have_timecode);

Generally all looks good, but it would seem better to reduce the amount of parameters here a lot by just passing the IDeckLinkTimeCode * here. It being NULL would mean no timecode then and the consumer can make use of that
Comment 3 Vivia Nikolaidou 2017-01-05 17:57:29 UTC
Created attachment 342976 [details] [review]
0001-decklinkvideosrc-Do-not-append-a-zero-timecode-if-no.patch

Thanks for the comments. Attaching the updated version.
Comment 4 Sebastian Dröge (slomo) 2017-01-09 16:38:03 UTC
commit 3cb43f35b837111a376924503a73dfda425731e0
Author: Vivia Nikolaidou <vivia@ahiru.eu>
Date:   Thu Jan 5 15:41:06 2017 +0200

    decklinkvideosrc: Do not append a zero timecode if none is found on the source
    
    If the source doesn't give us timecode information, do not append a zero
    timecode to the frames.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=776900
Comment 5 Sebastian Dröge (slomo) 2017-01-09 16:38:25 UTC
Merging into 1.10 later