Bugzilla Bug 103299

There is some signedness 'bugs' in ogm rip, I'll attatch the diffs when this
bug
is opened. (please forward the patch or tell me to)

Reproducible: Always
Steps to Reproduce:
1.
2.
3.

Created an attachment (id=66516) [edit]
Fixes for the subrip sources.

+  int pts100; /* FIXME: Should perhaps be time_t */

I don't know how large that timestamp can be, ir if it's a delta or not..

Created an attachment (id=66517) [edit]
Signedness fixes for libogmrip.

-  unsigned int pts100;
+  int pts100; /* FIXME: Should perhaps be time_t */

if that comment is true then it should be fixed ... time_t is not always the
same size as int depending on the host architecture, so passing
references/pointers could cause issues

(In reply to comment #3)
> -  unsigned int pts100;
> +  int pts100; /* FIXME: Should perhaps be time_t */
> 
> if that comment is true then it should be fixed ... time_t is not always the
> same size as int depending on the host architecture, so passing
> references/pointers could cause issues

well as i stated in the patch comment, if its just a delta or so then it's ok i
assume... but...(In reply to comment #3)

The patch to fix the signedness in libogmrip makes ogmrip compile cleanly with
gcc4 but this is not the right way to fix this issue. In fact, there is a bug in
ogmrip_codec_get_chapters and ogmrip_codec_set_chapters definition because the
last chapter to encode might be negative if it should be the last chapter of the
title. That is, the 'end' parameter should be gint, not guint. I'll provide a
patch as soon as I'm back from holydays.

Regarding the subrip patch, I think it should be better to change
int vobsub_get_next_packet (void *vobhandle, void **data, int *timestamp)
to
int vobsub_get_next_packet (void *vobhandle, void **data, unsigned int *timestamp)
in both vobsub.h and vobsub.c instead of removing the unsigned qualifier to
pts100 in spudec.c.

Created an attachment (id=66595) [edit]
Something more along the lines of this?

I think i cought all of the comments, I wonder about set_chapters though,
should that also use signed ints?

that looks acceptable to me at a glance

perhaps try contacting the upstream maintainers too while you're at it ? :)

Created an attachment (id=66778) [edit]
gcc4 patch

This patch improves Ian's patch: it defines the get_chapters and set_chapters
functions correctly. Thanks Ian for your work.

(In reply to comment #9)
> This patch improves Ian's patch: it defines the get_chapters and set_chapters
> functions correctly. Thanks Ian for your work.

NP =), Glad i could help =)

Patch commited in CVS. Special thanks all for your work.