Support reading the YCbCr Matrix header

[GoogleCodeExporter]

Splitting off issue #78.

This patch adds a way to read the YCbCr Matrix header introduced in xy-VSFilter 
and Aegisub last year. In short, it is the solution to all colour space issues 
ASS has.

The patch only makes libass parse the value of the header into an enum 
constant, and the actual colour correction is expected to be performed by the 
consumer (mplayer or VLC). I have started work on implementing it in mplayer2, 
and there is interest in implementing it in mpv.

The patch itself:
    http://code.google.com/r/astiob-libass/source/detail?r=295a71090a8ba60ef12e16f6105735eba3fe1fe7

There is a bit of discussion here:
    http://code.google.com/r/astiob-libass/source/detail?r=444fafe96dd90361a1fd3ff786b1eabdc5837836
(This is an older version of the patch.)

Original issue reported on code.google.com by chortos@inbox.lv on 2 Feb 2013 at 4:57

[GoogleCodeExporter]

Are all these colorspace values mandated by xy-vsfilter? That's nothing short 
of insane...

I see one real issue: parse_ycbcr_matrix mutates the input string, which might 
not be ok. The would make public API functions like ass_process_data mutates 
the buffer passed by the user. Maybe the best way to fix this is copying the 
value in a small buffer, bounded by the maximum valid value.

Also, maybe there should be a YCBCR_UNKNOWN value if the string can't be parsed.

Other than that, I think this patch is mostly ok, except for the following 
nits, but I can't blame you for them, because you did what the source already 
did (generally a good thing):
- The enum for YCbCrMatrix should be a separate type (but track_type does the 
same)
- parse_ycbcr_matrix is probably better in ass_parse.c (but there's already 
some other parsing code in ass_utils.c)
I'd say fixing these is not necessary, I'm just pointing it out.

Original comment by nfxjfg@googlemail.com on 3 Feb 2013 at 11:50

[GoogleCodeExporter]

PS: and all public identifiers should have an ASS_ prefix.

Original comment by nfxjfg@googlemail.com on 3 Feb 2013 at 12:18

[GoogleCodeExporter]

Yeah. In general it would be best to have a named enum with an ASS_ prefix and 
a getter function in ass.h, but there are no other getter functions and clients 
like mplayer2 widely use ASS_Track internals already.

xy-VSFilter actually supports only (TV|PC).(601|709) and None, but Aegisub does 
support writing 240M and FCC. The number of matrices doesn't really change the 
(in)sanity of the header: it just says that the script's native colorspace is 
YCbCr, converted for serialization purposes only to RGB using the specified 
matrix. The possible matrices are the same that are used for video.

As for mutating the input string, this in an internal function called from two 
places that both already mutate their inputs. To be fair, one of the two 
(ass_process_force_style) restores the original string after processing, so 
perhaps parse_ycbcr_matrix should do the same.

I'm not sure about YCBCR_UNKNOWN. Would it be any useful for libass users?

Original comment by chortos@inbox.lv on 3 Feb 2013 at 3:46

[GoogleCodeExporter]

> As for mutating the input string, this in an internal function called from 
two places that both already mutate their inputs. 

I would consider this a bug. Can you point out where else it mutates input data?

> one of the two (ass_process_force_style) restores the original string after 
processing

That's.... weird.

> I'm not sure about YCBCR_UNKNOWN. Would it be any useful for libass users?

Not really, but I would say this is just proper for a value that is only read 
so that the application can do policy decisions based on it. But I have no hard 
feelings about this.

Original comment by nfxjfg@googlemail.com on 3 Feb 2013 at 6:39

[GoogleCodeExporter]

Yeah, about the different values... I don't even know what this 'FCC' color 
space is. I does not make much sense to add something that will never be used. 
Same goes for the SMPTE ones. We should be fine with just BT601 and BT709; the 
value of this feature as a whole is questionable anyway.

Original comment by g...@chown.ath.cx on 4 Feb 2013 at 12:06

[GoogleCodeExporter]

> FCC
> SMPTE

I think they copied whatever they found in ffmpeg's avcodec.h.

Original comment by nfxjfg@googlemail.com on 4 Feb 2013 at 2:45

[GoogleCodeExporter]

parse_ycbcr_matrix is called from two functions:

* ass_process_force_style. It mutates the data it works with, but actually the 
memory is owned by libass (it's allocated in ass_set_style_overrides), so this 
is fine. It restores the original data after processing it.

* process_info_line. Which is called from process_line. Which is called from 
process_text, which mutates the data and doesn't bother restoring it. Which is 
called from:
    * parse_memory. Which is called from:
        * ass_read_file, which allocates the data by itself.
        * ass_read_memory, which is an API function that *uses the user-supplied pointer unchanged* if (#ifndef CONFIG_ICONV || !codepage).
    * ass_process_data, which is an API function that copies the user-supplied data into a buffer before doing anything else with it.

So the conclusion is that ass_read_memory irreversibly mutilates user data by 
sprinkling it with null bytes. This reminds me of an issue I noticed some time 
ago but apparently forgot about: there are two few const modifiers on pointers 
in the API.

A quick fix would be to make process_text restore the original data, but of 
course this fix is incomplete, even if sufficient in practice: theoretically 
the user-supplied point might point to read-only memory that causes a crash 
when written to. Copying the data into a buffer in ass_read_memory is probably 
the right solution. This essentially already happens anyway when iconv is used.

> I don't even know what this 'FCC' color space is. I does not make much sense 
to add something that will never be used. Same goes for the SMPTE ones. We 
should be fine with just BT601 and BT709

> I think they copied whatever they found in ffmpeg's avcodec.h.

Aegisub probably did. But avcodec.h copied the list from the MPEG-2 and MPEG-4 
standards. mplayer2 currently supports BT.601, BT.709 and 240M.  ffmpeg 
supports all four matrices, as does everything based on it (including 
ffmpegsource which is used by Aegisub). madVR supports all four as well, and in 
fact even at least one more, although it technically isn't YCbCr.

The only cost for libass with this patch is six simple lines of code per 
matrix. Not knowing what a particular matrix means is fine: that's the video 
player's job.

Oh, and I think I'm slightly in favour of YCBCR_UNKNOWN now. It could be used 
to print an error message, for example.

Original comment by chortos@inbox.lv on 5 Feb 2013 at 1:57

[GoogleCodeExporter]

I'll open a ticket for these issues.

As for parse_ycbcr_matrix(), I feel a bit bad for making contributors rewrite 
their patches over and over, but then I'm merely a contributor myself, and the 
it should be trivial enough to fix (just needs testing...).

I don't think the additional colorspaces are a problem, just unnecessary and 
confusing. But if Aegisub decided to write them, so be it.

Original comment by nfxjfg@googlemail.com on 8 Feb 2013 at 9:53

[GoogleCodeExporter]

Sorry, I got distracted by other things. :( What solution did we settle on? Do 
I just restore the original value (and hope that the caller ensures the memory 
is writeable and thread-local), or should I put in a bit more effort and avoid 
ever modifying the input?

Original comment by chortos@inbox.lv on 25 Feb 2013 at 12:26

[GoogleCodeExporter]

I'd suggest that your patch doesn't write to the input buffer. It would be easy 
to change. You could copy the input string to a small temporary buffer on the 
stack (since you know what the longest match will be, the buffer can be fixed 
size). This would cause less trouble for the pour soul who will eventually fix 
the write accesses to the input buffer in other parts of libass.

Original comment by nfxjfg@googlemail.com on 25 Feb 2013 at 9:25

[GoogleCodeExporter]

Great idea! I was thinking along the lines of strncmp, which was getting ugly 
pretty quickly.

Right, I've implemented this (and YCBCR_UNKNOWN) and pushed.

Original comment by chortos@inbox.lv on 25 Feb 2013 at 10:24

[GoogleCodeExporter]

Sorry for double-{posting,emailing}, but I just realized a direct link would be 
helpful:
    http://code.google.com/r/astiob-libass/source/detail?r=7f21f204daf80fefe364a0e48847d741af43656c

Original comment by chortos@inbox.lv on 25 Feb 2013 at 10:26

[GoogleCodeExporter]

I'd say this is ok.

Original comment by nfxjfg@googlemail.com on 25 Feb 2013 at 11:01

[GoogleCodeExporter]

Pushed. Sorry for the unbearable long delays. :(

Original comment by g...@chown.ath.cx on 3 Mar 2013 at 10:18

• Changed state: Fixed