GNOME Bugzilla – Bug 644150
[basevideoencoder] Store the GstForceKeyUnit event for forwarding it later
Last modified: 2011-06-29 09:50:18 UTC
Created attachment 182757 [details] [review] Store the event to forward it later The GstForceKeyUnit event is swallowed by the encoder and should be forwarded before pushing the forced keyframe.
Created attachment 182758 [details] [review] Forward the event before pushing the forced keyframe
quick comments: s/g_print/GST_DEBUG/ Unreffing the event should be done in dispose, not finalize (it could have a ref to another GObject) Set "base_video_encoder->force_key_unit_event" to NULL after sending it (just to be safe. You probably want to clear it on flushes and the like. In the src_event function, you keep a pointer to the event, but not a ref... Also, when sending it out, you may want to set the timestamp, running-time, stream-time (if you have them),
(In reply to comment #2) > quick comments: > > s/g_print/GST_DEBUG/ My mistake, this shouldn't be there. > > Unreffing the event should be done in dispose, not finalize (it could have a > ref to another GObject) Right > > Set "base_video_encoder->force_key_unit_event" to NULL after sending it (just > to be safe. You probably want to clear it on flushes and the like. ok > > In the src_event function, you keep a pointer to the event, but not a ref... The ref is kept by not unrefing, as it was done before. > > Also, when sending it out, you may want to set the timestamp, running-time, > stream-time (if you have them), As I can see in the draft, this should be defined in the event's structure right?
(In reply to comment #3) > (In reply to comment #2) > > Also, when sending it out, you may want to set the timestamp, running-time, > > stream-time (if you have them), > As I can see in the draft, this should be defined in the event's structure > right? Not in upstream events, but when you reverse it, you can add that info.. Also, you want to make sure that the direction of the new event is right ?
Created attachment 182764 [details] [review] Store the event to forward it later
Created attachment 182765 [details] [review] Forward the event before pushing the forced keyframe
(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > Also, when sending it out, you may want to set the timestamp, running-time, > > > stream-time (if you have them), > > As I can see in the draft, this should be defined in the event's structure > > right? > > Not in upstream events, but when you reverse it, you can add that info.. Also, > you want to make sure that the direction of the new event is right ? Right, I should check the direction too.
in dispose, set the event to NULL (dispose can be called more than once). You also want to lock what you're doing in dispose and chain up to the parent's dispose function
In GstBaseVideoEncoder, should I forward the event as it is if it's an upstream event?
Upstream events should not be forwarded further upstream. An upstream event should be recreated as a downstream event and attached to a frame the same way a downstream event would be.
(In reply to comment #10) > Upstream events should not be forwarded further upstream. > > An upstream event should be recreated as a downstream event and attached to a > frame the same way a downstream event would be. Now everything makes sense. I was a little bit confused with this upstream event, but of course it needs to be recreated and sent back downstream to be reused by muxers and sinks.
Created attachment 182861 [details] [review] Store the event to forward it later In dispose, take the lock of the object and set the event to NULL. For the upstream event, change the direction of the event.
Created attachment 182862 [details] [review] Forward the event before pushing the forced keyframe With the object's lock, get a pointer to the event and set base_video_encoder->force_key_unit_event=NULL when we have a forced keyframe. Update the timestamp field of the structure before pushing the event.
If you modify the event, you should copy it first with gst_event_copy().. I think the rules for writability vs refcount are the same as for buffers.
Created attachment 182881 [details] [review] Store the event to forward it later Get a copy of the event as we might need to modify it.
Created attachment 182883 [details] [review] Forward the event only before the IDR frame in the x264 encoder Same thing for the x264 encoder. The event should be forwarded right before the IDR frame.
In the x264, the GstForceKeyUnit event forces a X264_TYPE_I frame, but shouldn't it force a X264_TYPE_IDR frame?
++ on the basevideoencoder/vp8 patches There is also bug #630292 about using the new API for that in x264, might be good to merge both. Aren't I frames also IDR frames in x264 ? Otherwise ++ on the x264 patch too.
(In reply to comment #18) > ++ on the basevideoencoder/vp8 patches > > There is also bug #630292 about using the new API for that in x264, might be > good to merge both. Aren't I frames also IDR frames in x264 ? Otherwise ++ on > the x264 patch too. An I frame can reference other I frames and and IDR frame is a special type of I frame that is independently decodable because it doesn't reference older I frames.
Does x264 do that ? Please check with x264 devs (to be honest, I don't know). I also noticed that the patch on bug #630292 should only be used if periodic intra refresh is used (not with regular IDR frames).
(In reply to comment #19) > (In reply to comment #18) > > ++ on the basevideoencoder/vp8 patches > > > > There is also bug #630292 about using the new API for that in x264, might be > > good to merge both. Aren't I frames also IDR frames in x264 ? Otherwise ++ on > > the x264 patch too. > An I frame can reference other I frames and and IDR frame is a special type of > I frame that is independently decodable because it doesn't reference older I > frames. Maybe I didn't expressed my self very well... what I meant is that a predicted frame doesn't always use the last I frame but it can use any of the reference frame stored. IDR frames ensures that the next predicted frames will not use any reference before this IDR frame.
Sorry, you're right.. The patch on bug #630292 should force an IDR frame correctly.
Is there something left to be done or is someone working on getting this merged into 0.10.34?
I have some GstForceKeyUnit stuff in a bunch of pending patches for base video encoder (and decoder).
Two of the patchs are obsolete with: https://bugzilla.gnome.org/show_bug.cgi?id=652961
Created attachment 190234 [details] [review] Use the new API for GstForceKeyUnit events Patch using the new API proposal in #607742
Created attachment 190235 [details] [review] Store the event to forward it later Store the downstream event to use it later using the new API
Created attachment 190320 [details] [review] Store the event to forward it later Fix previous patch after using it in a real pipeline
This bug is now obsolete after this commit: http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/commit/?id=16c6a49bd44e2389162ac3d67d7fd24f6f43df43 I'll open a new bug for the use of the new API