Created attachment 46803 [details] [review]
tentative patch

Proposed patch attached.

Fixes problems for me for two DVD titles on same disc. Probably needs a bit
more testing.

Cheers
 -Tim

The previous patch just removes something that's obviously wrong and should keep
things working for the normal case. However, it doesn't really address the
problem of a video starting at a nonzero offset properly.

The proper solution would rather be something along these lines:

/* FIXME: copy from libtheora, theora should somehow make this available for
seeking */
static int
_theora_ilog (unsigned int v)
{
  int ret = 0;
  while (v) {
    ret++;
    v >>= 1;
  }
  return (ret);
}

static GstBuffer *
theora_buffer_from_packet (....)
{
  ......

  /* if duration is 0, it's a header packet and should
   *  have granulepos 0 (so no delta regardless of delay) */
  if (duration == 0) {
    granulepos_delta = 0;
    timestamp_delta = 0;
  } else {
    /* granulepos: lowest ilog bits are number of frames since last
     * keyframe. Bits above that represents the framenumber of the
     * last keyframe. */
    guint ilog = _theora_ilog (enc->info.keyframe_frequency_force - 1);
    granulepos_delta = enc->initial_delay * enc->fps / GST_SECOND;
    granulepos_delta <<= ilog;
    /* The timestamp passed to us comes from theora_granule_time()
     *  based on the granulepos theora created, which counts from 0,
     *  so we have to adjust the timestamp */
    timestamp_delta = enc->initial_delay;
  }

  buf = gst_pad_alloc_buffer (enc->srcpad,
      GST_BUFFER_OFFSET_NONE, packet->bytes);
  memcpy (GST_BUFFER_DATA (buf), packet->packet, packet->bytes);
  GST_BUFFER_OFFSET (buf) = enc->bytes_out;
  GST_BUFFER_OFFSET_END (buf) = packet->granulepos + granulepos_delta;
  GST_BUFFER_TIMESTAMP (buf) = timestamp + timestamp_delta;
  GST_BUFFER_DURATION (buf) = duration;

  ....
}

However, in the last 24 hours I've tried this and several other combinations of
fixes, and there's just nothing that works across the board. I begin to suspect
that playback in totem/playbin is slightly buggered as well though, as some
clips I've encoded end up having a different a/v (un-)sync after a seek than before.

No idea what's going on here. Can't test with mplayer as it crashes on
ogg/theora files, and don't entirely trust totem/playbin, so someone else needs
should probably have a  look at this.

Cheers
 -Tim

So the patch as you're saying is incomplete, and it removes some functionality
that was added to make sure streams stayed synchronized.  Removing it breaks
that. I think this patch should be worked on further and skipped for this release.

Created attachment 47647 [details] [review]
theoradec timestamp improvements (treat with care)

This patch is just an idea basically, and part of the general ogg granulepos
A/V sync effort. To be clear, it's ugly, but I think it simplifies the code a
bit and makes timestamps more reliable in more circumstances.

How it is supposed to work is basically this: an ogg page is made up of
multiple packets. We get these packets one-per-buffer from oggdemux. Only the
last packet of a page carries the page's granulepos in GST_BUFFER_OFFSET_END.
So what we do is we store decoded frames in a queue, and once we get the last
packet of a page, we tag all the buffers with the correct frame numbers and
time stamps and push them all out at once. There usually aren't that many
packets per page in theora streams, so it's not as bad as it sounds. It does
make sure though that even the first frames are stamped correctly, unlike the
current solution.

Note that this has only received limited testing because I don't have reliable
test streams available (our encoders currently produce crack according to
oggz-validate), so handle this patch with care. Problems I've found are A/V
sync after seeking in totem, but that might be due to a whole number of
problems in other plugins that still need to be fixed.

Created attachment 47648 [details] [review]
vorbisdec timestamp improvements (treat with car)

This patch basically makes vorbisdec work according to the same principle as
the theoradec patch above: we store decoded buffers until we get the last
packet of an ogg page, and then stamp the buffers in the queue correctly and
push them all out.

Same disclaimer as above (although I believe that it's more likely that the
encoding is to blame and this patch is sound).

Created attachment 48111 [details] [review]
fix for the granulepos bug

this patch fixes the broken granulepos adjustment; at least it now does what it
intended to do originally.

This has been fixed in the last gst-plugins release (0.8.10) => closing ..

Cheers
 -Tim

Note that I just fixed a similar issue in 0.10. I hope there are very few of these discrepancies still out there.

Is there any automated way to test? Review of 0.8 changes since the branch point for each plugin or something?