And, additionally, mdvd parser, if won't find number of FPS in the file, like:

{1}{1}25.000

it will use 23,976 (it's hardcoded). IMHO the parser should use the movie's FPS (for example Totem shows 25.000 FPS and it's working fine). Some movies are encoded in 25.000 FPS and there is no information in the subtitle file about this. Windows players handle this perfectly, though.

And, there is something wrong with this line:

  in_seg = gst_segment_clip (state->segment, GST_FORMAT_TIME,
      state->start_time, state->start_time + state->duration, &clip_start,
      &clip_stop);

Sometimes, after seeking, the subtitles are not shown, even if they are supposed to be shown. GStreamer will show the next line and continue to show subs properly then, it's only after seeking.

I discovered, that this can be fixed by changing this:

  if (in_seg) {
    state->start_time = clip_start;
    state->duration = clip_stop - clip_start;
  } else {
    return NULL;
  }

to this:

  if (in_seg) {
    state->start_time = clip_start;
    state->duration = clip_stop - clip_start;
  }

We are simply parsing subs even if (in_seq) shows that we don't have to. Seeking forward works slightly slower, though, so that's not what we want (on my 3ghz Celeron comp GStreamer is the fastest engine, this change slows it down so it works like few Windows players). Haven't found a solution yet.

I think I'll rewrite this function from scratch, I've been creating a media player based on GStreamer and I've created my own subtitle parser - with Pango markup, and it was working great.

These two patches (fix for second patch, please?), and ability to read FPS not from file but just from the movie, and GStreamer will finally handle MicroDVD subtitles properly.

Thanks for the fix, it should work fine in CVS now:

 2008-02-23  Tim-Philipp Müller  <tim at centricular dot net>

        Based on patch by: Tomasz Sałaciński <tsalacinski gmail com>

        * gst/subparse/gstsubparse.c: (parse_mdvdsub):
        * tests/check/elements/subparse.c: (test_microdvd_with_italics),
          (subparse_suite):
          Forward slashes at the beginning and end of a line also signify
          italics (Fixes: #518162).

(I'm not entirely clear whether the forward/end slashes may span multiple lines though - do they? If so, we currently don't handle that properly).


This: "line = g_strndup(line, strlen(line) - 1);" leaks by the way.


(In reply to comment #1)
> And, additionally, mdvd parser, if won't find number of FPS in the file, like:
> 
> {1}{1}25.000
> 
> it will use 23,976 (it's hardcoded).

It *should* parse the fps from that line, there's even a unit test to make sure it does. Could you investigate why it fails? 23.976 is only used as fallback when we don't find a hard-coded framerate.


> IMHO the parser should use the movie's FPS

If no framerate is specified, any framerate is as good as any other really. microdvd subs without framerate are per definition really pretty much broken, it's a frame-based format after all :)  Also, the subparse element does not have access to the framerate of the video either, it would need to be set via a property by the player etc. etc.


> (for example Totem shows 25.000 FPS and it's working fine). Some movies are
> encoded in 25.000 FPS and there is no information in the subtitle file about
> this. Windows players handle this perfectly, though.

That's mostly just luck then (admittedly the 23.976 fallback is somewhat unfortunate for european users of course).

I will accept a patch that reads the default framerate from an environment variable though.



> And, there is something wrong with this line:
>   (snip) 
> Sometimes, after seeking, the subtitles are not shown, even if they are
> supposed to be shown. GStreamer will show the next line and continue to show
> subs properly then, it's only after seeking.

I don't think this is a problem in subparse, it's a problem in the textoverlay element and has to do with keyframe seeking. I've got a fix for that somewhere though, I should commit that, thanks for reminding me.

> (I'm not entirely clear whether the forward/end slashes may span multiple lines
> though - do they? If so, we currently don't handle that properly).

The thing is, that leading slash makes the line italic, trailing slash is not doing anything, just sometimes subtitle authors are putting / italic text / into the subs and media players are removing the trailing slash (for ex. polish media players like SubEdit player).

And, media players are reading the FPS for subtitles from the file and when it won't succeed, they are loading them from the movie.

Maybe adding a movie_fps float to state structure can resolve the problem?

> I don't think this is a problem in subparse, it's a problem in the textoverlay
> element and has to do with keyframe seeking. I've got a fix for that somewhere
> though, I should commit that, thanks for reminding me.

I agree with you, the code looks perfect to me. Removing it slows down gstreamer, but fixes this bug.


> It *should* parse the fps from that line, there's even a unit test to make sure
> it does. Could you investigate why it fails? 23.976 is only used as fallback
> when we don't find a hard-coded framerate.

And it does parse the FPS number from this line. It does it perfectly, so IMHO there is no need to check anything.

Actually, the only thing that will make life much easier, is the current frame. But I believe it won't be easy to pass the current frame number to subparse...