GNOME Bugzilla – Bug 681678
frei0r: port to 1.0
Last modified: 2012-09-30 13:48:30 UTC
Created attachment 220939 [details] [review] Patch from git diff Here is my github branch : https://github.com/MathieuDuponchelle/gst-plugins-bad/commits/frei0rPluginsPort I attached a patch as well. The mixer compiles fine here, but as I commented out the buffer allocation methods it previsibly segfaults. Having a base mixer would be great, as we could use a default pool.
Thanks, will try to review that in the next days For the mixer, could you also include a patch for that to make it compile and just use gst_buffer_new_allocate() for buffer allocation for now?
Created attachment 220942 [details] [review] Commits squashed, git-format-patch
Cool :) For the mixer, I'll get back to it tomorrow I think.
Looks mostly good, any progress on the mixer ? You can make the caps with ST_VIDEO_CAPS_MAKE ("{ BGRA, RGBA ... "}) gst_frei0r_src_get_caps looks a lot like gst_pad_use_fixed_caps() ?
Thanks for the review, I didn't know you could make caps with a static array. I don't know about gst_pad_use_fixed_caps, I'll have a look when possible. As for the mixer, I tried to only use gst_buffer_new_allocate() but it still segfaulted, the port was not so trivial and it's possible I made a mistake, so I went on to other stuff as this was not critical to pitivi. I'll try to have a look when I find time as well.
Just dropping a note, to say that I have resumed that work. Most of the porting is done now, the src and the filter works. I'm currently working on the caps negotiation in the mixer. It's pretty much like the videomixer, so it won't support renegotiation. At the moment, the initial code causes a deadlock, which is quite normal. Here's the branch for those who would like to have a look. http://cgit.collabora.com/git/user/nicolas/gst-plugins-bad.git/log/?h=frei0r-1.0
Oh, and I use gst_buffer_new_allocate() for now.
Created attachment 225237 [details] [review] Port to 1.0 API This port is complete and working.
Cool, thanks for porting the mixer and fixing your previous comments, I'm sorry I couldn't find time to do it, also the port looks good to me, don't know if my review is acceptable as I initiated the bug report.
Review of attachment 225237 [details] [review]: ::: gst/frei0r/gstfrei0rmixer.c @@ +183,2 @@ + caps = gst_pad_get_current_caps (self->src); + caps = gst_pad_get_pad_template_caps (self->src); Reference leak! ::: gst/frei0r/gstfrei0rsrc.c @@ +76,3 @@ + GST_BUFFER_DURATION (buf) = + gst_util_uint64_scale (self->n_frames, GST_SECOND * self->info.fps_d, + self->info.fps_n) - GST_BUFFER_TIMESTAMP (buf); shouldn't duration just be: gst_util_uint64_scale (GST_SECOND, self->info.fps_d, self->fps_n); ?
> + GST_BUFFER_DURATION (buf) = > + gst_util_uint64_scale (self->n_frames, GST_SECOND * self->info.fps_d, > + self->info.fps_n) - GST_BUFFER_TIMESTAMP (buf); > > shouldn't duration just be: gst_util_uint64_scale (GST_SECOND, > self->info.fps_d, self->fps_n); ? Nope, the existing code is fine, it makes sure you get a continuous perfectly timestamped stream without rounding errors. So it's basically the same as: TIMESTAMP = scale (n_frames, GST_SECOND * fps_d, fps_n) DURATION = scale (n_frames + 1, GST_SECOND * fps_d, fps_n) - TIMESTAMP
Created attachment 225343 [details] [review] Same patch removing the offending caps leak line I simply forgot to remove this line. Sorry.
Olivier has pushed this, thanks for the patches everyone! commit 9a2735a063036fc0146f66b73efb9be231c492b0 Author: Nicolas Dufresne <nicolas.dufresne@collabora.com> Date: Fri Sep 28 11:59:57 2012 -0400 frei0r: Port to 1.0 https://bugzilla.gnome.org/show_bug.cgi?id=681678