Created attachment 152134 [details] [review]
Use default granpos functions for Kate streams, which use the usual granule shift system

Created attachment 152135 [details] [review]
This allows playbin2 to use the element automatically

These two patches are necessary, but not sufficient for playbin2 to work with the tiger element, more patches to follow.

> Created an attachment (id=152134) [details] [review]
> Use default granpos functions for Kate streams, which use the usual granule
> shift system

Ah, thanks. Are you sure this yields the right results though, I get something like this with:

gst-launch-0.10 filesrc location=/home/tpm/samples/kate/Stephen_Fry-Happy_Birthday_GNU-nq_600px_425kbit.ogv ! oggdemux name=d d.serial_7ce15173 ! fakesink -v 

chain   ******* < (   93 bytes, timestamp: 0:00:02.040000000, duration: 5124095:34:33.669551616, offset: 2000000000, offset_end: 214748364800, flags: 0) 0x1026740

chain   ******* < (   80 bytes, timestamp: 0:00:06.040000000, duration: 5124095:34:33.669551616, offset: 6000000000, offset_end: 644245094400, flags: 0) 0x12178a0

chain   ******* < (   41 bytes, timestamp: 0:00:09.040000000, duration: 5124095:34:33.669551616, offset: 9000000000, offset_end: 966367641600, flags: 0) 0x1217c20

Also, could you please put your name into the author field so we don't have to fix this up, thanks!

Created attachment 152145 [details] [review]
With fixed author name.

Created attachment 152146 [details] [review]
With fixed author name

Quite curious. The timestamps output by the katedec element are fine (2, 6, 9 seconds), and I also get the timestamps you reported if I omit the katedec stage.
As the difference (0.04) is the duration of a frame, I suspect something in oggdemux or around does some adjustment meant for Theora granpos. I'll have a look at this, but in the meantime the code is correct, I've just logged the granpos and calculations and the values come out correct (50, 150, 225 at 25 fps).
New patches with fixed author name, and git config updated to make it stick :)
Thanks

Created attachment 152147 [details] [review]
Properly set up the media type for kate streams

Ah, forgot another patch ready to commit. Cut and paste I suspect :)

Created attachment 152148 [details] [review]
Sparse streams aren't timed by end time, and their duration isn't implicit

I found it, some code in oggdemux was assuming timing by end. Your command line now yields the correct timestamps, apart from one case which is fishy which I'll look at next.
I've chosen to handle sparse streams differently, which makes sense for this timestamp issue, but isn't 100% correct, but was strongly implied in the Ogg spec, so is unlikely to go wrong (famous last words).

This patch is ok as a workaround for this release, but the timestamp/duration for a granule needs to be turned into a virtual function for GstOggStreams.  The boolean 'is_sparse' is not related to granules indicating the beginning or end time of a packet (but happens to correlate currently).

I actually went to look for that part of the Ogg doc (not the spec), and this is the part that deals with discontinuous (what GStreamer calls sparse) streams and timing (found in ogg-multiplex.html in the libogg distribution):

  start-time and end-time positioning

   A granule position represents the instantaneous time location between two pages. However, continuous streams and
   discontinuous streams differ on whether the granulepos represents the end-time of the data on a page or the start-time.
   Continuous streams are 'end-time' encoded; the granulepos represents the point in time immediately after the last data
   decoded from a page. Discontinuous streams are 'start-time' encoded; the granulepos represents the point in time of the
   first data decoded from the page.

Created attachment 152249 [details] [review]
Keep track of segments and synchronize timing to them

With this, seeking inside the video does not cause subtitles to disappear.
Testing with Totem, switching streams (eg, en to fr subtitles) does not work, and actual languages are not displayed in the selection menu. As far as I can see, these are the only remaining problems. So far. Which I'll be looking at next.

With the same file and playbin2 I now get warning messages from subtitleoverlay (not-negotiated, could not get subtitle caps or somesuch), even though I now do see the text overlay "Computers"

(In reply to comment #12)
> With the same file and playbin2 I now get warning messages from subtitleoverlay
> (not-negotiated, could not get subtitle caps or somesuch), even though I now do
> see the text overlay "Computers"

The problem here is, that katedec pushes stuff (in this case the NEWSEGMENT event from upstream) downstream through it's srcpad. It should set the caps on the srcpad first and then send NEWSEGMENT and other serialized events downstream, possibly caching them when they need to be forwarded from upstream.

I'm not getting any of those. I use:
GST_DEBUG=playbin2:4,subtitleoverlay:4 gst-launch playbin2 uri=file://somefile
Can you give me your command line please ?

Created attachment 152322 [details] [review]
Don't escape special characters, as we'll send that buffer as text/plain, not x-pango-markup

1
00:00:00,401 --> 00:00:02,925
Art Co-op "Ekran" presents

would yield:

Art Co-op "Ekran" presents

Oddly enough, I'm now getting what is probably what you were seeing, so I'll check it out. I'm pretty sure I did not get these earlier, as I grepped the log for 'nego' after running a few times, and there were no matches...

WARNING: from element /GstPlayBin2:playbin20/GstPlaySink:playsink0/GstBin:tbin/GstSubtitleOverlay:suboverlay: Internal GStreamer error: negotiation problem.  Please file a bug at http://bugzilla.gnome.org/enter_bug.cgi?product=GStreamer.
Additional debug info:
gstsubtitleoverlay.c(709): _pad_blocked_cb (): /GstPlayBin2:playbin20/GstPlaySink:playsink0/GstBin:tbin/GstSubtitleOverlay:suboverlay:
Subtitle sink is blocked but we have no subtitle caps

Created attachment 152354 [details] [review]
Give the tiger element primary rank, so other patches may be tested along with playbin2

Got it:
I'm now getting those messages because it's using katedec rather than tiger.
If you're seeing these, it's because tiger isn't primary ranked, I haven't sent a patch to change that as it's not yet working fully. That explains why I wasn't seeing these earlier. The clue was in Sebastian's mention of katedec, but at the time I thought he meant tiger since it was the one I was working on.
The attached patch does just that, but should only be committed once it all works fine.
Meanwhile, I'll have a look at those messages from katedec.

Created attachment 152453 [details] [review]
Set the function ptr for the sink pad, not the source pad - mismatch

Created attachment 152455 [details] [review]
katedec now keeps track of segments too, and clips incoming buffers to them

This should be applied after "152249: Keep track of segments and synchronize timing to them", as it moves some of the code to the gstkateutil so it can be shared.
I now don't get the complaints about subtitle caps, but I'm afraid I don't know well enough GStreamer to work out whether I fixed it, or if it's gone for another reason, sorry.

Hi,
I noticed there's a release of -base about to be done, would it be possible to apply http://bugzilla-attachments.gnome.org/attachment.cgi?id=152145 ?
It's pretty simple and unbreaks decoding granpos to time (which means subtitles actually get displayed again).
The other patches can wait of course, just this small one seems like a good candidate to get in before the release.
Thanks.

And after updating to the latest git trees, I now get the subtitle caps logs again. Looking at it... :S

Created attachment 153537 [details] [review]
keep track of segments, and delay events till caps are known

With this patch, Totem now works with katedec for the outstanding problems (language tags appearing in the selection menu, seeking, changing language at run time).
There's still a remaining problem, that sometimes the video freezes (only after changing language a few times apparently). Seeking makes it start again. I haven't worked out why yet.

Totem does not work with tiger for switching languages. For some reason, headers aren't received, so the decoder can't initialize itself, and receives data packets which it can't decode yet. Any clue as to what I'm doing wrong there would be welcome to direct my debugging.

Also: I did the event delaying because you said it was needed, but I have no clue why it is needed in the first place. I'm blind here. Is it needed for tiger too ?

Created attachment 156681 [details] [review]
More work on segment handling

This builds upon 153537, and should be applied after it.
Totem likes Kate streams with text subtitles, but not ones with image subtitles.
I have not yet found why. I suspect playbin2 or subtitleoverlay do different things for text/x-pango-markup and video/x-dvd-subpicture.
Comments welcome on my blind exploration of gst's world :)

Review of attachment 153537 [details] [review]:

Sorry for taking that long to review

First of all, please try to split your changes into separate commits, all doing a single change, instead of one large commit that does $stuff. That makes it much easier to review

::: ext/kate/gstkatedec.c
@@ +393,3 @@
+  switch (transition) {
+    case GST_STATE_CHANGE_PAUSED_TO_READY:
+      g_queue_free (kd->event_queue);

You're going to leak all events here that were not pushed downstream yet

@@ +425,3 @@
+
+  // Delay events till we've set caps
+  if (kd->delay_events) {

You should never delay flush-start, flush-stop and eos events, only all others

@@ +460,3 @@
+
+      gst_event_parse_seek (event, &rate, &format, &flags, &cur_type, &cur,
+          &stop_type, &stop);

No need to parse the seek event here if you're not going to use the values anyway

::: ext/kate/gstkatetiger.c
@@ +652,3 @@
+        kate_float t =
+            gst_segment_to_stream_time (&tiger->video_segment, GST_FORMAT_TIME,
+            tiger->video_segment.last_stop) / (gdouble) GST_SECOND;

If you're using this for synchronization you should really use running time instead of stream time

@@ +892,3 @@
+      }
+
+      res = gst_pad_event_default (pad, event);

Only forward events here if the source pad caps are set already, same story as in katedec

::: ext/kate/gstkateutil.c
@@ +168,3 @@
     GST_ELEMENT_ERROR (element, STREAM, DECODE, (NULL),
         ("Failed to decode Kate packet: %d", ret));
+    gst_util_dump_mem (kp.data, kp.nbytes);

Please remove this :)

Review of attachment 156681 [details] [review]:

Also feel free to squash commits together if they belong together ;)

::: ext/kate/gstkatetiger.c
@@ +562,3 @@
   if (gst_kate_util_decoder_base_update_segment (&tiger->decoder,
           GST_ELEMENT_CAST (tiger), buf)) {
+    GstPad *tagpad = gst_pad_get_peer (pad);

You should probably unref this pad somewhere again

@@ +868,3 @@
+    // Delay events till we've set caps - only tag events though, as doing so for the rest seems to freeze
+    // the pipeline for reasons I don't know, and the reason why katedec does it is not valid here (as katedec
+    // sets the src pad caps at runtime, but tiger does not)

Yes, as said in the review of the other patch... delay everything except flush-start, flush-stop and eos :) newsegment delaying until the caps are know is very important

::: ext/kate/gstkateutil.c
@@ +129,3 @@
   decoder->original_canvas_height = 0;
+  if (decoder->event_queue) {
+    g_queue_free (decoder->event_queue);

Same as in the other patch, make sure to unref all events and free all data still in the queue before destroying the queue

@@ +142,3 @@
+    GstKateDecoderBaseQueuedEvent *item;
+    GST_WARNING_OBJECT (decoder, "We have to delay the event");
+    item = g_malloc (sizeof (GstKateDecoderBaseQueuedEvent));

Use g_slice_new() here and g_slice_free() later for freeing it again

I've been looking at this again, and I'm stumped by this:

WARNING: from element
/GstPlayBin2:playbin20/GstPlaySink:playsink0/GstBin:tbin/GstSubtitleOverlay:suboverlay:
Internal GStreamer error: negotiation problem.  Please file a bug at
http://bugzilla.gnome.org/enter_bug.cgi?product=GStreamer.
Additional debug info:
gstsubtitleoverlay.c(709): _pad_blocked_cb ():
/GstPlayBin2:playbin20/GstPlaySink:playsink0/GstBin:tbin/GstSubtitleOverlay:suboverlay:
Subtitle sink is blocked but we have no subtitle caps

Looking at the logs, I do set the source pad caps to something simple (either text/plain or video/x-dvd-subpicture) when the first packet is received. This works fine as far as I can tell.
Later, subtitleoverlay requests negotiated caps and gets NULL, and moans with the error above. I just can't find how to stop this from happening. I suspect that, since subtitleoverlay gets the caps from the input selector, the caps from the katedec source aren't getting propagated to that, somehow.
I've tried adding a getcaps and setcaps function to katedec, but that doesn't help. Either subtitleoverlay sees NULL, or the template caps (which are compound).
Any idea just what I might be doing wrong ?

Thanks

Created attachment 177023 [details] [review]
kate: add segment tracking, and various other improvements

Updated after fixing the oggdemux part.
That annoying warning is now gone, and stream switching works in Totem (with a 30 second wait till the other track shows up due to multiqueue being overeager, but I have a proof of concept that fixes that).
A patch to come after that will make tiger use headers from caps if it isn't given any headers.
One thing left is that when using tiger, text tags aren't recognized: that's because playbin2 asks the raw pads directly, and the stream isn't decoded by that time, so tags (including language) aren't known there.

That multiqueue fix sounds very interesting :)

oggdemux should in the future extract and post tags for every codec, currently it only does this for VP8 and OGM subtitles. There are patches for this in bugzilla somewhere

commit 2a2c76cdbd89ee0c587b1dab2d196e6d446a9e37
Author: Vincent Penquerc'h <ogg.k.ogg.k@googlemail.com>
Date:   Mon Jan 25 18:26:25 2010 +0000

    tiger: Give tiger primary rank

commit 8574e8f991f68506af3a0c369c933020688bd4c3
Author: Vincent Penquerc'h <ogg.k.ogg.k@googlemail.com>
Date:   Mon Jan 25 18:58:38 2010 +0000

    kate: add segment tracking, and various other improvements
    
    https://bugzilla.gnome.org/show_bug.cgi?id=600929

Thanks for the commit \o/

I've got a patch for the language thing, using using that very tag system.
I'll post that in a new bug now.

Also note that GST_SEEK_TYPE_CUR is relative to the beginning of the current segment, not relative to the current position (just don't support SEEK_TYPE_CUR, very little else does).

Created attachment 177046 [details] [review]
kate: if seeking with GST_SEEK_TYPE_CUR, flush everything

We don't know how to calculate the target, so be safe.

Thanks for the heads up, patch attached.

Comment on attachment 177046 [details] [review]
kate: if seeking with GST_SEEK_TYPE_CUR, flush everything

We should be able to calculate it somehow I think, but this will work too I guess :)

> If you're using this for synchronization you should really use running time
> instead of stream time

Are you really sure about that ? After making that change, subtitles don't get displayed after a seek. I'm attaching a patch that reverts to the original stream time behavior, and it works again. Would this break something else ?

Created attachment 177930 [details] [review]
kate: use running time instead of stream time

Using stream time breaks with seeking.

Do you reset/reinit the text and video segments if you get a FLUSH event (or when handling a flushing seek)? Using running time for synchronizing different streams is definitely the right thing to do

Yes, I do reset segments (the text one on FLUSH START/STOP on the text pad, the video one on FLUSH START/STOP on the video pad).

Created attachment 178156 [details] [review]
kate: ensure the kate pad does not shoot ahead of the video pad

Sync both pads by waiting in the kate chain function.
Do not reset our internal segment from segment updates, in order
to be able to map video running time to kate running time, to
give libtiger the timestamp it expects. This allows us to use
running time to sync to video, which is The Right Way.

It would be nice it this could go on the release despite the freeze. Since the element had rank NONE, this would not cause any regression.

Comment on attachment 178156 [details] [review]
kate: ensure the kate pad does not shoot ahead of the video pad

>+#define GST_KATE_TIGER_GET_COND(element) ((element)->cond)
>+#define GST_KATE_TIGER_WAIT(element) (g_cond_wait (GST_KATE_TIGER_GET_COND (element), (element)->mutex))        //GST_OBJECT_GET_LOCK (element)))
>+#define GST_KATE_TIGER_SIGNAL(element) (g_cond_signal (GST_KATE_TIGER_GET_COND (element)))
>+#define GST_KATE_TIGER_BROADCAST(element) (g_cond_broadcast (GST_KATE_TIGER_GET_COND (element)))

No C++ comments please (guess the GST_OBJECT_GET_LOCK can just be removed?).

Nitpick: It's easy to shoot yourself in the foot with this kind of macro definition (with element not cast to GstKateTiger* or whatever before dereferencing in GET_COND and in _WAIT).

> It would be nice it this could go on the release despite the freeze.
> Since the element had rank NONE, this would not cause any
> regression.

Go for it.

Created attachment 178157 [details] [review]
kate: ensure the kate pad does not shoot ahead of the video pad

Sync both pads by waiting in the kate chain function.
Do not reset our internal segment from segment updates, in order
to be able to map video running time to kate running time, to
give libtiger the timestamp it expects. This allows us to use
running time to sync to video, which is The Right Way.

Heh, I forgot this when I removed the debug bits before attaching.
I've removed those macros, they're unneeded anyway.
Thanks.

Shouldn't break anything, since katetiger isn't autoplugged anyway at the moment:

 commit 0b790b663c1423948794302b6ed88cefab236cff
 Author: Vincent Penquerc'h <vincent.penquerch@collabora.co.uk>
 Date:   Wed Jan 12 16:39:22 2011 +0000

    kate: ensure the kate pad does not shoot ahead of the video pad
    
    Sync both pads by waiting in the kate chain function.
    Do not reset our internal segment from segment updates, in order
    to be able to map video running time to kate running time, to
    give libtiger the timestamp it expects. This allows us to use
    running time to sync to video, which is The Right Way.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=600929