For this to work there either needs to be something like videorate inside theoraenc, that creates "duplicated frames" if there's a frame missing and makes sure that there's a constant framerate. Or videorate needs to mark duplicated frames somehow by some buffer flag.

Arguably GST_BUFFER_FLAG_GAP could be used for this in video:
> the buffer has been created to fill a gap in the stream and contains media neutral data (elements
> can switch to optimized code path that ignores the buffer content).

It's created to fill a gap in the stream and contains something like neutral data (for audio this is silence, for video this could be something like the previous frame). And elements, like theoraenc, can do optimized stuff for this.

Any oppinions?

commit ac5976960448a65e516934a0d538ea2abcc321ab
Author: Sebastian Dröge <sebastian.droege@collabora.co.uk>
Date:   Fri Aug 20 12:03:44 2010 +0200

    videorate: Mark duplicated frames with the GAP flag
    
    We currently don't use the GAP flag for video and the docs say
    that this is for buffers, that have been created to fill a gap
    and contains neutral data. For video this is the previous frame.
    
    This information can be used by encoders to encode the duplicated
    frames more efficiently. See bug #627459.



Now this only needs to be used in theoraenc. Unfortunately the TH_ENCCTL_SET_DUP_COUNT API is absolutely suboptimal for our use case here because we know the number of duplicates only after the next non-duplicate frame is received. And always waiting for that will increase the latency a lot. That's something that should be fixed in libtheora

See https://trac.xiph.org/ticket/1730 for the libtheora bug

I am not conviced the GAP flag is the solution here. The GAP flag is defined so that e.g. a filter can pass the incomming buffer unmodified along. In audio e.g. a silent buffer would remain silent. In video this would be a fully transparent frame.

I see the point of having a duplicated frame and wanting to take benefit out of that. Could encoders keep a ref on the last frame and apply the duplicate optimisation if the get the same buffer again. That should work as videorate uses subbuffers for duplicated frames.

I was wondering if it's possible to do same for vp8.
There is VPX_FRAME_IS_DROPPABLE : vpx_encoder.h . So make be just write 0byte frame market as DROPPABLE. And correct en/decoder to handle 0byte DROPPABLE frames.

droppable frames are a bit different. they're encoded frames, that could be dropped from the stream without having any effect on future frames (i.e. droppable frames are not referenced by future frames). This can be used for example if your network connection becomes slow for a short time.

Theora has something like this too but we don't have any way yet to flag such frames for the payloaders, muxers, etc.

(In reply to comment #4)
> I am not conviced the GAP flag is the solution here. The GAP flag is defined so
> that e.g. a filter can pass the incomming buffer unmodified along. In audio
> e.g. a silent buffer would remain silent. In video this would be a fully
> transparent frame.

Silence is, what is produced by a soundcard if there's a gap. The previous frame is what is shown by video sinks if there's a gap. A transparent frame on it's own has no real meaning (what should be shown? It's transparent after all and as such has no information). Also transparency only makes sense for formats with alpha channel...

> I see the point of having a duplicated frame and wanting to take benefit out of
> that. Could encoders keep a ref on the last frame and apply the duplicate
> optimisation if the get the same buffer again. That should work as videorate
> uses subbuffers for duplicated frames.

That would work for most situations, yes. But I can think of many uncommon situations where this would cause major problems (if you want details ask). We really need something in the metadata to flag such buffers.

I agree that a gap flag on a video frame should indicate a duplicate frame.

One problem with using this in theoraenc is that libtheora requires that you give it the number of duplicates along with the original frame.  Thus, to use this feature, one would have to implement a one-frame queue inside theoraenc, increasing the latency of the encoder.

On the other hand, checking the duration of incoming buffers and implementing videorate functionality in theoraenc, can be done with libtheora as-is, with no increase in latency.

I'd prefer to wait until it's fixed in libtheora to request a new encoded frame from the encoder that is a dup of the previous frame, see the bug in the Xiph trac. Monty and gmaxwell in #theora said that this should be easy to implement, now that the encoder keeps a copy of the previous, unencoded frame anyway, and this would be easy enough for someone who touches the theora codebase the first time.

Adding complicated functionality in theoraenc to queue frames or do other things when this can easily be fixed on the other side feels wrong

Any changes so far?

I've started looking at this on the libtheora side.

Created attachment 187412 [details] [review]
mark count of dupps before current frame.

This patch is remix of Vincents patch and result of discussion with gmaxwell on #theora. gmaxwell suggested to count dup frames before non dup frame and tell libtheora to produce dups of this frames:

Time goes like this:    [123456789ABCDEF]
Webcam produces frames: [*  *  **   * * ]
Give these dupe counts: [0  3  30   3 2 ]

There is no need to patch libtheora.

This patch is wrong for two reasons IMHO
1) You add latency for no good reason, you need to wait until the next non-dup frame to output the encoded dup frames
2) You create dup frames from the next non-dup frame, not the previous non-dup frame. TH_ENCCTL_SET_DUP_COUNT will output N dup frames of the next encoded frame but the GAP flag on buffers says that the current frame (the one with the flag) is a duplicate of the previous frame.

I'm not agree:
1) the latency is only for dup frames, non dups are send without latency.
2) libtheora handle dups by setting 0 byte frame to ogg.
3) the real frame is never lost, so there is no difference if we mark empty frames as dup of frame 1 or 2.
4) we need to make dups only to make ogg happy. it should make no difference for rtp.

If we send real dup we wost processor time. If we send marked dups, we make useless calls (probably not a real problem). If stream produce really many dups, i looks bad so or so.

But no body will see the difference on latency of one frame.

(In reply to comment #14)
> I'm not agree:
> 1) the latency is only for dup frames, non dups are send without latency.

Exactly, so you add 3 frames latency in your example above in a few places

> 3) the real frame is never lost, so there is no difference if we mark empty
> frames as dup of frame 1 or 2.

It makes a difference, e.g. assume frame A has 3 dups, frame B has no dup and then comes frame C. What you want is:
A AAA B C
What you get is:
A B BBB C

> 4) we need to make dups only to make ogg happy. it should make no difference
> for rtp.

Well, yes, for RTP I assume the payloader should drop the dup frames, similar to what the speex payloader does.

> If we send real dup we wost processor time. If we send marked dups, we make
> useless calls (probably not a real problem). If stream produce really many
> dups, i looks bad so or so.

No need to argue with me about the need of something like this, I'm just saying that your implementation and what can be done with libtheora currently is suboptimal.

Time goes like this:    [123456789ABCDEF]
Webcam produces frames: [*  *  **   * * ]
Give these dupe counts: [0  3  30   3 2 ]
Patch will do:          [14447778CCCCEE ]
Best will be:           [11144478888CCEE]

So or so, the real frame is always on proper place. We create non existing frames from previous or current frame. So or so it can be not 100% sync with audio.

Also i think this patch is good because it cost less and can work with current libtheora.

IMHO, The latency is imaginer, because ogg do not use timestamps for video stream. We need only correct count of frames per second.

Other two cents:
- Frames miss the start of action. So it is only guess if action started right direct after 1 frame or direct before 2.

- stream compatibility. For computer forensics it will be a challenge. The stream created with Theora 1.1 and say Theora~1.5 (because the correct fix will be to change theora API) has different time recreation. Programs currently using old api even with new theora will use old api. So in my job (computer forensic ) i'll haw real problem to find what api was used to create current stream. And in what what the time recreation is wrong. So i think there is nothing to say against this argument.

Action:    [...*::::----]
Frames:    [*     *   * ]
Theora 1.1:[FF00000F000F]
Theora~1.5:[F00000F000F0]

(In reply to comment #16)
>
> So or so, the real frame is always on proper place. We create non existing
> frames from previous or current frame. So or so it can be not 100% sync with
> audio.

Well, you're creating a time machine here ;) Which is the main reason why it feels wrong to me

(In reply to comment #17)
> IMHO, The latency is imaginer, because ogg do not use timestamps for video
> stream. We need only correct count of frames per second.

For creating files the one-frame latency is fine. Sure. But you also add this latency to live video streams for example and, depending on the container format, you'll also could violate bitrate constraints because you send the duplicated frames (0 byte theora packets but N bytes container packets) in bursts with the next non-dup frame.

(In reply to comment #18)
> Other two cents:
> - Frames miss the start of action. So it is only guess if action started right
> direct after 1 frame or direct before 2.

With your "time machine" it will actually be a bit harder... because from looking at the video you don't know if you're looking at a "backwards-replicated" dup frame or the real action.

> - stream compatibility. For computer forensics it will be a challenge. The
> stream created with Theora 1.1 and say Theora~1.5 (because the correct fix will
> be to change theora API) has different time recreation. Programs currently
> using old api even with new theora will use old api. So in my job (computer
> forensic ) i'll haw real problem to find what api was used to create current
> stream. And in what what the time recreation is wrong. So i think there is
> nothing to say against this argument.

Nothing to add to this comment, there's not only the "time machine" issue but in forensic you'll also additionally have to consider the encoder version to make accurate conclusions.

Ok, i almost give up :)

> (In reply to comment #19)
> > IMHO, The latency is imaginer, because ogg do not use timestamps for video
> > stream. We need only correct count of frames per second.
> 
> For creating files the one-frame latency is fine. Sure. But you also add this
> latency to live video streams for example and, depending on the container
> format, you'll also could violate bitrate constraints because you send the
> duplicated frames (0 byte theora packets but N bytes container packets) in
> bursts with the next non-dup frame.

In this case even changing API in theora will get same problem, it will only exclude time machine problem....  (after some thinking) theoraenc create always container packets, and if dup detection is working (and it seems to work), it create also empthy container packets. So if it is a problem, then it already exist.

I just tested it with mkv, it seems works fine.

About time machine problem. between two existing frames we have some count of missing frames. What happens in the time between this frames, no body know. At least there is no evidence.
How to fill this empty space is up to codec. Fll it with current frame or with next frame, it is just assumption about something what we do not know. But we need to tell the codec, how many frames are missing.

(In reply to comment #20)
> Ok, i almost give up :)
> 
> > (In reply to comment #19)
> > > IMHO, The latency is imaginer, because ogg do not use timestamps for video
> > > stream. We need only correct count of frames per second.
> > 
> > For creating files the one-frame latency is fine. Sure. But you also add this
> > latency to live video streams for example and, depending on the container
> > format, you'll also could violate bitrate constraints because you send the
> > duplicated frames (0 byte theora packets but N bytes container packets) in
> > bursts with the next non-dup frame.
> 
> In this case even changing API in theora will get same problem, it will only
> exclude time machine problem....  (after some thinking) theoraenc create always
> container packets, and if dup detection is working (and it seems to work), it
> create also empthy container packets. So if it is a problem, then it already
> exist.
> 
> I just tested it with mkv, it seems works fine.

But in that case you can send the empty packets between the previous and next frame instead of sending them after encoding the next frame and then sending the next frame maybe too late.
 
> About time machine problem. between two existing frames we have some count of
> missing frames. What happens in the time between this frames, no body know. At
> least there is no evidence.
> How to fill this empty space is up to codec. Fll it with current frame or with
> next frame, it is just assumption about something what we do not know. But we
> need to tell the codec, how many frames are missing.

I guess this starts to become a philosophical discussion now ;) IMHO the previous frame is always better because it shows what your last observation was instead of showing the future.

Just to follow up, we've closed https://trac.xiph.org/ticket/1730 as 'wontfix'.

As you say, TH_ENCCTL_SET_DUP_COUNT isn't useful for this in a live context. We recommend adjusting the speed-level setting on gsttheoraenc instead.

Ok, as discussed on IRC let's implement this with TH_ENCCTL_SET_DUP_COUNT by adding one frame latency in all cases (or more for dup frames), disable this behaviour by default and add a property to change it.

Alexey said he was going to implement this.

Created attachment 187651 [details] [review]
dup on gap

Here is a new patch. It collect the count of dup frames and make sure it is not bigger than keyframe interval. Also it trys to recreate timestamps of dropped GAP frames.

I also removed function theora_enc_is_discontinuous, it badly fails on variable frame rate. Every time the interval between frames is too big, it reseted the codec, granulopos and forced to make each frame to be a kayframe. 

For testing use:
gst-launch -e oggmux name=mux ! filesink location=test.ogg \
  v4l2src ! video/x-raw-yuv,framerate=30/1 ! videorate skip-to-first=1 ! theoraenc dup-on-gap=1 ! queue ! mux. \
  pulsesrc ! vorbisenc ! queue ! mux.

Created stream seems to work fine with totem, but it looks like mplayer or vlc do not play it correctly.

Regards,
  Alexey

* Please don't mix multiple changes in a single patch. Attach one patch for the dup-property/SET_DUP_COUNT and another one for removing the discont function.

* libtheora will handle the bitrate/keyframe constraints internally already and will not output more dup frames than allowed

* You should keep a queue with buffer timestamps and durations for the dup frames and use these values for the output instead of interpolating

* I think your GAP flag handling is wrong. You should always keep the last non-gap buffer, count the number of gap buffers and on the next non-gap buffer you encode the previous non-gap buffer, get the dups and then keep the new non-gap buffer and do everything from the beginning again

(In reply to comment #25)
> * Please don't mix multiple changes in a single patch. Attach one patch for the
> dup-property/SET_DUP_COUNT and another one for removing the discont function.

ok. i removed it because it will not work with discont function

> * libtheora will handle the bitrate/keyframe constraints internally already and
> will not output more dup frames than allowed

Not correct, if we apply more dups than keyframe freq, th_encode_ctl will return error. And we will get only one frame back. So why should we fail, if we can set a limit for dups?

> * You should keep a queue with buffer timestamps and durations for the dup
> frames and use these values for the output instead of interpolating

I thought about this, but why? The dup setting make sence only with ogg,
also only with constant frame rate, only after videorate. If we get not constant rate it will fail, so or so.

> * I think your GAP flag handling is wrong. You should always keep the last
> non-gap buffer, count the number of gap buffers and on the next non-gap buffer
> you encode the previous non-gap buffer, get the dups and then keep the new
> non-gap buffer and do everything from the beginning again

Ok. This will cure the timemachine effect.

> > * I think your GAP flag handling is wrong. You should always keep the last
> > non-gap buffer, count the number of gap buffers and on the next non-gap buffer
> > you encode the previous non-gap buffer, get the dups and then keep the new
> > non-gap buffer and do everything from the beginning again
> 
> Ok. This will cure the timemachine effect.

Your suggestion looks like this. is it correct?:
After videorate  [1*2**3****4*5*]
Your suggestion  [11*2**3****4*]
This Patch       [12*3**4****5*]

(In reply to comment #26)
> (In reply to comment #25)
> > * Please don't mix multiple changes in a single patch. Attach one patch for the
> > dup-property/SET_DUP_COUNT and another one for removing the discont function.
> 
> ok. i removed it because it will not work with discont function

Ok, please do it in a different patch and explain why it does not work with the dup stuff. It's there a for a reason :)

> > * libtheora will handle the bitrate/keyframe constraints internally already and
> > will not output more dup frames than allowed
> 
> Not correct, if we apply more dups than keyframe freq, th_encode_ctl will
> return error. And we will get only one frame back. So why should we fail, if we
> can set a limit for dups?

Oh, I assumed that it simple outputs non-dup frames for some then. In that case your approach is correct to handle this outside libtheora

> > * You should keep a queue with buffer timestamps and durations for the dup
> > frames and use these values for the output instead of interpolating
> 
> I thought about this, but why? The dup setting make sence only with ogg,
> also only with constant frame rate, only after videorate. If we get not
> constant rate it will fail, so or so.

Dup also makes sense for non-ogg, it's not only videorate that sets the gap flag on buffers and people are using videorate before other muxers too.

Also other elements might set the gap flag on buffers in non-constant framerate streams

(In reply to comment #27)
> > > * I think your GAP flag handling is wrong. You should always keep the last
> > > non-gap buffer, count the number of gap buffers and on the next non-gap buffer
> > > you encode the previous non-gap buffer, get the dups and then keep the new
> > > non-gap buffer and do everything from the beginning again
> > 
> > Ok. This will cure the timemachine effect.
> 
> Your suggestion looks like this. is it correct?:
> After videorate  [1*2**3****4*5*]
> Your suggestion  [11*2**3****4*]
> This Patch       [12*3**4****5*]

My suggestion would be to transform the gap buffers into dup frames, i.e.

After videorate   [1*2**3****4*5*]
After theoraenc [1*2**3****4*5*]

Created attachment 187901 [details] [review]
0001-Remove-theora_enc_is_discontinuous.patch

Created attachment 187903 [details] [review]
0002-Add-new-option-dup-on-gap.patch

This is reworked last patch. The timestams now collected and recovered for new dup frames.

The stream should have same frame order as before theoraenc, no time machine effect.

Review of attachment 187903 [details] [review]:

::: ext/theora/gsttheoraenc.c
@@ +417,3 @@
           (GParamFlags) G_PARAM_READWRITE | G_PARAM_STATIC_STRINGS));
+  g_object_class_install_property (gobject_class, PROP_DUP_ON_GAP,
+      g_param_spec_boolean ("dup-on-gap", "Create DUP frame on GAP flag",

The name of the property could be better I guess... but I currently don't have any suggestions

@@ +1088,3 @@
+{
+  GValue value = { 0 };
+  g_type_init ();

Don't call g_type_init(), that's not required and done as part of gst_init() already

@@ +1093,3 @@
+
+  if (!enc->prevtimes_array)
+    enc->prevtimes_array = g_value_array_new (enc->force_keyframe);

This array should be cleared on flush-stop events and when going from PAUSED to READY in the state change function

@@ +1105,3 @@
+  g_assert (enc->prevtimes_array->n_values == 0);
+  value = g_value_get_uint64 (g_value_array_get_nth (enc->prevtimes_array, 0));
+  g_value_array_remove (enc->prevtimes_array, 0);

Maybe a GQueue or another FIFO data structure would be better here, you're currently copying the complete array when removing an element

@@ +1125,3 @@
+    enc->prevbuf = NULL;
+  }
+  enc->prevbuf = gst_buffer_copy (buffer);

gst_buffer_ref() is enough here

Review of attachment 187901 [details] [review]:

Ok, now I see why this function is causing for VFR input and gap buffers. IMHO it should be fixed to work with VFR input and gap buffers, it's useful to have

(In reply to comment #31)
> This array should be cleared on flush-stop events and when going from PAUSED to
> READY in the state change function

Do we use old prefbuf after this events? Or should it be dropped too?

Yes, drop it too

Created attachment 187950 [details] [review]
0001-Add-new-option-dup-on-gap.patch v3

Review of attachment 187950 [details] [review]:

Did you test that this produces the correct output, as we've discussed it earlier in this bug?
And do you have any idea how to fix the discont check function?

::: ext/theora/gsttheoraenc.c
@@ +1095,3 @@
+  g_assert (enc->t_queue != NULL);
+
+  ptr = g_malloc (sizeof (GstClockTime));

Oh right, we're talking about guint64 here and can't just store them in the GQueue. The best might be to implement your own simple ringbuffer on top of an array then for the queue... or at least use GSlice to allocate the elements.
Sorry

@@ +1343,3 @@
+        theora_timefifo_in (enc, &timestamp);
+        /* We should haveone frame delay to create correct frame order.
+         * First time we got buuffer, prevbuf should be empty. Nothing alse

Typos ;)

@@ +1355,3 @@
+          t_queue_lenght++;
+
+          if (t_queue_lenght > 2) {

and yet another typo, length :)

(In reply to comment #36) 
> Did you test that this produces the correct output, as we've discussed it
> earlier in this bug?

Yes, it reproduce exact same stream.

> And do you have any idea how to fix the discont check function?

It will be good to reset the frame interval counter each time we set a GAP frame in the queue. For simple VFR stream it will make sense to make the frame interval, before reset, tunable or just bigger.

> ::: ext/theora/gsttheoraenc.c
> @@ +1095,3 @@
> +  g_assert (enc->t_queue != NULL);
> +
> +  ptr = g_malloc (sizeof (GstClockTime));
> 
> Oh right, we're talking about guint64 here and can't just store them in the
> GQueue. The best might be to implement your own simple ringbuffer on top of an
> array then for the queue... or at least use GSlice to allocate the elements.
> Sorry

Ok, i will use g_slice_alloc.
 
> @@ +1343,3 @@
> +        theora_timefifo_in (enc, &timestamp);
> +        /* We should haveone frame delay to create correct frame order.
> +         * First time we got buuffer, prevbuf should be empty. Nothing alse
> 
> Typos ;)
> 
> @@ +1355,3 @@
> +          t_queue_lenght++;
> +
> +          if (t_queue_lenght > 2) {
> 
> and yet another typo, length :)

I'll do it USUP after hospital.

What is the actual use case for "discont check function"?

It is use less with videorate and if dup-on-gap is enabled.

It directly affect every thing where webcam is used. With lowlight, discount will reset codec on every frame, the result is a keyframe on every frame and granulepos is reseted as well. In this case mjpeg stream and theora stream do not have a big difference.

Created attachment 189204 [details] [review]
0001-Add-new-option-dup-on-gap.patch v4

New version:
- g_malloc/free replaced with g_slice_*
- reverted discontinuous patch. It will do the same as before if dup-on-gap is disabled.
- corrected some typos.

Created attachment 189205 [details] [review]
0001-Add-new-option-dup-on-gap.patch v5

New update:
- fit the patch to work with new "theora: separate encode and push block in chain, into own function." patch.

Any updates here?

Review of attachment 189205 [details] [review]:

Some typos in the commit message and elsewhere

Also I think you keep the last buffer queued and don't encode/dequeue them on EOS.

::: ext/theora/gsttheoraenc.c
@@ +427,3 @@
+          "This is goot to work with variable frame rate stabelized "
+          "by videorate element, but may be not good for other things "
+          "like rtp stream.",

Typos: "duplicates", "good", "stabilized", "streams"
Say something about live streams instead of just RTP

@@ +1215,3 @@
+{
+  if (enc->dup_on_gap) {
+    guint lenght;

length

@@ +1219,3 @@
+    lenght = g_queue_get_length (enc->t_queue);
+
+    /* there is some thing really bad if lenght is smaller than 1 */

length

@@ +1237,3 @@
+    g_queue_free (enc->t_queue);
+  }
+  /* prevbuf make no sence without timestamps, 

"sense" and "makes"

@@ +1315,3 @@
+    theora_enc_init_buffer (ycbcr, &enc->info, GST_BUFFER_DATA (buffer));
+
+    if (theora_enc_is_discontinuous (enc, running_time, duration)) {

What exactly is the reason now, why the discont detection happens with the dup frames but not in normal mode?

Created attachment 191183 [details] [review]
0001-Add-new-option-dup-on-gap.patch v6

Changes in this patch:
- fixed some typos.
- function theora_enc_encode_and_push no can be used to encode only collected buffers and empty queue.
- EOS will now encode last buffers.
- discontinuous is working even if dup-on-gap activated.

I'm not sure why there is a need for a queue here. What you need to know is the timestamp (and duration) of the last non GAP frame, and the timestamp of the new non GAP frame, no ?

No, because theora is not fixed framerate in other containers than Ogg.

If it's not fixed framerate, then you don't need any dupes, surely.

uff... ogg do not have timestamps for video frames.

(In reply to comment #46)
> If it's not fixed framerate, then you don't need any dupes, surely.

Good point but you might still have duplicates, nothing is preventing elements from setting the GAP flag in a variable framerate stream.

Review of attachment 191183 [details] [review]:

Update common, it should not be included in the patch.
Also please try to fix all the typos.

::: ext/theora/gsttheoraenc.c
@@ +424,3 @@
+  g_object_class_install_property (gobject_class, PROP_DUP_ON_GAP,
+      g_param_spec_boolean ("dup-on-gap", "Create DUP frame on GAP flag",
+          "Allow codec to handle frames with GAP flag as duplicates of next frame. "

Of previous frame, right ?

@@ +573,3 @@
   enc->timestamp_offset = 0;
 
+  theora_timefifo_free (enc);

Will leave the queue pointer dangling. NULL it in that function.

@@ +940,3 @@
       gst_segment_init (&enc->segment, GST_FORMAT_UNDEFINED);
       res = gst_pad_push_event (enc->srcpad, event);
+      theora_timefifo_free (enc);

Dangling too.

@@ +1174,3 @@
+
+static void
+theora_timefifo_in (GstTheoraEnc * enc, GstClockTime * timestamp)

Either pass a GstClockTime, or make the pointer const.

@@ +1239,3 @@
+    if (g_queue_get_length (enc->t_queue))
+      g_queue_foreach (enc->t_queue, (GFunc) theora_free_gstclocktime, NULL);
+    g_queue_free (enc->t_queue);

enc->t_queue = NULL; here to avoid dangling pointers above.

@@ +1318,3 @@
+            t_queue_length -= 2;
+            res = th_encode_ctl (enc->encoder, TH_ENCCTL_SET_DUP_COUNT,
+                &t_queue_length, sizeof (t_queue_length));

This is setting dup count for every gap frame, would it not be better to only set it once we know, before sending the actual frame for encoding ?

@@ +1331,3 @@
+      if (t_queue_length > 1) {
+        res = th_encode_ctl (enc->encoder, TH_ENCCTL_SET_DUP_COUNT,
+            &t_queue_length, sizeof (t_queue_length));

I think you're setting one too much here. For N frames, you encode the frame and N-1 dups.

::: ext/theora/gsttheoraenc.h
@@ +143,3 @@
 GType gst_theora_enc_get_type (void);
+static GstFlowReturn
+theora_enc_encode_and_push (GstTheoraEnc * enc, ogg_packet op,

Why is this here ? It should not be used anywhere else than gsttheoraenc.c.

Created attachment 203893 [details] [review]
0001-Add-new-option-dup-on-gap.patch v7

changes:
- spell checking
- move big comment to reduce "if" block
- fix pointer dangling in theora_timefifo_free

Review of attachment 203893 [details] [review]:

::: ext/theora/gsttheoraenc.c
@@ +1343,3 @@
+      if (t_queue_length > 1) {
+        res = th_encode_ctl (enc->encoder, TH_ENCCTL_SET_DUP_COUNT,
+            &t_queue_length, sizeof (t_queue_length));

You haven't either changed this, or confirmed it it not requesting one dupe too much.

@@ +1394,3 @@
+    GstClockTime timestamp = 0;
+    GST_DEBUG_OBJECT (enc, "encoded. granule:%x, packet:%p, "
+        "bytes:%u", op.granulepos, op.packet, op.bytes);

granulepos is 64 bit, this will not work right on 32 bit archs. Use G_GINT64_FORMAT (not nice to read, but the least bad of options I think).
op.bytes is a long too, use %ld.

@@ +1411,2 @@
   }
+  /* ESO or discontinuos, is not a viald situation, so there is nothing

Typos left.

Created attachment 203910 [details] [review]
0001-Add-new-option-dup-on-gap.patch v8

changes:
- make n-1 dups on eos 
- use G_GINT64_FORMAT and %ld for _DEBUG_
- remove more typos.

Created attachment 203911 [details]
diff v7 vs v8

Created attachment 203945 [details] [review]
theoraenc: add "dup-on-gap" option

This option will produce duplicate frames if we get
a frame with GAP flag. This will reduce CPU load and file size.

This option should be disabled for real time applications, because it
collects GAP frames and waits until it gets a non GAP frame to start
encoding.

v30.06.2011: make some spell changes.
v03.07.2011: add handling of EOS and discontinuous for dup-on-gap.
v19.12.2011: fix pointer dangling in theora_timefifo_free

Bugzilla: https://bugzilla.gnome.org/show_bug.cgi?id=627459

Signed-off-by: Oleksij Rempel (Alexey Fisher) <bug-track@fisher-privat.net>

I fixed the typos I saw. Also moved a repeated warning to a debug with counter.

I tried this patch on a non live+videorate pipeline (encoding some short piece of propaganda with Thoggen), and the patched theoraenc generated invalid granpos (dup-on-gap was not enabled). Something to do with timestamps I suppose, though I've not debugged.

Created attachment 203947 [details] [review]
0001-Add-new-option-dup-on-gap.patch v10

changes:
- Fixed timestamp bug

     if (enc->dup_on_gap && !enc->current_discont)
       timestamp = theora_timefifo_out (enc);
+    else
+      timestamp = GST_BUFFER_TIMESTAMP (buffer);

Also tested with Transmageddon, as well as Thoggen. Files all seem OK now.
Pushed, thanks.

commit 5f3a31f4d1208211a74bdba43ca8644614bc18bf
Author: Oleksij Rempel (Alexey Fisher) <bug-track@fisher-privat.net>
Date:   Mon Dec 19 17:41:23 2011 +0100

    theoraenc: add "dup-on-gap" option
    
    This option will produce duplicate frames if we get
    a frame with GAP flag. This will reduce CPU load and file size.
    
    This option should be disabled for real time applications, because it
    collects GAP frames and waits until it gets a non GAP frame to start
    encoding.
    
    v30.06.2011: make some spell changes.
    v03.07.2011: add handling of EOS and discontinuous for dup-on-gap.
    v19.12.2011: fix pointer dangling in theora_timefifo_free
    v20.12.2010: fix timestamp bug for dup-on-gap=0
    
    Bugzilla: https://bugzilla.gnome.org/show_bug.cgi?id=627459
    
    Signed-off-by: Oleksij Rempel (Alexey Fisher) <bug-track@fisher-privat.net>