Created attachment 72149 [details]
New element to add support for REAL version 3 and 4: gstrealdec

Created attachment 72150 [details]
Makefile needed for compilation

Created attachment 72270 [details]
New element to add support for REAL version 3 and 4: gstrealdec

I'm absolutely no expert in gstreamer but isn't it better to put this in gstreamer-pitfdll?

Isn't that why Ronald Bultje made gstreamer-pitfdll (http://ronald.bitfreak.net/pitfdll.php) to have a proprietary dll loader outside gstreamer "base" plugins.

For the record i _really_ miss this in gstreamer. Would make my day if it got in. In gstreamer-ugly or gstreamer-pitfdll.

(In reply to comment #4)
> I'm absolutely no expert in gstreamer but isn't it better to put this in
> gstreamer-pitfdll?
> 
> Isn't that why Ronald Bultje made gstreamer-pitfdll
> (http://ronald.bitfreak.net/pitfdll.php) to have a proprietary dll loader
> outside gstreamer "base" plugins.

  This patch is to wrap the linux .so real codecs, and not win32 dlls. So it shouldn't go in pitfdll.

> 
> For the record i _really_ miss this in gstreamer. Would make my day if it got
> in. In gstreamer-ugly or gstreamer-pitfdll.
> 

Created attachment 72932 [details] [review]
New element to add support for REAL version 3 and 4: gstrealdec

Created attachment 72933 [details]
Test program to demonstrate that the element actually works.

Created attachment 72934 [details]
Makefile needed for compilation

Created attachment 72952 [details]
New element to add support for REAL version 3 and 4: gstrealdec

Fixed timestamps.

Created attachment 72953 [details]
Test program to demonstrate that the element actually works

Created attachment 73000 [details]
Test program to demonstrate that the element actually works

Created attachment 73001 [details]
New element to add support for REAL version 3 and 4: gstrealdec

*** Bug 362691 has been marked as a duplicate of this bug. ***

Will this patch be accepted by gst-plugins-ugly? 

I cannot find a single file that this element will decode.  The error is always:

Could not decode buffer: 2147942487

According to MPlayer reverse engineering docs this corresponds to E_INVALIDREG.

Aside from that, some things to note:

Such error code/developer information belongs in the debug message part you pass to GST_ELEMENT_ERROR.  Just use (NULL) for the message (which will get substituted by the standard message for that error).

Another thing that caught my eye is this:
 result = dec->transform (
        (gchar *) gst_adapter_take (dec->adapter, dec->length),
        (gchar *) GST_BUFFER_DATA (out),
        tin, tout, dec->context);

I somehow doubt that the proprietary driver calls g_free on the first argument, therefore the element leaks _all_ data that is passed in!  You can also get rid of all that casting to gchar * by defining the prototype for the transform function to use data arguments of type guint8 *, which makes more sense (this is why GstAdapter and GstBuffer use this type).

I must say that you make it relatively hard for people to try out your elements.  You should provide a single patch file that sets up the full autotool integration alongside the source.  This way, people only need to grab a single file and can fully install the plugin into a checked out tree with just a single command.

P.S.: This should be against gst-plugins-bad, regardless of the fact that the final destination is doubtlessly -ugly.

I haven't used this element with files. I tested it using the live streams on http://www.tagesschau.de. This patch is a proof of concept - I did not claim that this element is ready for production. 

The first problem was to implement the proprietary hand-shake algorithm in the rtsp element (#341752).

The second problem was to decode the video stream. This is what this element is for.

Now on to your comments: 

(1) error messages: As long as the element is not fully working (i.e. audio/video in sync, no image problems...), the error messages will help improving the driver.

(2) gst_adapter_take: You are right. That explains why the pipeline got slower and slower in time... That's exactly why I published the patch - to get feedback.

(3) patch format: I'll add the configure.ac modification as a separate patch. If you can tell me how to tell cvs diff to include the new files in the resulting file, I'd be very grateful.

(In reply to comment #16)
> I haven't used this element with files. I tested it using the live streams on
> http://www.tagesschau.de. This patch is a proof of concept - I did not claim
> that this element is ready for production. 
> 

OK.  I figured that it should work from the subsequent comments.  Thanks for clearing this up :-)

Any idea why it doesn't work with files?  More or less randomly poking around in the code, the best thing I could get was a green frame.

[...] 
> (1) error messages: As long as the element is not fully working (i.e.
> audio/video in sync, no image problems...), the error messages will help
> improving the driver.
> 

OK, e.g. Totem does not show the debug part of the error message.

[...]
> (3) patch format: I'll add the configure.ac modification as a separate patch.
> If you can tell me how to tell cvs diff to include the new files in the
> resulting file, I'd be very grateful.

I don't know anything about CVS, I'm waiting for it to die.

If you don't have write access to the repository, all changes are private to yourself anyways so there's no reason to rely on CVS.  To obtain a patch to send in, one normally uses diff -Naur against an unmodified tree.  If this becomes harder to manage, you can use your favorite revision control system to practically fork the tree locally (one preferably picks a system where this is easy).

Created attachment 78839 [details]
New element to add support for REAL version 3 and 4: gstrealdec

Element now 'works' (you see some frames) with file iz120a-stops_at_67s-RV30.rm (http://samples.mplayerhq.hu/real/VC-RV30/).

Created attachment 78961 [details]
New element to add support for REAL version 3 and 4: gstrealdec

All frames of file iz120a-stops_at_67s-RV30.rm are now decoded correctly (though I am not sure about timestamps).

Hi, it seems to decode all videos fine. But as you mentionned there is still an issue:
_ timestamps on outgoing buffers seem wrong (the video playback is stuttered). For some reason, some outputted buffers don't have any timestamps (GST_CLOCK_TIME_NONE). Also, the buffers don't seem to be timestamped with regular intervals. Maybe you should have a look at how mplayer handles that.

The problem seems to come from gst_realdec_decode(). At the end, if there's some data left in the adapter, you call the function again with a new buffer... but that buffer doesn't have a valid timestamp. So eventually the timestamping gets broken. Unfortunately, I don't know enough about the format to know what should be done. Mplayer seems to do the timestamp correction in the demuxer... which doesn't seem right.. but plays fine all the same.

Also... I can't find any video that has frametypes different from 0 or 1 ! Are you sure there isn't something wrong there ?

I think the timestamp problems IS because of the frametypes issue. Every time there is enough data to call _decode() again, the buffer is very small (a few hundred bytes, 4 or 5 times smaller than a keyframe), which is a very good hint that it is NOT a keyframe. And if you look at the timestamp calculation for non-keyframes, you will notice they don't take into account the incoming timestamp.

Triple-check the frame_type detection, I'm pretty sure the error is there.

*sigh*. The bug is in fact even more stupid than that. It took me some time to realize dec->version was set to 2, whereas it's a rv30 stream.

The error is in gst_realdec_setcaps() . When you check some versions, you are using '=' instead of '=='

so you need to replace:

  if (((dec->version = GST_REAL_DEC_VERSION_2) ||
        (dec->version = GST_REAL_DEC_VERSION_3)) &&
      (dec->format >= 0x20200002)) {
    guint32 msg[15];

by

  if (((dec->version == GST_REAL_DEC_VERSION_2) ||
        (dec->version == GST_REAL_DEC_VERSION_3)) &&
      (dec->format >= 0x20200002)) {
    guint32 msg[15];


Once you do that, playback is perfect.

Thanks, I cleaned up the patch a bit more and commited it.

2007-01-01  Lutz Mueller <lutz@topfrose.de>

	reviewed by: Edward Hervey  <edward@fluendo.com>

	* configure.ac:
	* gst/real/Makefile.am:
	* gst/real/gstreal.c: (gst_realdec_alloc_buffer),
	(gst_realdec_decode), (gst_realdec_chain),
	(gst_realdec_activate_push), (gst_realdec_setcaps),
	(gst_realdec_init), (gst_realdec_base_init),
	(gst_realdec_change_state), (gst_realdec_finalize),
	(gst_realdec_set_property), (gst_realdec_get_property),
	(gst_realdec_class_init), (plugin_init):
	New plugin for decoding RealVideo Streams using the x86 32bit
	shared libraries.
	Closes #354174

Should the codecs be in:
#define DEFAULT_PATH_RV20 "/usr/lib/win32/drv2.so.6.0"
#define DEFAULT_PATH_RV30 "/usr/lib/win32/drv3.so.6.0"
#define DEFAULT_PATH_RV40 "/usr/lib/win32/drv4.so.6.0"

When its not win32 codecs but native linux codecs?

In Ubuntu Edgy they are installed in:
/usr/lib/codecs/drv2.so.6.0
/usr/lib/codecs/drv3.so.6.0
/usr/lib/codecs/drv4.so.6.0

Our ways crossed - I continued to work on the driver and renamed it gstrealvideodec - in order to be able to provide in the same directory the audio decoder (gstrealaudiodec). I can't see the work you checked in CVS yet (anonymous access). What should I do? Can you still rename the decoder or should I write the audio decoder in another directory?

Regarding the default paths: The debian package w32codecs installs the codecs in /usr/lib/win32. The default paths should probably be determined by the configure script.

There is one issue I fixed today in the video decoder: Right after the call to dec->init in the setcaps function, I've hardcoded some bits. You need to replace the code by the code below:

  res = dec->init (&data, &dec->context);
  if (res)
    goto could_not_initialize;

  if ((v = gst_structure_get_value (s, "codec_data"))) {
    GstBuffer *buf = g_value_peek_pointer (v);
    guint32 *msg = g_new0 (guint32, 11 + GST_BUFFER_SIZE (buf));
    guint i;

    if (!msg)
      goto could_not_allocate;
    msg[ 0] = 0x24;
    msg[ 1] = 1 + ((dec->subformat >> 16) & 7);
    msg[ 2] = (guint32) &msg[9];
    msg[ 9] = dec->width;
    msg[10] = dec->height;
    for (i = 0; i < GST_BUFFER_SIZE (buf); i++)
      msg[i + 11] = 4 * GST_BUFFER_DATA (buf)[i];
    res = dec->custom_message (msg, dec->context);
    g_free (msg);
    if (res)
      goto could_not_send_message;
  }

(...)

could_not_allocate:
  dlclose (dec->handle);
  dec->handle = NULL;
  GST_DEBUG_OBJECT (dec, "Could not allocate memory.");
  return FALSE;

Lutz, can you provide a patch against cvs for your latest modifications ?

I can't as I can only use anoncvs that doesn't show the new directory yet.

I downloaded it from anoncvs and it worked great:
gst-plugins-bad/gst/real/

Lutz: Note that cvs up does not create new directories, use cvs up -d

Created attachment 79150 [details] [review]
Remove hardcoded values. Clean up.

(gst_realdec_setcaps) Remove the hardcoded values. Use the ones supplied in the caps instead. 

(gst_realdec_change_state) Removed.

(gst_realdec_get_property) Correctly return the default paths.

I looked for the plugin in gst-plugins-ugly. Isn't that the right location?

New plugins always go to -bad first, regardless of where they end up.

Lutz, a few additional changes that would be nice to see:

_ use proper debug level. Use ERROR/WARNING where it's needed. This will make the message appear on standard output for example.
_ you said you renamed the plugin/element, but there's no mention of that in your patch.
_ as for splitting the audio and video decoder, maybe it would be good to split up the work in more files like:
* realplugin.c : contains the GST_PLUGIN_DEFINE and all plugin-global code
* gstrealvideo.[ch] : contains the RealVideo plugin
* gstrealaudio.[ch] : contains the RealAudio plugin

  You had initially a test which I didn't include, since 1/ we put tests in separate directories (gst-plugins-bad/tests/), 2/ it requires human interaction and 3/ it depends on a network location which might change in the future. For decoders, it's really hard to make unit tests. We already have the gst-media-test which you could try with several files to serve such purposes.

The plugin is currently in gst-plugins-bad/gst/real/ btw.

Anyway, really nice work. Quite comforting to see all these .rm files show up green in the gst-media-test now :)

I prefer incremental patches and will supply the renaming patch once my last one has been applied.

2007-01-02  Lutz Mueller <lutz@topfrose.de>

	reviewed by: Edward Hervey  <edward@fluendo.com>

	* gst/real/gstreal.c:
	(gst_realdec_setcaps): Use codec_data supplied in caps.
	(gst_realdec_get_property): Correctly return default path.
	(gst_realdec_class_init): Remove unused state_change method.

Created attachment 79203 [details] [review]
Rename RealDec to RealVideoDec etc. and use header file.

Created attachment 79204 [details]
header file for real video decoder

I'll submit a patch for moving the video decoder into gstrealvideodec.c after the renaming patch has been applied.

Created attachment 79232 [details]
gstrealaudiodec.h

Created attachment 79233 [details]
gstrealaudiodec.c

Element 'works' insofar that it doesn't crash with my testfile and produces noice.

Created attachment 79314 [details]
gstrealaudiodec.c

Now with even more sound. Frequency seems to match, but the speed is not ok.

Regarding gstrealaudiodec.c :

_ Do not use g_message, use gstreamer debugging system

_ g_strdup(g_value_get_string(x)) ==> g_value_dup_string(x)

_ "REAL audio" ==> "RealAudio" (for names and descriptions)

_ Could you comment on the use of the password property ? And why it's not available as a gobject property.

Created attachment 79403 [details]
gstrealaudiodec.c

g_message: I know. I posted the code in the hope someone with a bit more knowledge on audio formats will look at it. This code has not been intended for inclusion in CVS yet.

g_value_dup_string: Thanks for the hint.

PROP_PASSWORD: I forgot to add it to the switch statement. I've taken the default password from xine. Don't ask me where they've got it from.

Made several modifications to the code to make it work on x86_64 also (the .so have been available for the past month). Commited the separation code too.

Still have to commit the audio part.

2007-01-05  Edward Hervey  <edward@fluendo.com>

	* configure.ac:
	Real video .so are now also available for x86_64, so we can build the
	Real plugin on i386 AND x86_64.
	* gst/real/Makefile.am:
	* gst/real/gstreal.c: (plugin_init):
	New plugin file for real .so wrapper plugins.
	* gst/real/gstrealvideodec.c: (gst_real_video_dec_alloc_buffer),
	(gst_real_video_dec_decode), (gst_real_video_dec_chain),
	(gst_real_video_dec_activate_push), (gst_real_video_dec_setcaps),
	(open_library), (close_library), (gst_real_video_dec_init),
	(gst_real_video_dec_base_init), (gst_real_video_dec_finalize),
	(gst_real_video_dec_set_property),
	(gst_real_video_dec_get_property), (gst_real_video_dec_class_init):
	* gst/real/gstrealvideodec.h:
	Moved RealVideo element to separate file
	Cleaned up code some more.
	Make it work on x86_64.
	Try several possible locations for .so
	Separate opening/closing libraries in separate functions.

Created attachment 79477 [details]
gstrealaudiodec.c

It's easier than initially thought. The descrambling code is already implemented in the demuxer. Now you hear actual music.

Created attachment 79480 [details] [review]
Implement error recovery on setcaps failure

In the audio decoder, if setcaps fails, the state of the decoder is not changed. I guess this is the correct behavior (although perhaps overkill). Attached patch implements this behavior for the video decoder, too. For example, instead of using the variable 'dec->width' in the setcaps function, a local variable 'width' is used. 'dec->width' is only set to the local variable 'width' if setting of the caps succeeds. Same with all hooks.

All patches commited, with some modifications to make it work also on x86_64.

2007-01-06  Edward Hervey  <edward@fluendo.com>

        Patch by: Lutz Mueller <lutz@topfrose.de>

        * gst/real/gstrealvideodec.c: (gst_real_video_dec_decode),
        (gst_real_video_dec_setcaps), (open_library), (close_library),
        (gst_real_video_dec_finalize):
        * gst/real/gstrealvideodec.h:
        Implement error recovery on setcaps failure.

2007-01-06  Edward Hervey  <edward@fluendo.com>

        Patch by: Lutz Mueller <lutz@topfrose.de>

        * gst/real/Makefile.am:
        * gst/real/gstreal.c: (plugin_init):
        * gst/real/gstrealaudiodec.c: (gst_real_audio_dec_chain),
        (gst_real_audio_dec_setcaps), (gst_real_audio_dec_init),
        (gst_real_audio_dec_base_init), (gst_real_audio_dec_change_state),
        (gst_real_audio_dec_finalize), (gst_real_audio_dec_set_property),
        (gst_real_audio_dec_get_property), (gst_real_audio_dec_class_init):
        * gst/real/gstrealaudiodec.h:
        Added RealAudio wrapper elementfactory.
        Modified structures so it can also work on x86_64 using the
        adequate .so .

Lutz, It would be better, now that all patches have been commited and we have a working plugin, to create new bug reports for issues/fixes/patches with these plugins.

I am going to create a new one for the .so path issue (seems to be different depending on distributions).