Created attachment 27956 [details] [review]
see above

You forgot to put videobalance for software colorbalance control with ximagesink.

to be clear, I don't want gapless song transition at all in gstplay.
As long as gstplay manages the complete "file to output" pipeline, it can't
really be used for gapless transition anyway.

Well basically you brought back the GST_PLAY_TYPE that has been kicked out. The
reason i removed that and used a single pipeline was to be able to just handle
all cases with a single pipeline.

For audio only medias the video part of gstplay is not even connected by spider
so no data flows into it (no overhead). Visualization was added removed on the
fly depending on what users wanted.

Current libgstplay runs in it's own thread and you just moved the g_idle_add
from the player.c file directly inside your gstplay object, i don't really see
how it was not usable in KDE before...

I don't really mind to just replace current libgstplay by a rewrite from scratch
but basically for me that one is just bringing back the GST_PLAY_TYPE that i
found so ugly before. The only discussion i can see about all that stuff is
wether or not applications have to do gst_play_new
(GST_PLAY_MODE_VIDEO_WITH_VISUALIZATION, &error) or just gst_play_new (&error)
and let the pipeline accomodate itself to the media flowing inside.

Maybe I'm not using it correctly, but when I change between .mp3 and .ogg 
files, the player goes bonkers. The stream length will be way too long, and 
audio output is high-pitched squeaking noise. I'm using the Audio-only mode. 
 
Changing between .mp3 and .mp3 is fine. The first .ogg also plays fine, but 
changing to another .ogg then just seems to do nothing. The previous GstPlay 
didn't have any problems with this as far as I could tell. 
 
I've replaced the 'caps-set' thing with a 'notify::caps' wrapper though, so 
maybe that's what's causing this. 
 
http://scentric.net/tmp/gtk-gst-play-test.tar.gz 
 
for a stand-alone version if anyone wants to give it a try. 
 
Cheers 
 -Tim 
 

I don't replace spider on setting a new location yet, will fix that up tonight.

Julien, I re-added the mode (GstPlayType, but different ;) ) for a simple
reason: players are different.

I'm not sure if it's the best solution to the problem, but I basically see it as
follows: there's multiple types of players. Now, I could just let the
application figure out if it wants to set a visualization element, an audio sink
and/or a video sink and select a mode based on that. However, I figured this
would require a lot of checks. The mode allows me to do a clean'n'mean
switch/case with g_return_if_fail()s for sinks in each mode (MODE_AUDIO requires
an audio sink, MODE_VIDEO requires a video sink as well). The result is less and
cleaner code. Like I said, I don't think it's optimal, but it's the best I could
think of.

I'll look at videobalance and volume tonight. I know they're missing as of now.
Reason is that I'm unsure how to handle them best. In some cases, they should
not be added. Either because we're using a hardware audio output that handles
volume itself *and* the application doesn't mind touching system volume instead
of only the stream volume (this is application-dependent!), or because the
element output already provides it (xvimagesink vs. colorbalance/ximagesink). I
might want the application to set such "interface emulation elements" instead of
making them part of libgstplay. Again, I'm unsure here. Feel free to give your
opinions/thoughts.

I think I prefer the small focused pipelines approach that Ronald is going for.

Instead of using GstPlayMode to select the pipeline how about subclassing?  The 
base GstPlay will define the whole interface and will support audio+video+vis.

Subclasses will then be more focused, such as:
GstAudioPlay
GstVideoPlay
GstAudioVisPlay

This way GstPlay remains the fully featured pipeline, and other apps can use 
the subclasses for their media playing. Developers should be happier about 
debugging the simpler subclass pipelines too.

You might not know the type of player you want before creating it.

One kick-ass feature that I'd like to see in gobject is the ability to morph an
object from one type to another.  This is likely to be impressively hard to do
well.  But I can dream.

Will players ever need that? I doubt that...

Created attachment 27986 [details] [review]
updated patch

* use notify::caps
* new autoplugger if location changes

Tim, your example doesn't really work for me, don't know why yet. The player
example works fine though. Some bugs:

* video_info is triggered for audio-only streams (16x16@1fps - default
fixation). I need to find a way around that.
* I'm not triggering stream_length/time_tick if we change location. Not sure if
I should. The old libgstplay did it. I probably should.
* Small stuff.

Steve: subclassing is another option, but the code would hardly change. This is
easier to program for now. I might move over to subclassing later on, but I
don't think it'd make it any easier or more difficult. So for now, it'd just add
bloat... Let's first get stuff right. :).

Created attachment 27996 [details] [review]
update

* fix for if notify::caps is called with NULL as caps (caps-unset)
* fix for wrong relinking in set_location()

This fixes Tim's application. :). Cool app!

With the third revision (from May 25), after ca. 1-2 hours of playing mp3s   
(start to end, then to next song, etc.), the playing stopped mid-song, and I   
got this:   
   
  (gstgtk:24394): GStreamer-WARNING **: Thread (null) is destroying itself.   
Function call will not return!   
   
Trying to start playing another song after that, I got:   
   
  (gstgtk:24394): GStreamer-CRITICAL **: file gstbin.c: line 521   
(gst_bin_remove_func): assertion `GST_ELEMENT_PARENT (element) == (GstObject   
*) bin' failed   
   
So far non-reproducible.   
   
  ---------   
   
And just now I got this: 
   
Playing: /foo/CD3/14.mp3   
EOS   
   
(gstgtk:12202): GLib-GObject-CRITICAL **: file gclosure.c: line 421   
(g_closure_invoke): assertion `closure->marshal || closure->meta_marshal'   
failed   
Playing: /foo/CD3/15.mp3   
Segmentation fault (core dumped)   
   
Hope that makes at least some sense, because the stack traces from the core   
are completed useless, I'm afraid.   
   
Cheers   
 -Tim   
   

Can you try handling the EOS in the main thread?

static gboolean
do_eos (gpointer data)
{
  .. your own eos hanlding ..

  /* once */
  return FALSE;
}

static void
catch_eos (GstPlay * play, gpointer data)
{
  g_idle_add ((GSourcefunc) do_eos, data);
}

That's what I've been doing (with regard to GstPlay at least) ... 
 
  static void  
  eos_cb (GstElement *obj, gpointer baz) 
  { 
    g_print ("EOS\n"); 
    gtk_widget_set_sensitive (hscale, FALSE); // this is wrong, but shouldn't 
affect GstPlay, should it? 
    g_idle_add ((GSourceFunc) next_song, NULL); 
  } 
 
I'm not entirely convinced it is a good idea to emit the signals from the 
thread they happen to come from just to avoid GMainLoop integration.  I don't 
really have a better suggestion though, apart from adding something like 
gst_play_set_main_context() that would make GstPlay wrap those signals in idle 
timeouts itself. It may be ugly, but at least it makes the issue more obvious. 
 
 Cheers 
  -Tim 
 

Hm... I'd suggest running it inside gdb (using --gst-fatal-warnings) and waiting
for the crashes to appear again, and then provide a backtrace. It's the best I
can think of right now...

Another minor issue:  
 
The stream length is not always correct for VBR mp3 files - with one of my 
files it's off by more than a whole minute. This is a bit of a problem for 
apps that have a seekbar. Maybe it should be re-checked regularly until the 
length returned matches the previous one or so? 
 
Cheers 
 -Tim 
 

I know the issue, but it's unrelated to libgstplay (imo). Feel free to file a
separate bug, those sort of issues do annoy me as well. ;).

I've created a sample video player on
http://gstreamer.freedesktop.org/media/incoming/gst-player-0.0.1.tar.gz. It has
many issues, amongst which:

* play-pause-play doesn't work
* ususally fails after a seek
* no visualizations
* I didn't even bother trying sound-only yet

But it works a bit. It's stable and simple enough for simple testing, I will try
to extend libgstplay based on this.

Note to myself: some, if not all, of the issues might be because I don't
g_idle_add() all signal handlers yet (offenders are change-state & error in
window.c change-state in timer.c). Which leads me to a design question: who
should do that? Currently, libgstplay-me does that for time-tick and
stream-length, but not for video/audio_info. GstElement signals such as
change-state etc. aren't put in the main thread either. The problem with using
g_idle_add() is that it requires a GMainLoop, which KDE etc. don't have. So
should I remove all this from libgstplay and let the application care?

Created attachment 28624 [details] [review]
update

gst-player example is still at the same location.

Visualization still needs doing. Other stuff works now.

couple of observations.

* patch doesn't apply cleanly; updated version attached.  One thing was a
compiler warning.
* still uses spider
* with any scheduler, I get some wrong info and a thread error at end of
playback of cooldance.ogg:
got video info 16 x 16 @ 1.000000 fps
         serial: 761002030
         serial: 571475272
        encoder: Xiph.Org libTheora I 20040317 3 2 0
encoder version: 3
got video info 128 x 96 @ 10.000000 fps
        encoder: Xiph.Org libVorbis I 20030909
encoder version: 0
nominal bitrate: 112001
got audio info 2 channels @ 44100 Hz
got length 0:00:00.000000000
EOS
setting pipeline to ready
 
GThread-ERROR **: file gthread-posix.c: line 160 (): error 'Device or resource
busy' during 'pthread_mutex_destroy ((pthread_mutex_t *) mutex)'
aborting...
Aborted


the length is wrong, and the first video information reported is wrong.

With opt, it plays the video jerkily, with basicgthread, it seems ok.

Other files have similar issues.

The API is not compatible, I'm not really sure why you decided to rewrite it. 
But it seems like this is a problem to get it integrated in a 0.8 series.

using your sample player on gravity.mpg, I get this backtrace:

Program received signal SIGSEGV, Segmentation fault.

+ Trace 47329

I seem to be getting this for every type of file I try.

As it stands, I can't really test whether this makes for a better playback
engine.  Let me know what I can try next.

Created attachment 29287 [details] [review]
updated patch

works against HEAD

I also noticed that you removed interfaces.  Does that mean the player can't
change the volume or colorbalance anymore, or did you provide some other
workaround for that ?

> No interfaces
Interfaces was on my TODO list. The application (so the sample player) is
supposed to include volume in the audio output pipeline if it wants it there.
Colorbalance is currently not planned to be added because it is a CPU hog. It
needs to be optimized first. I've done some patching on it some time ago and got
it down from 80% CPU use to 30% CPU use, but that's still too much. This means,
indeed, that for ximagesink as output, we will currently not allow for video
adjustment. I'll work on this later, but it's low priority. I'm not a good
assembler coder.

> Wrong video information
The first video information is indeed wrong. I don't consider that a blocker
issue right now because it'll set it right in a split second. I don't really
know how to fix it. It is a bug, though.

> Typefind crash
I don't know why your typefinding fails. I think the current gst-plugins gstplay
uses source ! typefind ! spider. I am currently using source ! spider, for no
good reason. Maybe changing that fixes it. I don't see any obvious reason why
this would be related to the patch itself...

> API incompatibility
The API is incompatible because this started as a playaround thing. I only made
it a play lib later on. I've promised somewhere i one of my comments to make it
compatible to the current libgstplay API sometime soon.

> jerky playback
opt/spider issue, as we discussed. I'd like to make the player use basicgthread
by default. Yes, this is a workaround.

> wrong length
I recently noticed that some plugins provide a wrong length before providing the
correct length. I didn't fix the patch for this yet. Maybe I shouldn't; maybe I
should just fix the plugin. Let me know which you prefer.

I'll look at the player example tonight, maybe there's a typo somewhere.

Now that wtay just produced a spider replacement i think we have all the needed
tools to build the optimal pipeline. I mean that we can know if the media has
audio and/or video and then assemble the best pipeline depending on the media
and user's settings (visualization for example). That makes setting the pipeline
type obsolete which was one of the major points of your rewrite.

It seems thomasvs is kind of locked doing some changes in libgstplay so that it
uses decodebin because of that rewrite you started.

Can we please solve that situation ? I really think it would be a mistake to
break  libgstplay's API, the only changes that libgstplay needs are in the
pipeline setup and that's something we can fix quite fast with decodebin.

If everybody agrees on that we can close that bug, kick spider out of libgstplay
and have a working gst-totem competing with mplayer and xine.

Cheers,

OK, I'm currently playing with playbin directly. All work is already in Totem
CVS. This is not needed right now.