Created attachment 122991 [details] [review]
Add asynchronous parsing functions to totem-pl-parser

This has been tested a bit with both the totem-pl-parser test suite, and with a hackily-modified Totem, but probably wants a bit more testing before being committed.

Note that it adds new API: totem_pl_parser_parse_async() and totem_pl_parser_parse_with_base_async(), and breaks ABI by extending TotemPlParserClass.

After reflection, I'm not so sure about using a signal (parsing-finished) to show that parsing's finished, rather than a callback function. Using a callback function would allow different userdata to be passed for each different parse operation, whereas with the signal approach, the signal handler has to be disconnected and reconnected for each new operation to change the userdata. Since the userdata will likely contain the MRL being parsed (or some other operation-specific identifier), this is less than optimal.

What about reusing the GAsync* stuff from GIO ?

You might also be interested in GTask, an Asynchronous toolkit library I've been writing for GObject.

Documentation:
  http://docs.dronelabs.com/gtask/
  http://docs.dronelabs.com/gtask/api

Sources:
  http://ftp.dronelabs.com/sources/gtask/0.1/gtask-0.1.2.tar.gz
  git://git.dronelabs.com/git/users/chris/gtask.git

-- Christian

Steve: The GAsync* stuff doesn't seem to add anything useful to the API, and I think I'd only consider using it if the callback were to be a lot more complex (and in need of a custom object to pass it data, a la GAsyncResult).

Christian: Again, that seems a little overkill for this API.

Philip, I'd certainly agree on the public API.  No need to add anything more than perhaps a new callback delegate.

At most, I was thinking it might make sense for use internally and allow to expose a callback delegate unique to totem which would be dispatched from a GTask callback.

Cheers

The code in totem_pl_parser_parse_with_base_async() is racy. It's possible that the thread already finished when we start adding more stuff to the queue. Either we should have a mutex, making it impossible for the thread to exit whilst we're adding more stuff, or you could keep a thread around all the time, and have it block with g_async_queue_timed_pop().

You should also probably lock and unlock the queue as necessary.

There also doesn't seem to be a way to cancel a parsing that was already started, which is a bit of a problem, and collides with the current sync behaviour of all the parsers.

I'm going to rewrite this. After doing more async stuff with GAsyncResult elsewhere, I've decided it's probably a good idea to use that after all. That would also mean we can use GCancellable to allow the operation to be cancelled asynchronously.

Created attachment 136023 [details] [review]
Add asynchronous parsing functions to totem-pl-parser (attempt 2)

This version uses GAsyncResult and GCancellable, and resolves all the issues with the first patch. In-progress parse operations can be cancelled from any thread using a GCancellable, and since each operation has its own thread, there are no race conditions on finishing the work queue.

As well as three new functions for async parsing (totem_pl_parser_parse_async, totem_pl_parser_parse_with_base_async and totem_pl_parser_parse_finish), it adds TOTEM_PL_PARSER_RESULT_CANCELLED, which is returned if a parse operation was cancelled before it finished.

The recurse level will be completely broken if we allow for multiple threads to access it...

Any reason not to move that to being a parameter of totem_pl_parser_parse_internal? It would be cleaner.

It's not going to be as easy to make GAsyncResult stuff use our own thread and job queue.

(In reply to comment #11)
> Any reason not to move that to being a parameter of
> totem_pl_parser_parse_internal? It would be cleaner.

Sure, but that means it would need to be passed down all the functions, so that we can decrease it on the stack when recursing.

> It's not going to be as easy to make GAsyncResult stuff use our own thread and
> job queue.

Don't understand the problem here...

(In reply to comment #12)
> (In reply to comment #11)
> > Any reason not to move that to being a parameter of
> > totem_pl_parser_parse_internal? It would be cleaner.
> 
> Sure, but that means it would need to be passed down all the functions, so that
> we can decrease it on the stack when recursing.

Yes, but that doesn't look too big a job, from a quick grep. Maybe I've missed something…

> > It's not going to be as easy to make GAsyncResult stuff use our own thread and
> > job queue.
> 
> Don't understand the problem here...

Actually, thinking about it some more, the recurse level could get broken even if we only allowed async operations in one thread, since a sync operation could be started right after an async one, and they'd still contend the recursion level.

(In reply to comment #13)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > Any reason not to move that to being a parameter of
> > > totem_pl_parser_parse_internal? It would be cleaner.
> > 
> > Sure, but that means it would need to be passed down all the functions, so that
> > we can decrease it on the stack when recursing.
> 
> Yes, but that doesn't look too big a job, from a quick grep. Maybe I've missed
> something…

It would update pretty much all the functions in totem-pl-parser that do parsing. That's quite a lot.

> > > It's not going to be as easy to make GAsyncResult stuff use our own thread and
> > > job queue.
> > 
> > Don't understand the problem here...
> 
> Actually, thinking about it some more, the recurse level could get broken even
> if we only allowed async operations in one thread, since a sync operation could
> be started right after an async one, and they'd still contend the recursion
> level.

Yes, which is why the recurse level needs to moved out of the TotemPlParser object itself, and be added in "parse" private data.

Created attachment 136772 [details] [review]
Refactor recursive parsing to preserve options throughout the operation

This patch is to be applied on top of the previous one, and refactors the parsing stack to recursively use a TotemPlParseData struct, containing the recurse level, as well as some global-ish options like whether to use fallbacks or whether to recurse at all.

One upside to this approach is that TotemPlParserPrivate is now truly private to totem-pl-parser.c.

The problem is that all those:
+	data.fallback = fallback;
+	data.recurse = parser->priv->recurse;
+	data.force = parser->priv->force;
+	data.disable_unsafe = parser->priv->disable_unsafe;
aren't per-parse options, they're options for the object itself.

The only thing that's worth passing is the recurse_level. Other than that, it looks fine to commit.

2009-06-17  Philip Withnall  <philip@tecnocode.co.uk>

	* docs/reference/totem-pl-parser-sections.txt:
	* plparse/plparser.symbols:
	* plparse/totem-pl-parser-builtins.c
	(totem_pl_parser_result_get_type):
	* plparse/totem-pl-parser-lines.c (totem_pl_parser_add_ram),
	(totem_pl_parser_add_m3u), (totem_pl_parser_add_ra):
	* plparse/totem-pl-parser-lines.h:
	* plparse/totem-pl-parser-media.c (totem_pl_parser_add_iso),
	(totem_pl_parser_add_cue), (totem_pl_parser_add_directory),
	(totem_pl_parser_add_block):
	* plparse/totem-pl-parser-media.h:
	* plparse/totem-pl-parser-misc.c (totem_pl_parser_add_gvp),
	(totem_pl_parser_add_desktop):
	* plparse/totem-pl-parser-misc.h:
	* plparse/totem-pl-parser-pla.c (totem_pl_parser_add_pla):
	* plparse/totem-pl-parser-pla.h:
	* plparse/totem-pl-parser-pls.c
	(totem_pl_parser_add_pls_with_contents), (totem_pl_parser_add_pls):
	* plparse/totem-pl-parser-pls.h:
	* plparse/totem-pl-parser-podcast.c (totem_pl_parser_add_rss),
	(totem_pl_parser_add_itpc), (totem_pl_parser_add_zune),
	(totem_pl_parser_add_atom), (totem_pl_parser_add_xml_feed),
	(totem_pl_parser_add_itms), (totem_pl_parser_add_opml):
	* plparse/totem-pl-parser-podcast.h:
	* plparse/totem-pl-parser-private.h:
	* plparse/totem-pl-parser-qt.c
	(totem_pl_parser_add_quicktime_rtsptext),
	(totem_pl_parser_add_quicktime_metalink),
	(totem_pl_parser_add_quicktime):
	* plparse/totem-pl-parser-qt.h:
	* plparse/totem-pl-parser-smil.c (totem_pl_parser_add_smil):
	* plparse/totem-pl-parser-smil.h:
	* plparse/totem-pl-parser-wm.c
	(totem_pl_parser_add_asf_reference_parser),
	(totem_pl_parser_add_asf_parser), (parse_asx_entry),
	(parse_asx_entryref), (parse_asx_entries),
	(totem_pl_parser_add_asx), (totem_pl_parser_add_asf):
	* plparse/totem-pl-parser-wm.h:
	* plparse/totem-pl-parser-xspf.c (totem_pl_parser_add_xspf):
	* plparse/totem-pl-parser-xspf.h:
	* plparse/totem-pl-parser.c (totem_pl_parser_class_init),
	(emit_playlist_ended_signal), (totem_pl_parser_playlist_end),
	(totem_pl_parser_is_debugging_enabled), (totem_pl_parser_init),
	(totem_pl_parser_finalize), (emit_entry_parsed_signal),
	(totem_pl_parser_add_uri_valist),
	(totem_pl_parser_scheme_is_ignored),
	(totem_pl_parser_mimetype_is_ignored),
	(totem_pl_parser_parse_internal), (parse_async_data_free),
	(parse_thread), (totem_pl_parser_parse_with_base_async),
	(totem_pl_parser_parse_with_base), (totem_pl_parser_parse_async),
	(totem_pl_parser_parse_finish),
	(totem_pl_parser_add_ignored_scheme),
	(totem_pl_parser_add_ignored_mimetype):
	* plparse/totem-pl-parser.h: Add asynchronous playlist parsing
	functions, totem_pl_parser_parse_async, *_parse_with_base_async and
	totem_pl_parser_parse_finish. This includes making TotemPlParser
	threadsafe, and ensuring it emits all its signals in the main thread.
	It also includes refactoring of recursive parsing so that global
	options are copied and preserved throughout the parse operation,
	and can't be changed mid-operation from another thread.
	(Closes: #561444)