Created attachment 233812 [details] [review]
basecamerabinsrc: Add auto-start property to
 basecamerabin

Ugh, patchsets in bugzilla.. Branch here, much easier :):
  http://cgit.collabora.com/git/user/sjoerd/gst-plugins-bad.git/log/?h=uvch264

Requires the patch from #692042

You removed image/jpeg from the caps.

Any reason you use gst_memory_copy() instead of gst_memory_shared() in the mjpeg demuxer ?

I don't think droping segment events is required anymore.

In the src, you replaced gst_pad_fixate_caps(), with gst_caps_fixate().. v4l2src actually has code in there, so it may make more sense to call gst_base_src_fixate(v4l2src, caps);

The getcaps query handling should pass the filter caps around.

*** Bug 678431 has been marked as a duplicate of this bug. ***

Thanks Sjoerd and great job! :)
Couple of comments :
- I noticed a bug in the code, the 'nv12' pad has format=NV21 instead of NV12 (same on 0.10 branch)
- The VF_CAPS also had image/jpeg which I don't think are in GST_VIDEO_FORMATS_ALL (so VID_CAPS also will be missing it)
- In gst_uvc_h264_src_fixate_caps, why did you remove the gst_caps_unref(tcaps) ?
- Same comment as Olivier about the gst_pad_fixate_caps
- I didn't look at the changed in the mjpeg_demux_chain cause it looks scary :)
- The custom_upstream "renegotiate" event handling should be removed and replaced with handling the reconfigure event (or is that automatic?).

- In tests/examples/uvch264/window.glade, you forgot to change (fourcc) into (string) for the caps.
- In http://cgit.collabora.com/git/user/sjoerd/gst-plugins-bad.git/commit/?h=uvch264&id=d9c64a61634070d50314bf61c6018c965ea97f0b you changed the section name but didn't change it in the docs/ file so it gets picked up by gtk-doc.

That's about it from my side, but this is my first 1.0 code that I've looked at so I might not be as qualified.

There is also a couple of bugfixes I've done recently on the 0.10 branch : http://cgit.collabora.com/git/user/kakaroto/gst-plugins-bad.git/log/?h=0.10
You can ignore the PropertyProbe patch (until we have an equivalent for 1.0), but there were a couple of bugs in transform_caps and handling of the caps ref that might need porting to the 1.0 code.

Thanks!

(In reply to comment #3)
> You removed image/jpeg from the caps.
> 
> Any reason you use gst_memory_copy() instead of gst_memory_shared() in the
> mjpeg demuxer ?

gst_memory_shared asserts the GstMemory doens't have GST_MEMORY_FLAG_NO_SHARE,, which v4l2src sets.. So sharing isn't possible in practise here.

In theory sharing if possible would be nice (ideally gst should get some api to do this)

> I don't think droping segment events is required anymore.

I'm not entirely sure about the segment handling, so i left it as is.. Youness might be able to clarify what it's necessary in 0.10
 
> In the src, you replaced gst_pad_fixate_caps(), with gst_caps_fixate()..
> v4l2src actually has code in there, so it may make more sense to call
> gst_base_src_fixate(v4l2src, caps);

Maybe, but that's not public api currently (and i don't think banging on the vmethod by hand is very nice :))

Ideally one might at some point want to merge the v4l specific bits into v4l2src such that the bin doesn't have to bang ioctls on the device behind v4l2src back (and merge the caps negotiation back into v4l2src as well) 

> The getcaps query handling should pass the filter caps around.

fixed in the git branch. Not tested as i don't have the camera with me :)

(In reply to comment #5)
> Thanks Sjoerd and great job! :)
> Couple of comments :
> - I noticed a bug in the code, the 'nv12' pad has format=NV21 instead of NV12
> (same on 0.10 branch)

Fixed

> - The VF_CAPS also had image/jpeg which I don't think are in
> GST_VIDEO_FORMATS_ALL (so VID_CAPS also will be missing it)

fixed

> - In gst_uvc_h264_src_fixate_caps, why did you remove the gst_caps_unref(tcaps)
> ?

gst_caps_normalize takes ownership in 1.0

> - Same comment as Olivier about the gst_pad_fixate_caps

See my reply there :)

> - I didn't look at the changed in the mjpeg_demux_chain cause it looks scary :)

heh, it's fine don't worry.. :p

> - The custom_upstream "renegotiate" event handling should be removed and
> replaced with handling the reconfigure event (or is that automatic?).

Removed that code (nothing send the event atm). Making sure uvch264 renegotiates correctly is something todo, but given v4l2src can't renegotiate properly atm it's a tad moot

> - In tests/examples/uvch264/window.glade, you forgot to change (fourcc) into
> (string) for the caps.
fixed

> - In
> http://cgit.collabora.com/git/user/sjoerd/gst-plugins-bad.git/commit/?h=uvch264&id=d9c64a61634070d50314bf61c6018c965ea97f0b
> you changed the section name but didn't change it in the docs/ file so it gets
> picked up by gtk-doc.
> 
> That's about it from my side, but this is my first 1.0 code that I've looked at
> so I might not be as qualified.

You had some good catches so i guess you are :)

> 
> There is also a couple of bugfixes I've done recently on the 0.10 branch :
> http://cgit.collabora.com/git/user/kakaroto/gst-plugins-bad.git/log/?h=0.10
> You can ignore the PropertyProbe patch (until we have an equivalent for 1.0),
> but there were a couple of bugs in transform_caps and handling of the caps ref
> that might need porting to the 1.0 code.

From just a quick skim it looks like it's only the refcounting bug that isn't pulled in yet (which changed in 1.0, but i should doublecheck if it's correct there).  or did i miss something?

(In reply to comment #7)
> > - In gst_uvc_h264_src_fixate_caps, why did you remove the gst_caps_unref(tcaps)
> > ?
> 
> gst_caps_normalize takes ownership in 1.0
> 
oh ok, a change in behavior. Thanks.

> > - Same comment as Olivier about the gst_pad_fixate_caps
> 
> See my reply there :)
I'm not exactly sure what it does differntly. It's probably just more optimized or something, but it should still work the same in the end. Would need testing.

> 
> > - The custom_upstream "renegotiate" event handling should be removed and
> > replaced with handling the reconfigure event (or is that automatic?).
> 
> Removed that code (nothing send the event atm). Making sure uvch264
> renegotiates correctly is something todo, but given v4l2src can't renegotiate
> properly atm it's a tad moot
Ah, I didn't notice it being removed from the diff, maybe I just missed it.
As for renegotiation, although there is a RESET ioctl for the h264 encoder, it doesn't seem to work for the C920, so either way, renegotiation forces me to put v4l2src to state READY and back to PLAYING, this means that even if v4l2src can't renegotiate at the moment, uvch264_src should still be able to achieve it.

> 
> > 
> > There is also a couple of bugfixes I've done recently on the 0.10 branch :
> > http://cgit.collabora.com/git/user/kakaroto/gst-plugins-bad.git/log/?h=0.10
> > You can ignore the PropertyProbe patch (until we have an equivalent for 1.0),
> > but there were a couple of bugs in transform_caps and handling of the caps ref
> > that might need porting to the 1.0 code.
> 
> From just a quick skim it looks like it's only the refcounting bug that isn't
> pulled in yet (which changed in 1.0, but i should doublecheck if it's correct
> there).  or did i miss something?
Yes, there was a refcounting issue in the case an error occurs, but also, I had to add a fakesink to that transform_caps otherwise basetransform returns the pad templates if it sees there's nothing linked on the other side of the colorspace. I don't know how that works in 1.0 though.


> I'm not entirely sure about the segment handling, so i left it as is.. Youness
> might be able to clarify what it's necessary in 0.10
The reason for segment handling is that when we renegotiate, we drop the EOS that's being sent when we pull v4l2src to state READY, and we put it back to PLAYING, it sends another new-segment which needs to be dropped (otherwise you get accumulation and whatever, and it screws everything up). I believe there's no more accumulation of segments in 1.0, in that case, that code might not be needed anymore, but again without testing we can't know for sure.
I don't have the hardware anymore (sent for a demo to the CES conference), so I can't test this myself now. I might request new cameras so I can keep maintaining the element.

This has landed in master now. Can the bug be closed, or are there outstanding issues? If there are outstanding issues, perhaps new bugs should be filed for them for clarity?

I agree, since this has been ported and merged, I suggest we close the bug. I believe Sjoerd fixed everything we said from the code review, and any issues will be found now only by testing (wish I still had the hardware). Any new issues can have a new bug opened for them.
Thanks Sjoerd for the work!

Cool, thanks everyone!

Comment on attachment 233812 [details] [review]
basecamerabinsrc: Add auto-start property to
 basecamerabin

commit 381fcda68b079b59f6d68b4f47f24e4885d592ba
Author: Youness Alaoui <youness.alaoui@collabora.co.uk>
Date:   Thu May 3 20:36:27 2012 -0400

    basecamerabinsrc: Add auto-start property to basecamerabin