[vlc-devel] [PATCH] cc.c : Added basic features of CIA-708 to cc.c

[Devin Heitmueller]

Hi Greeshma,

It's good to see somebody working on EIA-708 support.  A few questions:

- It looks like this patch just creates some basic data structures and
enums.  Do you have additional patches which demonstrate this
functionality?  I would be hesitant to see anything committed that
doesn't actually result in a functional change in the code.  I think
it would be better to see the development happen on some branch, and
then the entire patch series can be submitted for upstream inclusion
once you have something that works.  The alternative tends to result
in code that needs to be heavily refactored as you make it actually
work, dead code that isn't actually needed in the final solution, and
introducing new constraints on existing functionality.

- I believe we're currently only feeding the EIA-608 bytes to the cc
decoder currently.  If we wanted to support EIA-708, the MPEG2 demux
would need to be modified to create additional decoder instances and
feed those bytes instead.

- We need to think about the GUI implications of supporting both
EIA-608 and 708.  Do we now show eight menu options for CC1-4 for
EIA-608 and CC1-4 for EIA-708?  Do we automatically pick EIA-708 over
EIA-608 (or the reverse) if both are present?  Do we have some global
configuration option which dictates which decoder gets used?

- The window anchor support we could potentially do without having to
support multiple subregions, given the current EIA-608 decoder always
assumes a 32x16 character window.  The anchor support could be simply
emulated by properly positioning the cursor within that window.

- Adding support for some of these features could exacerbate known
performance issues in the existing SPU renderer.  There are some
assumptions made with regards to things such as font size and
positioning which could cause a significant reduction in compositing
performance if we provide more flexibility as allowed by EIA-708.  I
can get into more specifics on this in a separate email.

On Tue, Mar 24, 2015 at 6:14 AM, greeshma <greeshmabalabadra at gmail.com> wrote:
>
> From 6260d6377da95f0dcc0e92e6342c4f9c540cc145 Mon Sep 17 00:00:00 2001
> From: greeshmab <greeshmabalabadra at gmail.com>
> Date: Tue, 24 Mar 2015 10:27:46 +0530
> Subject: [PATCH] Added basic features of CIA-708 from wiki page
>
> Signed-off-by: greeshma <greeshmabalabadra at gmail.com>
> ---
>  modules/codec/cc.c | 250
> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 250 insertions(+)
>
> diff --git a/modules/codec/cc.c b/modules/codec/cc.c
> index 91e35ee..89754f6 100644
> --- a/modules/codec/cc.c
> +++ b/modules/codec/cc.c
> @@ -1132,3 +1132,253 @@ static char *Eia608Text( eia608_t *h, bool b_html )
>          Eia608Strlcat( psz, "</text>", i_size );
>      return psz;
>  }
> +
> +
> +//Based on the Eia708 Wikipedia page
> +//For SetPenAttributes function, Code is based on CCExtractor code

This could be a problem.  The current code for cc.c is LGPL, and using
code from CCExtractor will result in the module having to be GPL
licensed.

Devin


-- 
Devin J. Heitmueller - Kernel Labs
http://www.kernellabs.com