add parsing of picture type from spec; add check that type 1 picture is 32x32 PNG

include/share/grabbag/picture.h

@@ -27,7 +27,16 @@
 extern "C" {
 #endif
 
-/* spec and error_message must not be NULL */
+/* spec should be of the form "[TYPE]|MIME_TYPE|[DESCRIPTION]|[WIDTHxHEIGHTxDEPTH[/COLORS]]|FILE", e.g.
+ *   "|image/jpeg|||cover.jpg"
+ *   "4|image/jpeg||300x300x24|backcover.jpg"
+ *   "|image/png|description|300x300x24/71|cover.png"
+ *   "-->|image/gif||300x300x24/71|http://blah.blah.blah/cover.gif"
+ *
+ * empty type means default to FLAC__STREAM_METADATA_PICTURE_TYPE_FRONT_COVER
+ * empty resolution spec means to get from the file (cannot get used with "-->" linked images)
+ * spec and error_message must not be NULL
+ */
 FLAC__StreamMetadata *grabbag__picture_parse_specification(const char *spec, const char **error_message);
 
 #ifdef __cplusplus

src/share/grabbag/picture.c

@@ -37,6 +37,31 @@ static char *local__strndup_(const char *s, size_t size)
 	return x;
 }
 
+static FLAC__bool local__parse_type_(const char *s, size_t len, FLAC__StreamMetadata_Picture *picture)
+{
+	size_t i;
+	FLAC__uint32 val = 0;
+
+	picture->type = FLAC__STREAM_METADATA_PICTURE_TYPE_FRONT_COVER;;
+
+	if(len == 0)
+		return true; /* empty string implies default to 'front cover' */
+
+	for(i = 0; i < len; i++) {
+		if(s[i] >= '0' && s[i] <= '9')
+			val = 10*val + (FLAC__uint32)(s[i] - '0');
+		else
+			return false;
+	}
+
+	if(i == len)
+		picture->type = val;
+	else
+		return false;
+
+	return true;
+}
+
 static FLAC__bool local__parse_resolution_(const char *s, size_t len, FLAC__StreamMetadata_Picture *picture)
 {
 	int state = 0;
@@ -236,7 +261,9 @@ FLAC__StreamMetadata *grabbag__picture_parse_specification(const char *spec, con
 		"unable to extract resolution and color info from file, user must set explicitly",
 		"error opening picture file",
 		"error reading picture file",
-		"invalid MIME type"
+		"invalid picture type",
+		"invalid MIME type",
+		"type 1 icon must be a 32x32 pixel PNG"
 	};
 
 	FLAC__ASSERT(0 != spec);
@@ -256,21 +283,25 @@ FLAC__StreamMetadata *grabbag__picture_parse_specification(const char *spec, con
 	for(p = spec; *error_message==0 && *p; ) {
 		if(*p == '|') {
 			switch(state) {
-				case 0: /* mime type */
-					if(p-spec == 0)
+				case 0: /* type */
+					if(!local__parse_type_(spec, p-spec, &obj->data.picture))
 						*error_message = error_messages[7];
+					break;
+				case 1: /* mime type */
+					if(p-spec == 0)
+						*error_message = error_messages[8];
 					else if(0 == (q = local__strndup_(spec, p-spec)))
 						*error_message = error_messages[0];
 					else if(!FLAC__metadata_object_picture_set_mime_type(obj, q, /*copy=*/false))
 						*error_message = error_messages[0];
 					break;
-				case 1: /* description */
+				case 2: /* description */
 					if(0 == (q = local__strndup_(spec, p-spec)))
 						*error_message = error_messages[0];
 					else if(!FLAC__metadata_object_picture_set_description(obj, (FLAC__byte*)q, /*copy=*/false))
 						*error_message = error_messages[0];
 					break;
-				case 2: /* resolution/color (e.g. [300x300x16[/1234]] */
+				case 3: /* resolution/color (e.g. [300x300x16[/1234]] */
 					if(!local__parse_resolution_(spec, p-spec, &obj->data.picture))
 						*error_message = error_messages[2];
 					break;
@@ -287,7 +318,7 @@ FLAC__StreamMetadata *grabbag__picture_parse_specification(const char *spec, con
 	}
 	/* parse filename, read file, try to extract resolution/color info if needed */
 	if(*error_message == 0) {
-		if(state != 3)
+		if(state != 4)
 			*error_message = error_messages[1];
 		else { /* 'spec' points to filename/URL */
 			if(0 == strcmp(obj->data.picture.mime_type, "-->")) { /* magic MIME type means URL */
@@ -328,6 +359,18 @@ FLAC__StreamMetadata *grabbag__picture_parse_specification(const char *spec, con
 		}
 	}
 
+	if(*error_message == 0) {
+		if(
+			obj->data.picture.type == FLAC__STREAM_METADATA_PICTURE_TYPE_FILE_ICON_STANDARD && 
+			(
+				(strcmp(obj->data.picture.mime_type, "image/png") && strcmp(obj->data.picture.mime_type, "-->")) ||
+				obj->data.picture.width != 32 ||
+				obj->data.picture.height != 32
+			)
+		)
+			*error_message = error_messages[9];
+	}
+
 	if(*error_message && obj) {
 		FLAC__metadata_object_delete(obj);
 		obj = 0;

src/test_grabbag/picture/main.c

@@ -32,23 +32,24 @@ typedef struct {
 	FLAC__uint32 height;
 	FLAC__uint32 depth;
 	FLAC__uint32 colors;
+	FLAC__StreamMetadata_Picture_Type type;
 } PictureFile;
 
 PictureFile picturefiles[] = {
-	{ "0.gif", "image/gif" , "", 24, 24, 24, 2 },
-	{ "1.gif", "image/gif" , "", 12,  8, 24, 256 },
-	{ "2.gif", "image/gif" , "", 16, 14, 24, 128 },
-	{ "0.jpg", "image/jpeg", "", 30, 20,  8, 0 },
-	{ "4.jpg", "image/jpeg", "", 31, 47, 24, 0 },
-	{ "0.png", "image/png" , "", 30, 20,  8, 0 },
-	{ "1.png", "image/png" , "", 30, 20,  8, 0 },
-	{ "2.png", "image/png" , "", 30, 20, 24, 7 },
-	{ "3.png", "image/png" , "", 30, 20, 24, 7 },
-	{ "4.png", "image/png" , "", 31, 47, 24, 0 },
-	{ "5.png", "image/png" , "", 31, 47, 24, 0 },
-	{ "6.png", "image/png" , "", 31, 47, 24, 23 },
-	{ "7.png", "image/png" , "", 31, 47, 24, 23 },
-	{ "8.png", "image/png" , "", 32, 32, 32, 0 }
+	{ "0.gif", "image/gif" , "", 24, 24, 24, 2, FLAC__STREAM_METADATA_PICTURE_TYPE_FRONT_COVER },
+	{ "1.gif", "image/gif" , "", 12,  8, 24, 256, FLAC__STREAM_METADATA_PICTURE_TYPE_BACK_COVER },
+	{ "2.gif", "image/gif" , "", 16, 14, 24, 128, FLAC__STREAM_METADATA_PICTURE_TYPE_OTHER },
+	{ "0.jpg", "image/jpeg", "", 30, 20,  8, 0, FLAC__STREAM_METADATA_PICTURE_TYPE_FRONT_COVER },
+	{ "4.jpg", "image/jpeg", "", 31, 47, 24, 0, FLAC__STREAM_METADATA_PICTURE_TYPE_FRONT_COVER },
+	{ "0.png", "image/png" , "", 30, 20,  8, 0, FLAC__STREAM_METADATA_PICTURE_TYPE_FRONT_COVER },
+	{ "1.png", "image/png" , "", 30, 20,  8, 0, FLAC__STREAM_METADATA_PICTURE_TYPE_FRONT_COVER },
+	{ "2.png", "image/png" , "", 30, 20, 24, 7, FLAC__STREAM_METADATA_PICTURE_TYPE_FRONT_COVER },
+	{ "3.png", "image/png" , "", 30, 20, 24, 7, FLAC__STREAM_METADATA_PICTURE_TYPE_FRONT_COVER },
+	{ "4.png", "image/png" , "", 31, 47, 24, 0, FLAC__STREAM_METADATA_PICTURE_TYPE_FRONT_COVER },
+	{ "5.png", "image/png" , "", 31, 47, 24, 0, FLAC__STREAM_METADATA_PICTURE_TYPE_FRONT_COVER },
+	{ "6.png", "image/png" , "", 31, 47, 24, 23, FLAC__STREAM_METADATA_PICTURE_TYPE_FRONT_COVER },
+	{ "7.png", "image/png" , "", 31, 47, 24, 23, FLAC__STREAM_METADATA_PICTURE_TYPE_FRONT_COVER },
+	{ "8.png", "image/png" , "", 32, 32, 32, 0, 999 }
 };
 
 static FLAC__bool debug_ = false;
@@ -68,13 +69,16 @@ static FLAC__bool test_one_picture(const char *prefix, const PictureFile *pf, co
 	FLAC__StreamMetadata *obj;
 	const char *error;
 	char s[4096];
-	snprintf(s, sizeof(s)-1, "%s|%s|%s|%s/%s", pf->mime_type, pf->description, res, prefix, pf->path);
+	snprintf(s, sizeof(s)-1, "%u|%s|%s|%s|%s/%s", (unsigned)pf->type, pf->mime_type, pf->description, res, prefix, pf->path);
 
 	printf("testing grabbag__picture_parse_specification(\"%s\")... ", s);
 	if(0 == (obj = grabbag__picture_parse_specification(s, &error)))
 		return failed_(error);
 	if(debug_) {
-		printf("\nmime_type=%s\ndescription=%s\nwidth=%u\nheight=%u\ndepth=%u\ncolors=%u\ndata_length=%u\n",
+		printf("\ntype=%u (%s)\nmime_type=%s\ndescription=%s\nwidth=%u\nheight=%u\ndepth=%u\ncolors=%u\ndata_length=%u\n",
+			obj->data.picture.type,
+			obj->data.picture.type < FLAC__STREAM_METADATA_PICTURE_TYPE_UNDEFINED?
+				FLAC__StreamMetadata_Picture_TypeString[obj->data.picture.type] : "UNDEFINED",
 			obj->data.picture.mime_type,
 			obj->data.picture.description,
 			obj->data.picture.width,
@@ -84,6 +88,8 @@ static FLAC__bool test_one_picture(const char *prefix, const PictureFile *pf, co
 			obj->data.picture.data_length
 		);
 	}
+	if(obj->data.picture.type != pf->type)
+		return failed_("picture type mismatch");
 	if(strcmp(obj->data.picture.mime_type, pf->mime_type))
 		return failed_("picture MIME type mismatch");
 	if(strcmp((const char *)obj->data.picture.description, (const char *)pf->description))
@@ -116,49 +122,55 @@ static FLAC__bool do_picture(const char *prefix)
 	printf("OK (failed as expected, error: %s)\n", error);
 
 	/* invalid spec: no filename */
-	printf("testing grabbag__picture_parse_specification(\"|||\")... ");
-	if(0 != (obj = grabbag__picture_parse_specification("|||", &error)))
+	printf("testing grabbag__picture_parse_specification(\"||||\")... ");
+	if(0 != (obj = grabbag__picture_parse_specification("||||", &error)))
 		return failed_("expected error, got object");
 	printf("OK (failed as expected: %s)\n", error);
 
 	/* invalid spec: no filename */
-	printf("testing grabbag__picture_parse_specification(\"image/gif|||\")... ");
-	if(0 != (obj = grabbag__picture_parse_specification("image/gif|||", &error)))
+	printf("testing grabbag__picture_parse_specification(\"|image/gif|||\")... ");
+	if(0 != (obj = grabbag__picture_parse_specification("|image/gif|||", &error)))
 		return failed_("expected error, got object");
 	printf("OK (failed as expected: %s)\n", error);
 
 	/* invalid spec: bad resolution */
-	printf("testing grabbag__picture_parse_specification(\"image/gif|desc|320|0.gif\")... ");
-	if(0 != (obj = grabbag__picture_parse_specification("image/gif|desc|320|0.gif", &error)))
+	printf("testing grabbag__picture_parse_specification(\"|image/gif|desc|320|0.gif\")... ");
+	if(0 != (obj = grabbag__picture_parse_specification("|image/gif|desc|320|0.gif", &error)))
 		return failed_("expected error, got object");
 	printf("OK (failed as expected: %s)\n", error);
 
 	/* invalid spec: bad resolution */
-	printf("testing grabbag__picture_parse_specification(\"image/gif|desc|320x240|0.gif\")... ");
-	if(0 != (obj = grabbag__picture_parse_specification("image/gif|desc|320x240|0.gif", &error)))
+	printf("testing grabbag__picture_parse_specification(\"|image/gif|desc|320x240|0.gif\")... ");
+	if(0 != (obj = grabbag__picture_parse_specification("|image/gif|desc|320x240|0.gif", &error)))
 		return failed_("expected error, got object");
 	printf("OK (failed as expected: %s)\n", error);
 
 	/* invalid spec: no filename */
-	printf("testing grabbag__picture_parse_specification(\"image/gif|desc|320x240x9|\")... ");
-	if(0 != (obj = grabbag__picture_parse_specification("image/gif|desc|320x240x9|", &error)))
+	printf("testing grabbag__picture_parse_specification(\"|image/gif|desc|320x240x9|\")... ");
+	if(0 != (obj = grabbag__picture_parse_specification("|image/gif|desc|320x240x9|", &error)))
 		return failed_("expected error, got object");
 	printf("OK (failed as expected: %s)\n", error);
 
 	/* invalid spec: #colors exceeds color depth */
-	printf("testing grabbag__picture_parse_specification(\"image/gif|desc|320x240x9/2345|0.gif\")... ");
-	if(0 != (obj = grabbag__picture_parse_specification("image/gif|desc|320x240x9/2345|0.gif", &error)))
+	printf("testing grabbag__picture_parse_specification(\"|image/gif|desc|320x240x9/2345|0.gif\")... ");
+	if(0 != (obj = grabbag__picture_parse_specification("|image/gif|desc|320x240x9/2345|0.gif", &error)))
+		return failed_("expected error, got object");
+	printf("OK (failed as expected: %s)\n", error);
+
+	/* invalid spec: standard icon has to be 32x32 PNG */
+	printf("testing grabbag__picture_parse_specification(\"1|-->|desc|32x24x9|0.gif\")... ");
+	if(0 != (obj = grabbag__picture_parse_specification("1|-->|desc|32x24x9|0.gif", &error)))
 		return failed_("expected error, got object");
 	printf("OK (failed as expected: %s)\n", error);
 
 	/* invalid spec: need resolution for linked URL */
-	printf("testing grabbag__picture_parse_specification(\"-->|desc||http://blah.blah.blah/z.gif\")... ");
-	if(0 != (obj = grabbag__picture_parse_specification("-->|desc||http://blah.blah.blah/z.gif", &error)))
+	printf("testing grabbag__picture_parse_specification(\"|-->|desc||http://blah.blah.blah/z.gif\")... ");
+	if(0 != (obj = grabbag__picture_parse_specification("|-->|desc||http://blah.blah.blah/z.gif", &error)))
 		return failed_("expected error, got object");
 	printf("OK (failed as expected: %s)\n", error);
 
-	printf("testing grabbag__picture_parse_specification(\"-->|desc|320x240x9|http://blah.blah.blah/z.gif\")... ");
-	if(0 == (obj = grabbag__picture_parse_specification("-->|desc|320x240x9|http://blah.blah.blah/z.gif", &error)))
+	printf("testing grabbag__picture_parse_specification(\"|-->|desc|320x240x9|http://blah.blah.blah/z.gif\")... ");
+	if(0 == (obj = grabbag__picture_parse_specification("|-->|desc|320x240x9|http://blah.blah.blah/z.gif", &error)))
 		return failed_(error);
 	printf("OK\n");