ogg dev - Dec 2008 - liboggplay: RGBA overlay video, rendering with libtiger

Hi,

the attached patch adds rendering of Kate streams
using libtiger. To do so, it adds a new type of RGBA video.
One can request the incoming video (in YUV) to be sent
in RGBA (ie, asking liboggplay to do the conversion in the
first place).

The large changes in the GLUT player also include fixes
for crash-on-no-sound, mono sound, and locking fixes
(I *think* - at least it works fine for me now).

If there are no comments and/or vetoes, I'll commit this
in a few days (plus probably fixes for big endian archs
relating to the YUV/RGB conversion, I've found a nice
chap with a big endian arch willing to do some tests).

Cheers
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Tiger-rending-support-for-Kate-streams.patch
Type: application/octet-stream
Size: 42571 bytes
Desc: not available
Url :
http://lists.xiph.org/pipermail/ogg-dev/attachments/20081215/901d5397/attachment-0001.obj

Hi ogg.k ogg.k, some drive by comments on the patch from a quick look.

Should these be wrapped in HAVE_KATE? Same with the definition of the
functions on oggplay.c? If I'm not building with Kate support it would
prevent dead code from being around.
-----------------8<---------------------------
OggPlayErrorCode
oggplay_get_kate_category(OggPlay *me, int track, const char** category);

OggPlayErrorCode
oggplay_get_kate_language(OggPlay *me, int track, const char** language);

OggPlayErrorCode
oggplay_set_kate_tiger_rendering(OggPlay *me, int track, int use_tiger);

OggPlayErrorCode
oggplay_overlay_kate_track_on_video(OggPlay *me, int kate_track, int
video_track);
-----------------8<---------------------------

Typo in the comment (is should be if):

+  unsigned char   * rgba; /* may be NULL is no alpha */

In glut_player.c, If realloc fails then it returns NULL and this will
overwrite the existing value of 'buffer' with null. That
'buffer' will
then never be free'd and result in a memory leak. Although I suspect
it'd crash anyway due to buffer being NULL when float_to_short_array
is called:

+    buffer = realloc(buffer, size * sizeof (short) * channels);

In oggplay.c there are checks like the following that do 'track
>me->num_tracks' and at least one that does 'track >
me->num_tracks'. I
think it should be '>' not '>=' in all cases, what do you
think? Code
following usually indexes into decode_data by the track number which
is an error if the track == num_tracks I think.

+  if (track < 0 || track >= me->num_tracks) {
+    return E_OGGPLAY_BAD_TRACK;

Check for NULL return from malloc needed here (the sample problem
occurs in existing code with 'record' but a bug has been raised about
that):

+    orecord = (OggPlayOverlayRecord*)malloc (size);
+    oggplay_data_initialise_header((OggPlayDecode *)decode,
&(orecord->header));

NULL check needed here:

+      record = (OggPlayOverlayRecord*)calloc (1, size);
+      record->header.samples_in_record = 1;

Chris.

> Should these be wrapped in HAVE_KATE? Same with the definition of the
> functions on oggplay.c? If I'm not building with Kate support it would
> prevent dead code from being around.
They could, but would change the API, and the implementation just
returns a "not implemented" error currently if HAVE_KATE is undefined,
(and a couple checks for validity) so it's not much dead code at all.
If the 'moving target API' isn't a problem I can change it so.
> In glut_player.c, If realloc fails then it returns NULL and this will
> overwrite the existing value of 'buffer' with null. That
'buffer' will
> then never be free'd and result in a memory leak. Although I suspect
Good point. Common lazy programer idiom. Will fix.
> In oggplay.c there are checks like the following that do 'track
>> me->num_tracks' and at least one that does 'track >
me->num_tracks'. I
> think it should be '>' not '>=' in all cases, what do
you think? Code
I think it should be >= all the time. I'll check those out.
> Check for NULL return from malloc needed here (the sample problem
> occurs in existing code with 'record' but a bug has been raised
about
> that):
Yes, adapted from existing, will change.
> NULL check needed here:
>
> +      record = (OggPlayOverlayRecord*)calloc (1, size);
> +      record->header.samples_in_record = 1;
OK

Thanks for the comments!

With feedback incorporated.
> Should these be wrapped in HAVE_KATE? Same with the definition of the
I've left the public APIs in, but the entire body is now defined out
apart from the error return code (but this means that using a
wrong track id or type won't be caught, FWIW). Wouldn't be nice
to others (eg, Python bindings) to remove the call altogether, I'd
prefer to let them in with the int foo() { return ENOTIMPL;} kind
of stub, unless you really want the extra handful of bytes.

[unchecked memory allocation]

I've fixed mine and a couple others, but some of the remaining
ones are in code I'm not too familiar with, so I'll look into them
later as it's not obvious *what* should be done should they fail.
> In oggplay.c there are checks like the following that do 'track
>> me->num_tracks' and at least one that does 'track >
me->num_tracks'. I
The only culprit was oggplay_set_offset, fixed.

BTW, I'd already committed a couple 'easy' fixes to liboggplay,
in case you want to have a look too. That last oggplay_set_offset
could go in now too, though I don't know what it's used for.

Cheers
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lop.diff
Type: application/octet-stream
Size: 42687 bytes
Desc: not available
Url :
http://lists.xiph.org/pipermail/ogg-dev/attachments/20081216/70827965/attachment-0001.obj