GNOME Bugzilla – Bug 698013
openal: port to 1.0
Last modified: 2013-05-15 17:39:50 UTC
Created attachment 241515 [details] [review] gstopenal plugin port to 1.0 and love, disable from NONPORTED. Hello, i just improved gst-plugins-bad/ext/openal to work on gstreamer-1.0. I don't know your criteria but maybe i made it good enough to even move from bad, your call. I'm interested in openal, opengl, ladspa (and a thing i'm going to call lvdspa for image filters) plugins. Perhaps more patches will be coming from me. The patch is gstreamer indented, modified based on pulse (good) and alsa (base) plugins structure. To what i can say it works, though some pipelines might go wrong on openalsrc cause OpenAL capture api is "try if it works" by design. Patch created via: $ git clone git://anongit.freedesktop.org/git/gstreamer/gst-plugins-bad $ git branch $ touch file $ git add file $ git commit -m "test commit" $ git format-patch origin
Hrm, this patch is a lot noisier than it needs to be because unfortunately you have changed things that didn't really need changing at all, like variable names which then necessitate changes in code that wouldn't have been touched otherwise (ctx -> context, sink -> openalsink etc.). And why did you revert the address of the FSF to the old/incorrect one?
Created attachment 241531 [details] [review] better src getcaps It just poped in my mind about no device context before getting caps.
Created attachment 241532 [details] [review] fix hopefully to the correct FSF address on license comments I picked that one i think from pulse or alsa, bad luck.
About the noise, yes is a lot of changes. But that's cause it was more messy than i could want, so you should pick that as more improvement than anything else. The variable beautification is just consistency on gst_more_verbose_functions(gpointer) and not gstmrvrbsfunc(gp). By the way, this way looks more like alsa (base) and pulse (good), after all this is in bad.
It only makes reviewing much more work. Could you attach a patch that just does the porting in one patch, and these style changes (and nothing else) in another?
Sorry, but this will make me (one single person) work the double for you (team) to work the half, i would prefer to keep it 1:1. This was a major rewrite and not just a 2 lines + aesthethics, *really*. *But i note the feedback and from now on, fixes and aesthethics will go on separate patches* hoping to be both parts 0.5:0.5 ;). (This was one of my first major patches to upstream projects). I hope the old = pushContext(ctx) -> old = pushContet(context); lines will be a pass through to review, then the review is easy to see. Also, you could apply the patches on a branch just to look at the result and see it directly for a glance, you'd see what happened at a higher level too, it is, how the final result looks compared to the start. It took me two days, and not a lot of openal code is changed so if you are proficient with gstreamer, i assume more than me even, you can review it with more proficiency. Though i not assume quickly, it is, it could take a full day review. I would like to move on and start working on the ladspa port as soon as possible. This is even harder, cause it was not even fully working on 0.10, unless i miss some pipelines quirks, but according to the web seems i'm not. Thanks.
In case it was not too much clear, ***i didn't apply any personal styles or design preferences***, i would think that would make a project very inconsistent and would be *very reasonably* rejected, and really i have consistency in mind, cause i really based all my stuff on, as i said repeteadly, *alsa and pulse sink and src plugins*. Seeing your indenting rules and some advice on your developers faq, you seem to care about *consistency*. From what i can say, that was what the plugin was not achieving before, that's what i tried to achieve. Thanks.
Created attachment 241579 [details] [review] fix choppy (jumping/skipping frames) playback on playbins I just discovered some choppy (jumping/skipping frames) playback with the openalsink, which lead to this patch. It all returns to be smooth. It introduced a performance issue though cause rest_us type conversion, check next patch together.
Created attachment 241581 [details] [review] Fix performance issue caused by type conversion. Hoping for it to be done.
Created attachment 241586 [details] [review] complete openal improved port to 1.0 Since the comes and goes of license lines, performance regressions, etc, this is the final *FULL* patch for helping the review, else use the five old ones.
Created attachment 241589 [details] [review] complete openal improved port to gstreamer 1.0 Sorry for the inconvenience of reuploading, i was reviewing it myself here in bugzilla, the license stuff, 3 addresses? was driving me nuts, i think i keeped the previous one, turns out i didn't, so i just got it directly what it was, no neighbours plugins comments. Keep it at least on a one day freeze before reviewing, ;).
Created attachment 241607 [details] [review] complete openal improved por to gstreamer 1.0 Wow, i just turned openalsrc into something really more stable, no more segfaults on unexpected caps. :) This with the stability i gave to openalsink mades what i think is a great boost. Still, always with this same test pipeline: GST_PLUGIN_PATH="." gst-launch-1.0 -e openalsrc ! audioconvert ! vorbisenc ! oggmux ! filesink location=test.ogg gives sometimes: Dropped 28224 samples. This is most likely because downstream can't keep up and is consuming samples too slowly. This is not a regression, it was also given before. From what i can say this is the only problem, and if i understand downstream is audioconvert or vorbisenc. Though perhaps the src sends too fast? Of course, i hope you catch some gstreamer glitches i might have leaved in. Thanks. Keep it at least on a one day freeze before reviewing, ;).
This last issue *seems to be fixed* permanently with a higher buffer-time (*10). I don't know if it is cause my netbook has not enough power or it is a machine independent issue so to set on the src code default itself. GST_PLUGIN_PATH="." gst-launch-1.0 -e openalsrc buffer-time=1000000 ! audioconvert ! vorbisenc ! oggmux ! filesink location=test.ogg I can tell wavenc does not need it, so i guess it is a run time issue. ?. P.D.: I'll talk more to me privately instead of keeping this bug as my blog ;P. Thanks.
Created attachment 241614 [details] [review] complete openal improved port to gstreamer 1.0 *_delay
Created attachment 241701 [details] [review] complete openal improved port to gstreamer 1.0 A bunch of format check, adds double, cleans static caps, comments mulaw, adds commented alaw, ima_adpcm. All more stable, optimized and cleaned. I keep using it, but i think that after testing it, i'm not going to submit anything else until you finally review that. Then perhaps if i find something new i could add a non-obsoleting patch or keeping it in my computer. Thanks. Let's see ladspa, :)
> i'm not going to submit anything else until you > finally review that. Then perhaps if i find something > new i could add a non-obsoleting patch or keeping it in my computer. "Finally"? It's not clear when anyone will get around to reviewing it. Maybe later today, maybe next month, maybe even later than that. You haven't exactly made it easy for us. I would suggest you keep updating the patch until someone starts changing the bug status to ASSIGNED to indicate that they're going to work on reviewing it. Alternatively, you could add incremental patches on top of the first patch. Thanks!
Pushed (without proper review; and with include fixes for git master and reverting some of the 'cleanups'): commit 30d7908df294a9526e090c7e0270093a6740cfb5 Author: Juan Manuel Borges Caño <juanmabcmail@gmail.com> Date: Wed Apr 17 02:18:58 2013 +0200 openal: improved port to 1.0 https://bugzilla.gnome.org/show_bug.cgi?id=698013 Thanks for the patch.
This commit causes a build error for me ('AL_FORMAT_MONO_ALAW_EXT' undeclared). See bug #700402.