[vlc-devel] [Patches] Dash: mostly cosmetics

[Tobias Güntner]

Am 07.12.2011 13:30, schrieb Hugo Beauzée-Luyssen:
> On Wed, Dec 7, 2011 at 12:39 PM, Tobias Güntner<fatbull at web.de>  wrote:
>> Am 06.12.2011 13:43, schrieb Hugo Beauzée-Luyssen:
>>
>>> Most of the error handling could be done once and for all after the
>>> parsing occurred.
>>
>>
>> Hmmm.
>>
>> Is the parsing really done if supposedly simple getters can still fail
>> because of errors in an input file? Aren't Attribute- and
>> ElementNotPresentException actually (tree) parsing errors? Why do most
>> getters return strings, although their names suggest otherwise? Why do
>> getters returning numbers have to call atoi first, i.e. parse their own
>> value? Shouldn't BasicCMParser take care of all these issues *before*
>> creating MPD objects?
>>
>
> I was thinking about some missing elements. Some attributes are
> mandatory and should indeed be parsed in the ctor, so the getter
> doesn't have to run atoi.
> Some getters return strings but shouldn't. Some patches I sent fix the
> problem for a few of them, some other need to be fixed.
> And indeed, the parser should run some input sanity check before
> trying to use the elements themselves.

That's not what I meant. Let me try again.

I suggest parsing that stuff in a real parser, not in constructors. And 
by "parsing" I mean the transformation of an XML DOM tree into actual 
MPD objects; not the transformation of a text blob into an XML DOM tree.

Parsing is more or less "on demand" right now, i.e., whenever a getter 
is called, it looks in a map whether an attribute was present in the 
input file and parses it, or throws an exception if there was a problem. 
This means every user of the MPD classes has to deal with parsing 
errors. Wouldn't it be a lot easier to handle this in one place?

* Let's remove all parsing logic from MPD classes, specifically the 
SomethingNotPresentExceptions and the attribute maps. Instead, create 
setters for individual attributes or have the constructors initialize 
them. Either way, attribute values are always valid and have the correct 
type. Only (trivial) getters and setters and perhaps a few special 
purpose methods remain. No exceptions are thrown.

* Have BasicCMParser read and parse all attributes. If attributes are 
missing, the parser can throw an AttributeNotPresentException. If some 
attributes are optional, the parser can instead set a reasonable default 
value or the class can set it in its ctor.

* Descend recursively to parse all XML nodes. If all child nodes and 
attributes could be parsed and are valid, create the appropriate MPD 
object and return it. If some elements are mandatory or may not be 
empty, this is the place to handle that, e.g., by throwing an 
ElementNotPresentException.

Pseudo code:

class BasicCMParser {
	string attribute(Node* parent, string name) {
		return attribute value or throw AttributeNotPresentException;
	}
	string attribute(Node* parent, string name, string default) {
		return attribute value or return default;
	}
	int intAttribute(Node* parent, string name, int default, int min, int 
max) {
		return attribute value or return default or throw 
InvalidAttributeValueException;
	}
	// ditto for other attribute types (int, date, enum, etc.)

	MPD* parseMPD(Node* root) {
		// parse attributes
		minBufferTime = intAttribute(root, "minBufferTime", 0, 0, MAX_TIME);
		// parse child elements
		vector<Period> periods;
		for(...) periods.add(parsePeriod(child));
		// input is valid, create the object
		return new MPD(...);
	}
	Period* parsePeriod(Node* node) {
		...
		parseGroup(...);
		...
	}
	...
	Representation* parseRepresentation(Node* node, SegmentInfoDefault* 
default) {
		...
		if(segment info is present)
			parseSegmentInfo(...);
		else
			use SegmentInfoDefault;
		...
	}
}

When the parser returns, users (like BasicCMManager) can call getters 
without having to worry about missing attributes. Also, callers don't 
have to worry about optional elements, either. For example, a 
Representation object can always store a SegmentInfo if the parser 
ensures that it is properly filled (i.e. from a SegmentInfo or 
SegmentInfoDefault element (or whatever the standard says; I'm not that 
familiar with it)).

Regards,
Tobias

PS:
The call to strstr in DOMParser::isDash looks suspicious. stream_Peek 
doesn't return a NUL terminated string, does it?