On Thu, Feb 16, 2006 at 08:01:38AM +0200, Oded Shimon wrote:
I dislike it. For this purpose, a pure bitmask with special case 0 is good enough... It's not quite a complication but it is simply not worth it... (bitmask has the slight disadvantage of overflowing, but seriously, 64 streams... Worst pathological scenario I can think of is 30 streams, 20 subtitles and 10 audio tracks)
a limitation to 30-60 streams is not acceptable
It's just a implementation limitation, not a spec one, but anyway, see new patch.
Agree. It's easier to have a custom v-reader/writer for arbitrary length bitflags than to have some huge mechanism to use ordinary read to 64bit integer types for this purpose.
That's somewhat a good point, but still not excuse enough IMO to put the info types in the main header, it literally defeats the purpose - info stuff goes in info packets, not in main headers...
then my vote is to simply not allow extending the table, its not needed anyway to add new fields to info packets
you either have a fixed table or you store the table in the file
Is the problem old programs not understanding new fields? It's not a big issue IMO...
Yes, I thought it was an issue before, but then realized it's probably not. In any case I'm against forbidding this in the design, in case we decide later that there are important fields we want to add.
Main difference in this patch is "type" is only for binary data. I couldn't decide an elegant way to design the table to accomodate this though...
Design of the table is an _implementation_ issue.
[Note: strings MUST be encoded in UTF-8] + [Note: strings MUST NOT be encoded with their terminating NUL]
IMO this should read "the character NUL (U+0000) is not legal within or at the end of a string."
+ code= id&3 + index= id>>2 + name= info_table[code][index][0] + if(name==NULL) name vb + if(code==0) { value s - else + } else if(code==1) { + nom s + denom v + value= nom/denom + } else if(code==2) { + mantissa s + exponent s + value= mantissa*pow(2,exp) + } else { + if(index) type= info_table[code][index][1] + else type vb value vb + }
OK IMO.
+info_table[4][][2]={ + { + {NULL , "i"}, // integer + }, + { + {NULL , "r"}, // rational + {"TrackTime" , "r"}, + }, + { + {NULL , "f"}, // float + },
IMO this part is silly/misleading, since "i", "r", and "f" are not "types".
+ { + {NULL , NULL }, + {NULL , "UTF8"}, + {"Author" , "UTF8"}, + {"Description" , "UTF8"}, + {"Copyright" , "UTF8"}, + {"Encoder" , "UTF8"}, + {"Title" , "UTF8"}, + {"Cover" , "JPEG"}, + {"Cover" , "PNG"}, + {"Source" , "UTF8"}, + {"CaptureDevice" , "UTF8"}, + {"CreationTime" , "UTF8"}, + {"Keywords" , "UTF8"}, + {"Language" , "UTF8"}, + {"Disposition" , "UTF8"}, + } }; + Note: No future entries will have NULL as a value.
Two comments: 1. Is it really necessary to have entry 1 ({NULL, "UTF8"}) ? It would be simpler if type were always coded when a custom name is. 2. Maybe this is stupid/pedantic, but the correct name is UTF-8, not UTF8. Is it worth wasting a byte to correct this? Rich