Dynamic dsp code review

Topic: Dynamic dsp code review (Read 3735 times) previous topic - next topic

0 Members and 1 Guest are viewing this topic.

Dynamic dsp code review

2011-02-10 20:56:16

the zipped project folder:
https://docs.google.com/leaf?id=0B8pD3Wjtvr...uthkey=CJrM98AP

If you could comment on the code that would be great. I felt extreme pain figuring out this much and I'm not sure I've done the best things. The sdk could sure use a ton more documentation with sample code snippets.

(note that the project references boost, you'll have to update the extra include directory on it to point to yours. (same thing of course for wtl)

Some side questions:
-In order to build this (based off the foo_sample component), I had to turn off precompiled headers. Why is that?
-Also, I was not able to build in release mode for some reason. It says something about unresolved external link... one was
"Error 3 error LNK2001: unresolved external symbol __imp__GetInfiniteWaitEvent@0 dsp.obj DynamicDSP"
another:
"Error 4 error LNK2001: unresolved external symbol __imp__uBugCheck@0 foobar2000_component_client.lib DynamicDSP"

What's the problem there?

Dynamic dsp code review

Reply #1 – 2011-02-10 22:26:03

The PFC stringcvt classes should help with wide <> UTF-8 conversions.

Dynamic dsp code review

Reply #2 – 2011-02-10 22:29:51

Considering your linker problems: Have you added shared.lib to the linker input for your project?

Note that your DLL name should start with "foo_" or foobar2000 will not even attempt to load it as a component.

Some remarks about the code:
1) The SDK contains utilities to convert between wide and UTF-8 strings. Look for pfc::stringcvt. Foobar2000 uses UTF-8 internally. Your helper functions depend on the current locale which is usually not UTF-8. There is also string_utf8_from_window which comes in handy for EDIT controls.

2) Titleformatting is quite easier than you think. The metadb_handle::format_title method does any required locking by itself and also takes care of providing information about the track. So instead of this:

Code: [Select]

            curTrack->metadb_lock();
            file_info_impl info;
            curTrack->get_info(info);
            pfc::string8 newDsp;
            curTrack->format_title(&titleformat_hook_impl_file_info(curTrack->get_location(), &info), newDsp, compiledTitleformat, 0);
            curTrack->metadb_unlock();

you can simple do that:

Code: [Select]

            pfc::string8 newDsp;
            curTrack->format_title(NULL, newDsp, compiledTitleformat, NULL);

3) This looks strange to me:

Code: [Select]

currentChain=chainsMap->operator [](newDsp);

I'd just use:

Code: [Select]

currentChain = chainsMap[newDsp];

Of course that requires changing the definition of chainsMap to

Code: [Select]

pfc::map_t<pfc::string8, dsp_chain_config_impl *, pfc::string::comparatorCaseInsensitive> chainsMap;

It will be automatically created and destroyed in the constructor/destructor of the dsp_sample class. If you use an appropriate smart pointer class to wrap your dsp_chain_config_impl instances, you do not event need to explicitly free all the entries in the dsp_sample destructor.

4) You allocate a dsp_chunk_list_impl using operator new, but deallocate if with free() so the destructor will not run. Use operator delete instead. Or even better: Use a local variable or a smart pointer which will automatically free your list when the variable goes out of scope.

5) Your g_get_default_preset method leaks memory. It never deletes its chainsMap.

6) You are leaking a dsp_manager instance. You create one via operator new in the constructors of dsp_sample, but never delete it as far as I can see. Better to declare it as:

Code: [Select]

dsp_manager manager;

7) You use newTrack as a flag. Since C++ has a bool type, I recommend using that to make the code easier to read.

Dynamic dsp code review

Reply #3 – 2011-02-11 15:39:46

wow, thanks!
I think I've got everything fixed up, and I uploaded a new version.
My release configuration was indeed missing shared.lib... but the debug one had it. So now I can build release.

One problem I just noticed is that if my current Chain has latency, I was losing the end of the track, about equal to the latency. I've added code to on_endoftrack like this:

Code: [Select]

void on_endoftrack(abort_callback & cb) {
		if(currentChain!=NULL){
			audio_chunk * chunk=insert_chunk();
			chunk->set_channels(1);
			chunk->set_srate(11025);
			chunk->set_silence(curLatency*chunk->get_srate());
			
			dsp_chunk_list_impl list;
			list.add_chunk(chunk);
			curLatency=manager.run(&list, curTrack, 0, cb);
			
			if(list.get_count()>0){
				*chunk=*(list.get_item(0));
				for(int i=1; i<list.get_count(); i++){
					audio_chunk * curChunk=list.get_item(i);
					audio_chunk * newChunk=insert_chunk();
					*newChunk = *curChunk;
				}
			}
		}
		newTrack=true;
		// Should do nothing except for special cases where your DSP performs special operations when changing tracks.
		// If this function does anything, you must change need_track_change_mark() to return true.
		// If you have pending audio data that you wish to output, you can use insert_chunk() to do so.		
	}

This seems to coax the rest of the audio out of my chain, but it's a bit weird. Is there a better/correct way to do this? What I'm basically doing is processing a silent audio chunk, with a length equal to my latency...

(Edit:while that works to get the audio, it makes the time left label behave funny at the end. ie, it will be saying 4:33 / 4:35, then 4:34 / 4:35, then back to 4:33, and so on. It just doesn't seem to know what to show)

Dynamic dsp code review

Reply #4 – 2011-02-11 20:32:33

In that code block above, if I add manager.flush(); right after manager.run(...) then the results are almost perfect. Not sample perfect though. I tested against a looping brown noise sample, and when my dsp doesn't apply to a file the sample loops seamlessly. When it does apply the sample loops with the tiniest dropout.

If I there is a way to fix that it'd be great.

(edit: actually it does quite a dropout if I use soundtouch in my chain.... my other test which worked better was with vlevel. Clearly I've got to go back to the drawing board)

Dynamic dsp code review

Reply #5 – 2011-02-11 21:10:31

You can no longer garuantee seamless playback, if you request track change notifications. See the SDK documentation for dsp::need_track_change_mark().

Notice