GNOME Bugzilla – Bug 652951
vp8enc: Set correct timestamp/duration for altref/invisible frames
Last modified: 2012-01-06 06:51:29 UTC
If using vp8enc with multipass and auto-alt-ref-frames=1 codec will produce altref frames marked as invisible. But there are fallowing problems in the code: in gstvp8enc.c, function gst_vp8_enc_shape_output: if (l == hook->invisible && frame->is_sync_point) { --> this part will be never called if we get invisible frame..... GST_BUFFER_FLAG_UNSET (buf, GST_BUFFER_FLAG_DELTA_UNIT); encoder->keyframe_distance = 0; } else { GST_BUFFER_FLAG_SET (buf, GST_BUFFER_FLAG_DELTA_UNIT); encoder->keyframe_distance++; } so it looks like it invisible frame shouldn't have DELTA_UNIT flag in matroskamux code: if (collect_pad->track->type == GST_MATROSKA_TRACK_TYPE_VIDEO && !GST_BUFFER_FLAG_IS_SET (buf, GST_BUFFER_FLAG_DELTA_UNIT)) { GST_LOG_OBJECT (mux, "have video keyframe, ts=%" GST_TIME_FORMAT, GST_TIME_ARGS (GST_BUFFER_TIMESTAMP (buf))); is_video_keyframe = TRUE; } looks like frame with no DELTA flag will be interpreted as keyframe. And later, for each keyframe 0x80 flag will be set: int flags = GST_BUFFER_FLAG_IS_SET (buf, GST_BUFFER_FLAG_DELTA_UNIT) ? 0 : 0x80; hdr = gst_matroska_mux_create_buffer_header (collect_pad->track, relative_timestamp, flags); in libvpx/vpxenc.c code, keyframe=0x80 and invisible frame=0x08: flags = 0; if(is_keyframe) flags |= 0x80; if(pkt->data.frame.flags & VPX_FRAME_IS_INVISIBLE) flags |= 0x08; Ebml_Write(glob, &flags, 1); Ebml_Write(glob, pkt->data.frame.buf, pkt->data.frame.sz); ................... So it looks like there is no gst_buffer flag for invisible frame. DELTA is just frame and no_DELTA is a key frame. Probably GAP flag will be good for invisible frame. And it should be fixed at same time in matrosk/webmmux and vp8enc.
steps to reproduce. auto-alt-ref-frames is important to reproduce it: make first pass: gst-launch filesrc location=some_file.ogg ! decodebin2 ! vp8enc multipass-cache-file=multipass.cache multipass-mode=1 mode=vbr bitrate=2000000 auto-alt-ref-frames=1 ! fakesink make second pass: gst-launch filesrc location=some_file.ogg ! decodebin2 ! vp8enc multipass-cache-file=multipass.cache multipass-mode=2 mode=vbr bitrate=2000000 auto-alt-ref-frames=1 ! webmmux ! filesink location=test.webm play the file. you will get lots of artifacts in the video. The reason is, altref/invisible frames are played.
Created attachment 190376 [details] [review] patch - add timestamp to invisible frame this patch add timestamp on invisible frame, so this make matroskamux happy and this will not drop invis frames.
Review of attachment 190376 [details] [review]: ::: ext/vp8/gstvp8enc.c @@ +993,3 @@ + encoder->last_invisible_ts = encoder->last_timestamp + 1; + GST_BUFFER_TIMESTAMP (buf) = encoder->last_timestamp + 1; + GST_BUFFER_DURATION (buf) = encoder->last_duration; The duration of invisible frames should be 1 then. Please also note that the timestamps and durations here are in nanoseconds and in matroskamux they are in microseconds or whatever, depending on the timescale property. Does it also work if you give the same timestamps as the previous visible frame and a duration of 0? @@ +1018,3 @@ + + if (encoder->last_invisible_ts == encoder->last_timestamp) + GST_WARNING_OBJECT (encoder, "oops, last_invisible_ts == currnet_timestamp"); Make this a g_warning
Created attachment 190695 [details] [review] patch v2
Review of attachment 190695 [details] [review]: ::: ext/vp8/gstvp8enc.c @@ +991,3 @@ } + encoder->last_invisible_ts = encoder->last_timestamp + GST_MSECOND; Same problem as before, depending on the timecode scale you could get the same timestamp as for the previous visible frame @@ +1018,3 @@ + + if (encoder->last_invisible_ts == encoder->last_timestamp) + g_warning ("oops, last_invisible_ts == currnet_timestamp"); Typo here and please use something more descriptive, including the element name and something that could be understood by the user
Created attachment 190698 [details] [review] patch v3 according to webm documentation, alltref frame may share same timestamp as fallowing D-frame. Just to make thinks easy, altref will do have same timestamp as D-freme
commit d1fe501eb735d866a6f1899154b82d5f09838bf3 Author: Alexey Fisher <bug-track@fisher-privat.net> Date: Sun Jun 26 15:15:54 2011 +0200 vp8enc: generate a timestamp for alt-ref frames. It will fix handling of altref/invisible frames since matroska-mux drop any fram with no timestamp. see also: http://www.webmproject.org/code/specs/container/ The encoder will currently set the AR's timestamp as close as possible to the previous frame while attempting to provide a timestamp that is strictly increasing. In cases where the time base given to the encoder at configure time is not granular enough to allow for this the AR will share the same timestamp as D, but should be treated as having no duration. Fixes bug #652951 Signed-off-by: Alexey Fisher <bug-track@fisher-privat.net>
Created attachment 191680 [details] [review] correct timestamp
Review of attachment 191680 [details] [review]: ::: ext/vp8/gstvp8enc.c @@ +993,3 @@ GST_BUFFER_FLAG_SET (buf, GST_BUFFER_FLAG_INVISIBLE); + GST_BUFFER_TIMESTAMP (buf) = encoder->last_ts + + (GST_BUFFER_TIMESTAMP (frame->src_buffer) - encoder->last_ts) / 2; You need to initialize last_ts to something sane (e.g. 0 or segment start) in ::reset for the case when the first frame is an altref frame. Also this means that multiple altref frames after another have all the same timestamp
Created attachment 191685 [details] [review] correct timestamp v2