Re: [libav-devel] [PATCH] mpeg12dec: Extract CC user data into frame side data

2013-11-25 Thread Vittorio Giovara
On Mon, Nov 25, 2013 at 8:08 AM, Anton Khirnov an...@khirnov.net wrote:

 On Sun, 24 Nov 2013 09:49:47 -0800, John Stebbins stebb...@jetheaddev.com 
 wrote:
 ---
  libavcodec/avcodec.h   |  5 
  libavcodec/mpeg12dec.c | 64 
 ++
  libavutil/frame.h  |  4 
  3 files changed, 73 insertions(+)

 diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
 index 4ce6d61..9e7d968 100644
 --- a/libavcodec/avcodec.h
 +++ b/libavcodec/avcodec.h
 @@ -845,6 +845,11 @@ typedef struct AVPanScan{
  int16_t position[3][2];
  }AVPanScan;

 +typedef struct AVClosedCaption {
 +int count;
 +uint8_t data[1];
 +} AVClosedCaption;

 First, those two fields should be documented. Perhaps it's obvious to you what
 they mean, but it's not so obvious to me. E.g. my first assumption would be 
 that
 count is the size of data in bytes, but looking at the code it is not so.

 Second, this is apparrently a dump of some specific wire format, right? Then 
 it
 should be documented which one it is and the struct and the side data type
 should have more specific names, so we can add other CC formats later.


Also, is it right to put this in avcodec? Maybe there can be filters
and/or utilities that might want to play with closed captions, but I'm
not familiar enough with the subject to say whether that's correct or
not.

I recommend a MICRO bump of the library this gets added to anyway.
Cheers,
Vittorio
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel


Re: [libav-devel] [PATCH] mpeg12dec: Extract CC user data into frame side data

2013-11-25 Thread John Stebbins
On 11/24/2013 09:44 PM, Diego Biurrun wrote:
 You return different values above, but no error checking whatsoever is
 performed here.  What was your intention?


Yes.  Although a malloc failure would most likely prove fatal later, it's not 
really a fatal problem here IMO. Loss of
subtitles due to an error seems reasonable to me.

-- 
John  GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01  83F0 49F1 D7B2 60D4 D0F7




signature.asc
Description: OpenPGP digital signature
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] mpeg12dec: Extract CC user data into frame side data

2013-11-25 Thread John Stebbins
On 11/25/2013 12:36 AM, Vittorio Giovara wrote:
 On Mon, Nov 25, 2013 at 8:08 AM, Anton Khirnov an...@khirnov.net wrote:
 On Sun, 24 Nov 2013 09:49:47 -0800, John Stebbins stebb...@jetheaddev.com 
 wrote:
 ---
  libavcodec/avcodec.h   |  5 
  libavcodec/mpeg12dec.c | 64 
 ++
  libavutil/frame.h  |  4 
  3 files changed, 73 insertions(+)

 diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
 index 4ce6d61..9e7d968 100644
 --- a/libavcodec/avcodec.h
 +++ b/libavcodec/avcodec.h
 @@ -845,6 +845,11 @@ typedef struct AVPanScan{
  int16_t position[3][2];
  }AVPanScan;

 +typedef struct AVClosedCaption {
 +int count;
 +uint8_t data[1];
 +} AVClosedCaption;
 First, those two fields should be documented. Perhaps it's obvious to you 
 what
 they mean, but it's not so obvious to me. E.g. my first assumption would be 
 that
 count is the size of data in bytes, but looking at the code it is not so.

 Second, this is apparrently a dump of some specific wire format, right? Then 
 it
 should be documented which one it is and the struct and the side data type
 should have more specific names, so we can add other CC formats later.

 Also, is it right to put this in avcodec? Maybe there can be filters
 and/or utilities that might want to play with closed captions, but I'm
 not familiar enough with the subject to say whether that's correct or
 not.

 I recommend a MICRO bump of the library this gets added to anyway.
 Cheers,
 Vittorio

CC data can be on it's own PID in a transport stream (DTVCC) which would make 
it an avformat thing.  But in this case,
you don't need SideData and the raw data would be returned in an AVPacket. I 
can't think of an occasion where you would
want to reuse this struct for anything but AVFrameSideData.

Roger on the micro bump.

-- 
John  GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01  83F0 49F1 D7B2 60D4 D0F7




signature.asc
Description: OpenPGP digital signature
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] mpeg12dec: Extract CC user data into frame side data

2013-11-25 Thread John Stebbins
On 11/24/2013 09:44 PM, Diego Biurrun wrote:
 On Sun, Nov 24, 2013 at 09:49:47AM -0800, John Stebbins wrote:
 --- a/libavcodec/mpeg12dec.c
 +++ b/libavcodec/mpeg12dec.c
 @@ -1529,6 +1530,15 @@ static int mpeg_field_start(MpegEncContext *s, const 
 uint8_t *buf, int buf_size)
  
 +if (s1-caption != NULL) {
 !s1-caption

 +if (sd != NULL) {
 +memcpy(sd-data, s1-caption, size);
 +}
 same, drop {}

Just verifying, this is the preferred style?  Because I see several other 
places in the code that enclose single lines
in braces.  I even see some other pending patches that are adding code that 
does this. I prefer the extra braces for
readability and it makes it so future patches don't require as much formatting 
changes. 

And treating pointers as booleans is a pet peeve of mine.  The code is more 
self documenting if you compare pointers to
pointers, ints to ints and save boolean operators for things that are truly 
meant to be boolean. But if this is libav
way, I won't say any more about it.

-- 
John  GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01  83F0 49F1 D7B2 60D4 D0F7




signature.asc
Description: OpenPGP digital signature
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] mpeg12dec: Extract CC user data into frame side data

2013-11-25 Thread John Stebbins

On 11/24/2013 11:08 PM, Anton Khirnov wrote:
 On Sun, 24 Nov 2013 09:49:47 -0800, John Stebbins stebb...@jetheaddev.com 
 wrote:
 ---
  libavcodec/avcodec.h   |  5 
  libavcodec/mpeg12dec.c | 64 
 ++
  libavutil/frame.h  |  4 
  3 files changed, 73 insertions(+)

 diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
 index 4ce6d61..9e7d968 100644
 --- a/libavcodec/avcodec.h
 +++ b/libavcodec/avcodec.h
 @@ -845,6 +845,11 @@ typedef struct AVPanScan{
  int16_t position[3][2];
  }AVPanScan;
  
 +typedef struct AVClosedCaption {
 +int count;
 +uint8_t data[1];
 +} AVClosedCaption;
 First, those two fields should be documented. Perhaps it's obvious to you what
 they mean, but it's not so obvious to me. E.g. my first assumption would be 
 that
 count is the size of data in bytes, but looking at the code it is not so.
Would you prefer that this be the byte count?  Taking a second look at it, this 
may be more obvious and doesn't really
have much effect on the CC decoder side of things.

 Second, this is apparrently a dump of some specific wire format, right? Then 
 it
 should be documented which one it is and the struct and the side data type
 should have more specific names, so we can add other CC formats later.
Will do.  I thought about parsing out the fields here. But that would have 
complicated feeding this data to a CC decoder
that expects the raw data as input.  I figured parsing of CC details really 
belongs in a CC decoder.

-- 
John  GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01  83F0 49F1 D7B2 60D4 D0F7




signature.asc
Description: OpenPGP digital signature
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] mpeg12dec: Extract CC user data into frame side data

2013-11-25 Thread Luca Barbato
On 25/11/13 16:41, John Stebbins wrote:

 Just verifying, this is the preferred style?  Because I see several
 other places in the code that enclose single lines in braces.

Basically

if (!foo)
bar = small_function(a, b);


if ((large condition()) != blah)) {
bar = do_something_with_a_(lot, of, args,
   and + computation,
   over more lines);
}

In words, if the condition is short and/or the single line conditional
statement is short and it isn't the last item of an if () { } else {},
drop the braces, keep them otherwise.

 I even
 see some other pending patches that are adding code that does this. I
 prefer the extra braces for readability and it makes it so future
 patches don't require as much formatting changes.

Personally I do agree, but the coding style is a compromise =)

 And treating pointers as booleans is a pet peeve of mine.  The code
 is more self documenting if you compare pointers to pointers, ints to
 ints and save boolean operators for things that are truly meant to be
 boolean. But if this is libav way, I won't say any more about it.

Thank you =)

lu
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel


Re: [libav-devel] [PATCH] mpeg12dec: Extract CC user data into frame side data

2013-11-25 Thread Anton Khirnov

On Mon, 25 Nov 2013 08:15:28 -0800, John Stebbins stebb...@jetheaddev.com 
wrote:
 
 On 11/24/2013 11:08 PM, Anton Khirnov wrote:
  On Sun, 24 Nov 2013 09:49:47 -0800, John Stebbins stebb...@jetheaddev.com 
  wrote:
  ---
   libavcodec/avcodec.h   |  5 
   libavcodec/mpeg12dec.c | 64 
  ++
   libavutil/frame.h  |  4 
   3 files changed, 73 insertions(+)
 
  diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
  index 4ce6d61..9e7d968 100644
  --- a/libavcodec/avcodec.h
  +++ b/libavcodec/avcodec.h
  @@ -845,6 +845,11 @@ typedef struct AVPanScan{
   int16_t position[3][2];
   }AVPanScan;
   
  +typedef struct AVClosedCaption {
  +int count;
  +uint8_t data[1];
  +} AVClosedCaption;
  First, those two fields should be documented. Perhaps it's obvious to you 
  what
  they mean, but it's not so obvious to me. E.g. my first assumption would be 
  that
  count is the size of data in bytes, but looking at the code it is not so.
 Would you prefer that this be the byte count?  Taking a second look at it, 
 this may be more obvious and doesn't really
 have much effect on the CC decoder side of things.

I don't know what I'd prefer because I still don't know what does count mean ;)
Is it a number of elements (whatever those are)?

Does the caller even need to know it? Perhaps it'd be better to make this just a
plain uint8_t* array instead of a struct.

-- 
Anton Khirnov
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel


Re: [libav-devel] [PATCH] mpeg12dec: Extract CC user data into frame side data

2013-11-25 Thread John Stebbins
On 11/25/2013 09:02 AM, Anton Khirnov wrote:
 On Mon, 25 Nov 2013 08:15:28 -0800, John Stebbins stebb...@jetheaddev.com 
 wrote:
 On 11/24/2013 11:08 PM, Anton Khirnov wrote:
 On Sun, 24 Nov 2013 09:49:47 -0800, John Stebbins stebb...@jetheaddev.com 
 wrote:
 ---
  libavcodec/avcodec.h   |  5 
  libavcodec/mpeg12dec.c | 64 
 ++
  libavutil/frame.h  |  4 
  3 files changed, 73 insertions(+)

 diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
 index 4ce6d61..9e7d968 100644
 --- a/libavcodec/avcodec.h
 +++ b/libavcodec/avcodec.h
 @@ -845,6 +845,11 @@ typedef struct AVPanScan{
  int16_t position[3][2];
  }AVPanScan;
  
 +typedef struct AVClosedCaption {
 +int count;
 +uint8_t data[1];
 +} AVClosedCaption;
 First, those two fields should be documented. Perhaps it's obvious to you 
 what
 they mean, but it's not so obvious to me. E.g. my first assumption would be 
 that
 count is the size of data in bytes, but looking at the code it is not so.
 Would you prefer that this be the byte count?  Taking a second look at it, 
 this may be more obvious and doesn't really
 have much effect on the CC decoder side of things.
 I don't know what I'd prefer because I still don't know what does count mean 
 ;)
 Is it a number of elements (whatever those are)?

Yes, it is currently the number of elements which are a fixed size of 3 bytes 
per element.


 Does the caller even need to know it? Perhaps it'd be better to make this 
 just a
 plain uint8_t* array instead of a struct.

The caller only needs to know it for the purposes of passing it along to a CC 
decoder.  So yes, we could certainly just
use AVFrameSideData.size for this.  I would have to add documentation around 
the new side data type AV_FRAME_DATA_A53_CC
to make it clear that AVFrameSideData.size specifies the number of CC bytes, 
but it eliminates duplicate information
that isn't strictly necessary.  Is this what you would prefer?

-- 
John  GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01  83F0 49F1 D7B2 60D4 D0F7




signature.asc
Description: OpenPGP digital signature
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

Re: [libav-devel] [PATCH] mpeg12dec: Extract CC user data into frame side data

2013-11-25 Thread Anton Khirnov

On Mon, 25 Nov 2013 09:14:17 -0800, John Stebbins stebb...@jetheaddev.com 
wrote:
 On 11/25/2013 09:02 AM, Anton Khirnov wrote:
  On Mon, 25 Nov 2013 08:15:28 -0800, John Stebbins stebb...@jetheaddev.com 
  wrote:
  On 11/24/2013 11:08 PM, Anton Khirnov wrote:
  On Sun, 24 Nov 2013 09:49:47 -0800, John Stebbins 
  stebb...@jetheaddev.com wrote:
  ---
   libavcodec/avcodec.h   |  5 
   libavcodec/mpeg12dec.c | 64 
  ++
   libavutil/frame.h  |  4 
   3 files changed, 73 insertions(+)
 
  diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
  index 4ce6d61..9e7d968 100644
  --- a/libavcodec/avcodec.h
  +++ b/libavcodec/avcodec.h
  @@ -845,6 +845,11 @@ typedef struct AVPanScan{
   int16_t position[3][2];
   }AVPanScan;
   
  +typedef struct AVClosedCaption {
  +int count;
  +uint8_t data[1];
  +} AVClosedCaption;
  First, those two fields should be documented. Perhaps it's obvious to you 
  what
  they mean, but it's not so obvious to me. E.g. my first assumption would 
  be that
  count is the size of data in bytes, but looking at the code it is not so.
  Would you prefer that this be the byte count?  Taking a second look at it, 
  this may be more obvious and doesn't really
  have much effect on the CC decoder side of things.
  I don't know what I'd prefer because I still don't know what does count 
  mean ;)
  Is it a number of elements (whatever those are)?
 
 Yes, it is currently the number of elements which are a fixed size of 3 bytes 
 per element.
 
 
  Does the caller even need to know it? Perhaps it'd be better to make this 
  just a
  plain uint8_t* array instead of a struct.
 
 The caller only needs to know it for the purposes of passing it along to a CC 
 decoder.  So yes, we could certainly just
 use AVFrameSideData.size for this.  I would have to add documentation around 
 the new side data type AV_FRAME_DATA_A53_CC
 to make it clear that AVFrameSideData.size specifies the number of CC bytes, 
 but it eliminates duplicate information
 that isn't strictly necessary.  Is this what you would prefer?
 

Yes, that sounds better to me.

-- 
Anton Khirnov
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel


Re: [libav-devel] [PATCH] mpeg12dec: Extract CC user data into frame side data

2013-11-25 Thread John Stebbins
On 11/24/2013 09:44 PM, Diego Biurrun wrote:
 On Sun, Nov 24, 2013 at 09:49:47AM -0800, John Stebbins wrote:
 @@ -2057,6 +2118,8 @@ static void mpeg_decode_user_data(AVCodecContext 
 *avctx,
  return;
  avctx-dtg_active_format = p[0]  0x0f;
  }
 +} else if (mpeg_decode_cc(avctx, p, buf_size)) {
 +return;
  }
 You return different values above, but no error checking whatsoever is
 performed here.  What was your intention?


I might not have completely understood you meaning the first time I read this.  
The intention here is to return early if
mpeg_decode_cc detected that this user data is cc data and processed it. 
There's no point in continuing to look for
other types of user data. It's really kind of pointless at present since it is 
the last thing in the function
currently.  But it sets the pattern to follow if anyone adds more user data 
parsing after it.  It can be dropped and we
can just rely on the next person that adds more user data parsing to do this 
right if you wish.

-- 
John  GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01  83F0 49F1 D7B2 60D4 D0F7




signature.asc
Description: OpenPGP digital signature
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel

[libav-devel] [PATCH] mpeg12dec: Extract CC user data into frame side data

2013-11-25 Thread John Stebbins
---
 libavcodec/mpeg12dec.c | 65 ++
 libavutil/frame.h  |  6 +
 2 files changed, 71 insertions(+)

diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
index 0e11dda..8fccf2b 100644
--- a/libavcodec/mpeg12dec.c
+++ b/libavcodec/mpeg12dec.c
@@ -44,6 +44,8 @@ typedef struct Mpeg1Context {
 int mpeg_enc_ctx_allocated; /* true if decoding context allocated */
 int repeat_field; /* true if we must repeat the field */
 AVPanScan pan_scan;  /** some temporary storage for the 
panscan */
+uint8_t *a53_caption;
+int a53_caption_size;
 int slice_count;
 int save_aspect_info;
 int save_width, save_height, save_progressive_seq;
@@ -1529,6 +1531,14 @@ static int mpeg_field_start(MpegEncContext *s, const 
uint8_t *buf, int buf_size)
 return AVERROR(ENOMEM);
 memcpy(pan_scan-data, s1-pan_scan, sizeof(s1-pan_scan));
 
+if (s1-a53_caption) {
+AVFrameSideData *sd = av_frame_new_side_data(
+s-current_picture_ptr-f, AV_FRAME_DATA_A53_CC,
+s1-a53_caption_size);
+if (sd)
+memcpy(sd-data, s1-a53_caption, s1-a53_caption_size);
+av_freep(s1-a53_caption);
+}
 if (HAVE_THREADS  (avctx-active_thread_type  FF_THREAD_FRAME))
 ff_thread_finish_setup(avctx);
 } else { // second field
@@ -2038,6 +2048,58 @@ static int vcr2_init_sequence(AVCodecContext *avctx)
 }
 
 
+static int mpeg_decode_a53_cc(AVCodecContext *avctx,
+  const uint8_t *p, int buf_size)
+{
+Mpeg1Context *s1 = avctx-priv_data;
+
+if (buf_size = 6 
+p[0] == 'G'  p[1] == 'A'  p[2] == '9'  p[3] == '4' 
+p[4] == 3  (p[5]  0x40)) {
+/* extract A53 Part 4 CC data */
+int cc_count = p[5]  0x1f;
+if (cc_count  0  buf_size = 7 + cc_count * 3) {
+s1-a53_caption_size = cc_count * 3;
+s1-a53_caption = av_malloc(s1-a53_caption_size);
+if (s1-a53_caption) {
+memcpy(s1-a53_caption, p + 7, s1-a53_caption_size);
+}
+}
+return 1;
+} else if (buf_size = 11 
+p[0] == 'C'  p[1] == 'C'  p[2] == 0x01  p[3] == 0xf8) {
+/* extract DVD CC data */
+int cc_count = 0;
+int i;
+// There is a caption count field in the data, but it is often
+// incorect.  So count the number of captions present.
+for (i = 5; i + 6 = buf_size  ((p[i]  0xfe) == 0xfe); i += 6)
+cc_count++;
+// Transform the DVD format into A53 Part 4 format
+if (cc_count  0) {
+s1-a53_caption_size = cc_count * 6;
+s1-a53_caption = av_malloc(s1-a53_caption_size);
+if (s1-a53_caption) {
+uint8_t field1 = !!(p[4]  0x80);
+uint8_t *cap = s1-a53_caption;
+p += 5;
+for (i = 0; i  cc_count; i++) {
+cap[0] = (p[0] == 0xff  field1) ? 0xfc : 0xfd;
+cap[1] = p[1];
+cap[2] = p[2];
+cap[3] = (p[3] == 0xff  !field1) ? 0xfc : 0xfd;
+cap[4] = p[4];
+cap[5] = p[5];
+cap += 6;
+p += 6;
+}
+}
+}
+return 1;
+}
+return 0;
+}
+
 static void mpeg_decode_user_data(AVCodecContext *avctx,
   const uint8_t *p, int buf_size)
 {
@@ -2057,6 +2119,8 @@ static void mpeg_decode_user_data(AVCodecContext *avctx,
 return;
 avctx-dtg_active_format = p[0]  0x0f;
 }
+} else if (mpeg_decode_a53_cc(avctx, p, buf_size)) {
+return;
 }
 }
 
@@ -2402,6 +2466,7 @@ static av_cold int mpeg_decode_end(AVCodecContext *avctx)
 
 if (s-mpeg_enc_ctx_allocated)
 ff_MPV_common_end(s-mpeg_enc_ctx);
+av_freep(s-a53_caption);
 return 0;
 }
 
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 449a6b7..d869d83 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -35,6 +35,12 @@ enum AVFrameSideDataType {
  * The data is the AVPanScan struct defined in libavcodec.
  */
 AV_FRAME_DATA_PANSCAN,
+/**
+ * ATSC A53 Part 4 Closed Captions.
+ * A53 CC bitstream is stored as uint8_t in AVFrameSideData.data.
+ * The number of bytes of CC data is AVFrameSideData.size.
+ */
+AV_FRAME_DATA_A53_CC,
 };
 
 typedef struct AVFrameSideData {
-- 
1.8.3.1

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel


Re: [libav-devel] [PATCH] mpeg12dec: Extract CC user data into frame side data

2013-11-25 Thread Anton Khirnov

On Mon, 25 Nov 2013 10:39:08 -0800, John Stebbins stebb...@jetheaddev.com 
wrote:
 ---
  libavcodec/mpeg12dec.c | 65 
 ++
  libavutil/frame.h  |  6 +
  2 files changed, 71 insertions(+)
 
 diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
 index 0e11dda..8fccf2b 100644
 --- a/libavcodec/mpeg12dec.c
 +++ b/libavcodec/mpeg12dec.c
 @@ -44,6 +44,8 @@ typedef struct Mpeg1Context {
  int mpeg_enc_ctx_allocated; /* true if decoding context allocated */
  int repeat_field; /* true if we must repeat the field */
  AVPanScan pan_scan;  /** some temporary storage for the 
 panscan */
 +uint8_t *a53_caption;
 +int a53_caption_size;
  int slice_count;
  int save_aspect_info;
  int save_width, save_height, save_progressive_seq;
 @@ -1529,6 +1531,14 @@ static int mpeg_field_start(MpegEncContext *s, const 
 uint8_t *buf, int buf_size)
  return AVERROR(ENOMEM);
  memcpy(pan_scan-data, s1-pan_scan, sizeof(s1-pan_scan));
  
 +if (s1-a53_caption) {
 +AVFrameSideData *sd = av_frame_new_side_data(
 +s-current_picture_ptr-f, AV_FRAME_DATA_A53_CC,
 +s1-a53_caption_size);
 +if (sd)
 +memcpy(sd-data, s1-a53_caption, s1-a53_caption_size);
 +av_freep(s1-a53_caption);
 +}
  if (HAVE_THREADS  (avctx-active_thread_type  FF_THREAD_FRAME))
  ff_thread_finish_setup(avctx);
  } else { // second field
 @@ -2038,6 +2048,58 @@ static int vcr2_init_sequence(AVCodecContext *avctx)
  }
  
  
 +static int mpeg_decode_a53_cc(AVCodecContext *avctx,
 +  const uint8_t *p, int buf_size)
 +{
 +Mpeg1Context *s1 = avctx-priv_data;
 +
 +if (buf_size = 6 
 +p[0] == 'G'  p[1] == 'A'  p[2] == '9'  p[3] == '4' 
 +p[4] == 3  (p[5]  0x40)) {
 +/* extract A53 Part 4 CC data */
 +int cc_count = p[5]  0x1f;
 +if (cc_count  0  buf_size = 7 + cc_count * 3) {
 +s1-a53_caption_size = cc_count * 3;
 +s1-a53_caption = av_malloc(s1-a53_caption_size);

Seems to me this could leak a53_caption remaining from the previous call (if we
never get to mpeg_field_start()), so you should free it to be safe.

Otherwise I think the patch is fine.

-- 
Anton Khirnov
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel


[libav-devel] [PATCH] mpeg12dec: Extract CC user data into frame side data

2013-11-25 Thread John Stebbins
---
 libavcodec/mpeg12dec.c | 67 ++
 libavutil/frame.h  |  6 +
 2 files changed, 73 insertions(+)

diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
index 0e11dda..9d0c3be 100644
--- a/libavcodec/mpeg12dec.c
+++ b/libavcodec/mpeg12dec.c
@@ -44,6 +44,8 @@ typedef struct Mpeg1Context {
 int mpeg_enc_ctx_allocated; /* true if decoding context allocated */
 int repeat_field; /* true if we must repeat the field */
 AVPanScan pan_scan;  /** some temporary storage for the 
panscan */
+uint8_t *a53_caption;
+int a53_caption_size;
 int slice_count;
 int save_aspect_info;
 int save_width, save_height, save_progressive_seq;
@@ -1529,6 +1531,14 @@ static int mpeg_field_start(MpegEncContext *s, const 
uint8_t *buf, int buf_size)
 return AVERROR(ENOMEM);
 memcpy(pan_scan-data, s1-pan_scan, sizeof(s1-pan_scan));
 
+if (s1-a53_caption) {
+AVFrameSideData *sd = av_frame_new_side_data(
+s-current_picture_ptr-f, AV_FRAME_DATA_A53_CC,
+s1-a53_caption_size);
+if (sd)
+memcpy(sd-data, s1-a53_caption, s1-a53_caption_size);
+av_freep(s1-a53_caption);
+}
 if (HAVE_THREADS  (avctx-active_thread_type  FF_THREAD_FRAME))
 ff_thread_finish_setup(avctx);
 } else { // second field
@@ -2038,6 +2048,60 @@ static int vcr2_init_sequence(AVCodecContext *avctx)
 }
 
 
+static int mpeg_decode_a53_cc(AVCodecContext *avctx,
+  const uint8_t *p, int buf_size)
+{
+Mpeg1Context *s1 = avctx-priv_data;
+
+if (buf_size = 6 
+p[0] == 'G'  p[1] == 'A'  p[2] == '9'  p[3] == '4' 
+p[4] == 3  (p[5]  0x40)) {
+/* extract A53 Part 4 CC data */
+int cc_count = p[5]  0x1f;
+if (cc_count  0  buf_size = 7 + cc_count * 3) {
+av_freep(s1-a53_caption);
+s1-a53_caption_size = cc_count * 3;
+s1-a53_caption = av_malloc(s1-a53_caption_size);
+if (s1-a53_caption) {
+memcpy(s1-a53_caption, p + 7, s1-a53_caption_size);
+}
+}
+return 1;
+} else if (buf_size = 11 
+p[0] == 'C'  p[1] == 'C'  p[2] == 0x01  p[3] == 0xf8) {
+/* extract DVD CC data */
+int cc_count = 0;
+int i;
+// There is a caption count field in the data, but it is often
+// incorect.  So count the number of captions present.
+for (i = 5; i + 6 = buf_size  ((p[i]  0xfe) == 0xfe); i += 6)
+cc_count++;
+// Transform the DVD format into A53 Part 4 format
+if (cc_count  0) {
+av_freep(s1-a53_caption);
+s1-a53_caption_size = cc_count * 6;
+s1-a53_caption = av_malloc(s1-a53_caption_size);
+if (s1-a53_caption) {
+uint8_t field1 = !!(p[4]  0x80);
+uint8_t *cap = s1-a53_caption;
+p += 5;
+for (i = 0; i  cc_count; i++) {
+cap[0] = (p[0] == 0xff  field1) ? 0xfc : 0xfd;
+cap[1] = p[1];
+cap[2] = p[2];
+cap[3] = (p[3] == 0xff  !field1) ? 0xfc : 0xfd;
+cap[4] = p[4];
+cap[5] = p[5];
+cap += 6;
+p += 6;
+}
+}
+}
+return 1;
+}
+return 0;
+}
+
 static void mpeg_decode_user_data(AVCodecContext *avctx,
   const uint8_t *p, int buf_size)
 {
@@ -2057,6 +2121,8 @@ static void mpeg_decode_user_data(AVCodecContext *avctx,
 return;
 avctx-dtg_active_format = p[0]  0x0f;
 }
+} else if (mpeg_decode_a53_cc(avctx, p, buf_size)) {
+return;
 }
 }
 
@@ -2402,6 +2468,7 @@ static av_cold int mpeg_decode_end(AVCodecContext *avctx)
 
 if (s-mpeg_enc_ctx_allocated)
 ff_MPV_common_end(s-mpeg_enc_ctx);
+av_freep(s-a53_caption);
 return 0;
 }
 
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 449a6b7..d869d83 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -35,6 +35,12 @@ enum AVFrameSideDataType {
  * The data is the AVPanScan struct defined in libavcodec.
  */
 AV_FRAME_DATA_PANSCAN,
+/**
+ * ATSC A53 Part 4 Closed Captions.
+ * A53 CC bitstream is stored as uint8_t in AVFrameSideData.data.
+ * The number of bytes of CC data is AVFrameSideData.size.
+ */
+AV_FRAME_DATA_A53_CC,
 };
 
 typedef struct AVFrameSideData {
-- 
1.8.3.1

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel


Re: [libav-devel] [PATCH] mpeg12dec: Extract CC user data into frame side data

2013-11-25 Thread Anton Khirnov

On Mon, 25 Nov 2013 15:57:28 -0800, John Stebbins stebb...@jetheaddev.com 
wrote:
 ---
  libavcodec/mpeg12dec.c | 67 
 ++
  libavutil/frame.h  |  6 +
  2 files changed, 73 insertions(+)
 

Added a version bump and an APIchanges entry and queued.
Thanks.

-- 
Anton Khirnov
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel


[libav-devel] [PATCH] mpeg12dec: Extract CC user data into frame side data

2013-11-24 Thread John Stebbins
---
 libavcodec/avcodec.h   |  5 
 libavcodec/mpeg12dec.c | 64 ++
 libavutil/frame.h  |  1 +
 3 files changed, 70 insertions(+)

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 4ce6d61..9e7d968 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -845,6 +845,11 @@ typedef struct AVPanScan{
 int16_t position[3][2];
 }AVPanScan;
 
+typedef struct AVClosedCaption {
+int count;
+uint8_t data[1];
+} AVClosedCaption;
+
 #if FF_API_QSCALE_TYPE
 #define FF_QSCALE_TYPE_MPEG1 0
 #define FF_QSCALE_TYPE_MPEG2 1
diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
index 0e11dda..19a91a1 100644
--- a/libavcodec/mpeg12dec.c
+++ b/libavcodec/mpeg12dec.c
@@ -44,6 +44,7 @@ typedef struct Mpeg1Context {
 int mpeg_enc_ctx_allocated; /* true if decoding context allocated */
 int repeat_field; /* true if we must repeat the field */
 AVPanScan pan_scan;  /** some temporary storage for the 
panscan */
+AVClosedCaption *caption;
 int slice_count;
 int save_aspect_info;
 int save_width, save_height, save_progressive_seq;
@@ -1529,6 +1530,15 @@ static int mpeg_field_start(MpegEncContext *s, const 
uint8_t *buf, int buf_size)
 return AVERROR(ENOMEM);
 memcpy(pan_scan-data, s1-pan_scan, sizeof(s1-pan_scan));
 
+if (s1-caption != NULL) {
+int size = sizeof(AVClosedCaption) + s1-caption-count * 3;
+AVFrameSideData *sd = av_frame_new_side_data(
+s-current_picture_ptr-f, AV_FRAME_DATA_CC, size);
+if (sd != NULL) {
+memcpy(sd-data, s1-caption, size);
+}
+av_freep(s1-caption);
+}
 if (HAVE_THREADS  (avctx-active_thread_type  FF_THREAD_FRAME))
 ff_thread_finish_setup(avctx);
 } else { // second field
@@ -2038,6 +2048,57 @@ static int vcr2_init_sequence(AVCodecContext *avctx)
 }
 
 
+static int mpeg_decode_cc(AVCodecContext *avctx, const uint8_t *p, int 
buf_size)
+{
+Mpeg1Context *s1 = avctx-priv_data;
+
+if (buf_size = 6 
+p[0] == 'G'  p[1] == 'A'  p[2] == '9'  p[3] == '4'  p[4] == 3) 
{
+/* extract A53 Part 4 CC data */
+int cc_count = p[5]  0x1f;
+if (cc_count  0  buf_size = 7 + cc_count * 3) {
+s1-caption = av_malloc(sizeof(AVClosedCaption) + cc_count * 3);
+if (s1-caption != NULL) {
+s1-caption-count = cc_count;
+memcpy(s1-caption-data, p + 7, cc_count * 3);
+}
+}
+return 1;
+} else if (buf_size = 11 
+p[0] == 'C'  p[1] == 'C'  p[2] == 0x01  p[3] == 0xf8) {
+/* extract DVD CC data */
+int cc_count = 0;
+int i;
+// There is a caption count field in the data, but it is often
+// incorect.  So count the number of captions present.
+for (i = 5; i + 6 = buf_size  ((p[i]  0xfe) == 0xfe); i += 6) {
+cc_count++;
+}
+// Transform the DVD format into A53 Part 4 format
+if (cc_count  0) {
+s1-caption = av_malloc(sizeof(AVClosedCaption) + cc_count * 6);
+if (s1-caption != NULL) {
+uint8_t field1 = !!(p[4]  0x80);
+uint8_t *cap = s1-caption-data;
+p += 5;
+s1-caption-count = cc_count * 2;
+for (i = 0; i  cc_count; i++) {
+cap[0] = (p[0] == 0xff  field1) ? 0xfc : 0xfd;
+cap[1] = p[1];
+cap[2] = p[2];
+cap[3] = (p[3] == 0xff  !field1) ? 0xfc : 0xfd;
+cap[4] = p[4];
+cap[5] = p[5];
+cap += 6;
+p += 6;
+}
+}
+}
+return 1;
+}
+return 0;
+}
+
 static void mpeg_decode_user_data(AVCodecContext *avctx,
   const uint8_t *p, int buf_size)
 {
@@ -2057,6 +2118,8 @@ static void mpeg_decode_user_data(AVCodecContext *avctx,
 return;
 avctx-dtg_active_format = p[0]  0x0f;
 }
+} else if (mpeg_decode_cc(avctx, p, buf_size)) {
+return;
 }
 }
 
@@ -2402,6 +2465,7 @@ static av_cold int mpeg_decode_end(AVCodecContext *avctx)
 
 if (s-mpeg_enc_ctx_allocated)
 ff_MPV_common_end(s-mpeg_enc_ctx);
+av_freep(s-caption);
 return 0;
 }
 
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 449a6b7..2361233 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -35,6 +35,7 @@ enum AVFrameSideDataType {
  * The data is the AVPanScan struct defined in libavcodec.
  */
 AV_FRAME_DATA_PANSCAN,
+AV_FRAME_DATA_CC,
 };
 
 typedef struct AVFrameSideData {
-- 
1.8.3.1

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel


Re: [libav-devel] [PATCH] mpeg12dec: Extract CC user data into frame side data

2013-11-24 Thread Tim W.
On 24 Nov 2013, at 18:08, John Stebbins stebb...@jetheaddev.com wrote:

 diff --git a/libavutil/frame.h b/libavutil/frame.h
 index 449a6b7..2361233 100644
 --- a/libavutil/frame.h
 +++ b/libavutil/frame.h
 @@ -35,6 +35,7 @@ enum AVFrameSideDataType {
  * The data is the AVPanScan struct defined in libavcodec.
  */
 AV_FRAME_DATA_PANSCAN,
 +AV_FRAME_DATA_CC,
 };
 
 typedef struct AVFrameSideData {

A doxy description of the associated data structure would be nice, so that 
calling applications know what to do with the side data ;-)

Tim
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel


[libav-devel] [PATCH] mpeg12dec: Extract CC user data into frame side data

2013-11-24 Thread John Stebbins
---
 libavcodec/avcodec.h   |  5 
 libavcodec/mpeg12dec.c | 64 ++
 libavutil/frame.h  |  4 
 3 files changed, 73 insertions(+)

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 4ce6d61..9e7d968 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -845,6 +845,11 @@ typedef struct AVPanScan{
 int16_t position[3][2];
 }AVPanScan;
 
+typedef struct AVClosedCaption {
+int count;
+uint8_t data[1];
+} AVClosedCaption;
+
 #if FF_API_QSCALE_TYPE
 #define FF_QSCALE_TYPE_MPEG1 0
 #define FF_QSCALE_TYPE_MPEG2 1
diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
index 0e11dda..19a91a1 100644
--- a/libavcodec/mpeg12dec.c
+++ b/libavcodec/mpeg12dec.c
@@ -44,6 +44,7 @@ typedef struct Mpeg1Context {
 int mpeg_enc_ctx_allocated; /* true if decoding context allocated */
 int repeat_field; /* true if we must repeat the field */
 AVPanScan pan_scan;  /** some temporary storage for the 
panscan */
+AVClosedCaption *caption;
 int slice_count;
 int save_aspect_info;
 int save_width, save_height, save_progressive_seq;
@@ -1529,6 +1530,15 @@ static int mpeg_field_start(MpegEncContext *s, const 
uint8_t *buf, int buf_size)
 return AVERROR(ENOMEM);
 memcpy(pan_scan-data, s1-pan_scan, sizeof(s1-pan_scan));
 
+if (s1-caption != NULL) {
+int size = sizeof(AVClosedCaption) + s1-caption-count * 3;
+AVFrameSideData *sd = av_frame_new_side_data(
+s-current_picture_ptr-f, AV_FRAME_DATA_CC, size);
+if (sd != NULL) {
+memcpy(sd-data, s1-caption, size);
+}
+av_freep(s1-caption);
+}
 if (HAVE_THREADS  (avctx-active_thread_type  FF_THREAD_FRAME))
 ff_thread_finish_setup(avctx);
 } else { // second field
@@ -2038,6 +2048,57 @@ static int vcr2_init_sequence(AVCodecContext *avctx)
 }
 
 
+static int mpeg_decode_cc(AVCodecContext *avctx, const uint8_t *p, int 
buf_size)
+{
+Mpeg1Context *s1 = avctx-priv_data;
+
+if (buf_size = 6 
+p[0] == 'G'  p[1] == 'A'  p[2] == '9'  p[3] == '4'  p[4] == 3) 
{
+/* extract A53 Part 4 CC data */
+int cc_count = p[5]  0x1f;
+if (cc_count  0  buf_size = 7 + cc_count * 3) {
+s1-caption = av_malloc(sizeof(AVClosedCaption) + cc_count * 3);
+if (s1-caption != NULL) {
+s1-caption-count = cc_count;
+memcpy(s1-caption-data, p + 7, cc_count * 3);
+}
+}
+return 1;
+} else if (buf_size = 11 
+p[0] == 'C'  p[1] == 'C'  p[2] == 0x01  p[3] == 0xf8) {
+/* extract DVD CC data */
+int cc_count = 0;
+int i;
+// There is a caption count field in the data, but it is often
+// incorect.  So count the number of captions present.
+for (i = 5; i + 6 = buf_size  ((p[i]  0xfe) == 0xfe); i += 6) {
+cc_count++;
+}
+// Transform the DVD format into A53 Part 4 format
+if (cc_count  0) {
+s1-caption = av_malloc(sizeof(AVClosedCaption) + cc_count * 6);
+if (s1-caption != NULL) {
+uint8_t field1 = !!(p[4]  0x80);
+uint8_t *cap = s1-caption-data;
+p += 5;
+s1-caption-count = cc_count * 2;
+for (i = 0; i  cc_count; i++) {
+cap[0] = (p[0] == 0xff  field1) ? 0xfc : 0xfd;
+cap[1] = p[1];
+cap[2] = p[2];
+cap[3] = (p[3] == 0xff  !field1) ? 0xfc : 0xfd;
+cap[4] = p[4];
+cap[5] = p[5];
+cap += 6;
+p += 6;
+}
+}
+}
+return 1;
+}
+return 0;
+}
+
 static void mpeg_decode_user_data(AVCodecContext *avctx,
   const uint8_t *p, int buf_size)
 {
@@ -2057,6 +2118,8 @@ static void mpeg_decode_user_data(AVCodecContext *avctx,
 return;
 avctx-dtg_active_format = p[0]  0x0f;
 }
+} else if (mpeg_decode_cc(avctx, p, buf_size)) {
+return;
 }
 }
 
@@ -2402,6 +2465,7 @@ static av_cold int mpeg_decode_end(AVCodecContext *avctx)
 
 if (s-mpeg_enc_ctx_allocated)
 ff_MPV_common_end(s-mpeg_enc_ctx);
+av_freep(s-caption);
 return 0;
 }
 
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 449a6b7..5d963ef 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -35,6 +35,10 @@ enum AVFrameSideDataType {
  * The data is the AVPanScan struct defined in libavcodec.
  */
 AV_FRAME_DATA_PANSCAN,
+/**
+ * CC data is in the AVClosedCaption struct defined in libavcodec.
+ */
+AV_FRAME_DATA_CC,
 };
 
 typedef struct AVFrameSideData {
-- 
1.8.3.1

___
libav-devel mailing list

Re: [libav-devel] [PATCH] mpeg12dec: Extract CC user data into frame side data

2013-11-24 Thread Kieran Kunhya
On 24 November 2013 17:49, John Stebbins stebb...@jetheaddev.com wrote:
 ---
  libavcodec/avcodec.h   |  5 
  libavcodec/mpeg12dec.c | 64 
 ++
  libavutil/frame.h  |  4 
  3 files changed, 73 insertions(+)

Superb! If you're interested in doing this for H264 I can make some
samples, otherwise I'll do it at some point.
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel


Re: [libav-devel] [PATCH] mpeg12dec: Extract CC user data into frame side data

2013-11-24 Thread John Stebbins

On 11/24/2013 06:26 PM, Kieran Kunhya wrote:

On 24 November 2013 17:49, John Stebbins stebb...@jetheaddev.com wrote:

---
  libavcodec/avcodec.h   |  5 
  libavcodec/mpeg12dec.c | 64 ++
  libavutil/frame.h  |  4 
  3 files changed, 73 insertions(+)


Superb! If you're interested in doing this for H264 I can make some
samples, otherwise I'll do it at some point.
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel



I'd be glad to add it if you can supply samples. Shouldn't be too hard.  Since 
I already have HandBrake wired up to process the CC data, it's probably a 
little easier for me to test it than it would be for you.

--
John  GnuPG fingerprint: D0EC B3DB C372 D1F1 0B01  83F0 49F1 D7B2 60D4 D0F7
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel


Re: [libav-devel] [PATCH] mpeg12dec: Extract CC user data into frame side data

2013-11-24 Thread Diego Biurrun
On Sun, Nov 24, 2013 at 09:49:47AM -0800, John Stebbins wrote:
 
 --- a/libavcodec/mpeg12dec.c
 +++ b/libavcodec/mpeg12dec.c
 @@ -1529,6 +1530,15 @@ static int mpeg_field_start(MpegEncContext *s, const 
 uint8_t *buf, int buf_size)
  
 +if (s1-caption != NULL) {

!s1-caption

 +if (sd != NULL) {
 +memcpy(sd-data, s1-caption, size);
 +}

same, drop {}

 @@ -2038,6 +2048,57 @@ static int vcr2_init_sequence(AVCodecContext *avctx)
  
 +if (buf_size = 6 
 +p[0] == 'G'  p[1] == 'A'  p[2] == '9'  p[3] == '4'  p[4] == 
 3) {
 +/* extract A53 Part 4 CC data */
 +int cc_count = p[5]  0x1f;
 +if (cc_count  0  buf_size = 7 + cc_count * 3) {
 +s1-caption = av_malloc(sizeof(AVClosedCaption) + cc_count * 3);
 +if (s1-caption != NULL) {

!s1-caption

 +for (i = 5; i + 6 = buf_size  ((p[i]  0xfe) == 0xfe); i += 6) {
 +cc_count++;
 +}

drop {}

 +// Transform the DVD format into A53 Part 4 format
 +if (cc_count  0) {
 +s1-caption = av_malloc(sizeof(AVClosedCaption) + cc_count * 6);
 +if (s1-caption != NULL) {

!s1-caption

 @@ -2057,6 +2118,8 @@ static void mpeg_decode_user_data(AVCodecContext *avctx,
  return;
  avctx-dtg_active_format = p[0]  0x0f;
  }
 +} else if (mpeg_decode_cc(avctx, p, buf_size)) {
 +return;
  }

You return different values above, but no error checking whatsoever is
performed here.  What was your intention?

Diego
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel


Re: [libav-devel] [PATCH] mpeg12dec: Extract CC user data into frame side data

2013-11-24 Thread Anton Khirnov

On Sun, 24 Nov 2013 09:49:47 -0800, John Stebbins stebb...@jetheaddev.com 
wrote:
 ---
  libavcodec/avcodec.h   |  5 
  libavcodec/mpeg12dec.c | 64 
 ++
  libavutil/frame.h  |  4 
  3 files changed, 73 insertions(+)
 
 diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
 index 4ce6d61..9e7d968 100644
 --- a/libavcodec/avcodec.h
 +++ b/libavcodec/avcodec.h
 @@ -845,6 +845,11 @@ typedef struct AVPanScan{
  int16_t position[3][2];
  }AVPanScan;
  
 +typedef struct AVClosedCaption {
 +int count;
 +uint8_t data[1];
 +} AVClosedCaption;

First, those two fields should be documented. Perhaps it's obvious to you what
they mean, but it's not so obvious to me. E.g. my first assumption would be that
count is the size of data in bytes, but looking at the code it is not so.

Second, this is apparrently a dump of some specific wire format, right? Then it
should be documented which one it is and the struct and the side data type
should have more specific names, so we can add other CC formats later.

-- 
Anton Khirnov
___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel


[libav-devel] [PATCH] mpeg12dec: Extract CC user data into frame side data

2013-11-23 Thread John Stebbins
---
 libavcodec/avcodec.h   |  5 +
 libavcodec/mpeg12dec.c | 55 ++
 libavutil/frame.h  |  1 +
 3 files changed, 61 insertions(+)

diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
index 4ce6d61..9e7d968 100644
--- a/libavcodec/avcodec.h
+++ b/libavcodec/avcodec.h
@@ -845,6 +845,11 @@ typedef struct AVPanScan{
 int16_t position[3][2];
 }AVPanScan;
 
+typedef struct AVClosedCaption {
+int count;
+uint8_t data[1];
+} AVClosedCaption;
+
 #if FF_API_QSCALE_TYPE
 #define FF_QSCALE_TYPE_MPEG1 0
 #define FF_QSCALE_TYPE_MPEG2 1
diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
index 0e11dda..5543caa 100644
--- a/libavcodec/mpeg12dec.c
+++ b/libavcodec/mpeg12dec.c
@@ -44,6 +44,7 @@ typedef struct Mpeg1Context {
 int mpeg_enc_ctx_allocated; /* true if decoding context allocated */
 int repeat_field; /* true if we must repeat the field */
 AVPanScan pan_scan;  /** some temporary storage for the 
panscan */
+AVClosedCaption *caption;
 int slice_count;
 int save_aspect_info;
 int save_width, save_height, save_progressive_seq;
@@ -1529,6 +1530,15 @@ static int mpeg_field_start(MpegEncContext *s, const 
uint8_t *buf, int buf_size)
 return AVERROR(ENOMEM);
 memcpy(pan_scan-data, s1-pan_scan, sizeof(s1-pan_scan));
 
+if (s1-caption != NULL) {
+int size = sizeof(AVClosedCaption) + s1-caption-count * 3;
+AVFrameSideData *sd = av_frame_new_side_data(
+s-current_picture_ptr-f, AV_FRAME_DATA_CC, size);
+if (sd != NULL) {
+memcpy(sd-data, s1-caption, size);
+}
+av_freep(s1-caption);
+}
 if (HAVE_THREADS  (avctx-active_thread_type  FF_THREAD_FRAME))
 ff_thread_finish_setup(avctx);
 } else { // second field
@@ -2041,6 +2051,7 @@ static int vcr2_init_sequence(AVCodecContext *avctx)
 static void mpeg_decode_user_data(AVCodecContext *avctx,
   const uint8_t *p, int buf_size)
 {
+Mpeg1Context *s1 = avctx-priv_data;
 const uint8_t *buf_end = p + buf_size;
 
 /* we parse the DTG active format information */
@@ -2058,6 +2069,49 @@ static void mpeg_decode_user_data(AVCodecContext *avctx,
 avctx-dtg_active_format = p[0]  0x0f;
 }
 }
+/* extract A53 Part 4 CC data */
+else if (buf_size = 6 
+p[0] == 'G'  p[1] == 'A'  p[2] == '9'  p[3] == '4'  p[4] == 3) 
{
+int cc_count = p[5]  0x1f;
+if (cc_count  0  buf_size = 7 + cc_count * 3) {
+s1-caption = av_malloc(sizeof(AVClosedCaption) + cc_count * 3);
+if (s1-caption != NULL) {
+s1-caption-count = cc_count;
+memcpy(s1-caption-data, p + 7, cc_count * 3);
+}
+}
+}
+/* extract DVD CC data */
+else if (buf_size = 11 
+p[0] == 'C'  p[1] == 'C'  p[2] == 0x01  p[3] == 0xf8) {
+int cc_count = 0;
+int i;
+// There is a caption count field in the data, but it is often
+// incorect.  So count the number of captions present.
+for (i = 5; i + 6 = buf_size  ((p[i]  0xfe) == 0xfe); i += 6) {
+cc_count++;
+}
+// Transform the DVD format into A53 Part 4 format
+if (cc_count  0) {
+s1-caption = av_malloc(sizeof(AVClosedCaption) + cc_count * 6);
+if (s1-caption != NULL) {
+uint8_t field1 = !!(p[4]  0x80);
+uint8_t *cap = s1-caption-data;
+p += 5;
+s1-caption-count = cc_count * 2;
+for (i = 0; i  cc_count; i++) {
+cap[0] = (p[0] == 0xff  field1) ? 0xfc : 0xfd;
+cap[1] = p[1];
+cap[2] = p[2];
+cap[3] = (p[3] == 0xff  !field1) ? 0xfc : 0xfd;
+cap[4] = p[4];
+cap[5] = p[5];
+cap += 6;
+p += 6;
+}
+}
+}
+}
 }
 
 static void mpeg_decode_gop(AVCodecContext *avctx,
@@ -2402,6 +2456,7 @@ static av_cold int mpeg_decode_end(AVCodecContext *avctx)
 
 if (s-mpeg_enc_ctx_allocated)
 ff_MPV_common_end(s-mpeg_enc_ctx);
+av_freep(s-caption);
 return 0;
 }
 
diff --git a/libavutil/frame.h b/libavutil/frame.h
index 449a6b7..2361233 100644
--- a/libavutil/frame.h
+++ b/libavutil/frame.h
@@ -35,6 +35,7 @@ enum AVFrameSideDataType {
  * The data is the AVPanScan struct defined in libavcodec.
  */
 AV_FRAME_DATA_PANSCAN,
+AV_FRAME_DATA_CC,
 };
 
 typedef struct AVFrameSideData {
-- 
1.8.3.1

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel


Re: [libav-devel] [PATCH] mpeg12dec: Extract CC user data into frame side data

2013-11-23 Thread Luca Barbato
On 24/11/13 00:11, John Stebbins wrote:
 ---
  libavcodec/avcodec.h   |  5 +
  libavcodec/mpeg12dec.c | 55 
 ++
  libavutil/frame.h  |  1 +
  3 files changed, 61 insertions(+)
 
 diff --git a/libavcodec/avcodec.h b/libavcodec/avcodec.h
 index 4ce6d61..9e7d968 100644
 --- a/libavcodec/avcodec.h
 +++ b/libavcodec/avcodec.h
 @@ -845,6 +845,11 @@ typedef struct AVPanScan{
  int16_t position[3][2];
  }AVPanScan;
  
 +typedef struct AVClosedCaption {
 +int count;
 +uint8_t data[1];
 +} AVClosedCaption;
 +
  #if FF_API_QSCALE_TYPE
  #define FF_QSCALE_TYPE_MPEG1 0
  #define FF_QSCALE_TYPE_MPEG2 1
 diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c
 index 0e11dda..5543caa 100644
 --- a/libavcodec/mpeg12dec.c
 +++ b/libavcodec/mpeg12dec.c
 @@ -44,6 +44,7 @@ typedef struct Mpeg1Context {
  int mpeg_enc_ctx_allocated; /* true if decoding context allocated */
  int repeat_field; /* true if we must repeat the field */
  AVPanScan pan_scan;  /** some temporary storage for the 
 panscan */
 +AVClosedCaption *caption;
  int slice_count;
  int save_aspect_info;
  int save_width, save_height, save_progressive_seq;
 @@ -1529,6 +1530,15 @@ static int mpeg_field_start(MpegEncContext *s, const 
 uint8_t *buf, int buf_size)
  return AVERROR(ENOMEM);
  memcpy(pan_scan-data, s1-pan_scan, sizeof(s1-pan_scan));
  
 +if (s1-caption != NULL) {
 +int size = sizeof(AVClosedCaption) + s1-caption-count * 3;
 +AVFrameSideData *sd = av_frame_new_side_data(
 +s-current_picture_ptr-f, AV_FRAME_DATA_CC, size);
 +if (sd != NULL) {
 +memcpy(sd-data, s1-caption, size);
 +}
 +av_freep(s1-caption);
 +}
  if (HAVE_THREADS  (avctx-active_thread_type  FF_THREAD_FRAME))
  ff_thread_finish_setup(avctx);
  } else { // second field
 @@ -2041,6 +2051,7 @@ static int vcr2_init_sequence(AVCodecContext *avctx)
  static void mpeg_decode_user_data(AVCodecContext *avctx,
const uint8_t *p, int buf_size)
  {
 +Mpeg1Context *s1 = avctx-priv_data;
  const uint8_t *buf_end = p + buf_size;
  
  /* we parse the DTG active format information */
 @@ -2058,6 +2069,49 @@ static void mpeg_decode_user_data(AVCodecContext 
 *avctx,
  avctx-dtg_active_format = p[0]  0x0f;
  }
  }

Could you make the following lines a function?

 +/* extract A53 Part 4 CC data */
 +else if (buf_size = 6 
 +p[0] == 'G'  p[1] == 'A'  p[2] == '9'  p[3] == '4'  p[4] == 
 3) {
 +int cc_count = p[5]  0x1f;
 +if (cc_count  0  buf_size = 7 + cc_count * 3) {
 +s1-caption = av_malloc(sizeof(AVClosedCaption) + cc_count * 3);
 +if (s1-caption != NULL) {
 +s1-caption-count = cc_count;
 +memcpy(s1-caption-data, p + 7, cc_count * 3);
 +}
 +}
 +}
 +/* extract DVD CC data */
 +else if (buf_size = 11 
 +p[0] == 'C'  p[1] == 'C'  p[2] == 0x01  p[3] == 0xf8) {
 +int cc_count = 0;
 +int i;
 +// There is a caption count field in the data, but it is often
 +// incorect.  So count the number of captions present.
  ^^ - r
 +for (i = 5; i + 6 = buf_size  ((p[i]  0xfe) == 0xfe); i += 6) {
 +cc_count++;
 +}
 +// Transform the DVD format into A53 Part 4 format
 +if (cc_count  0) {
 +s1-caption = av_malloc(sizeof(AVClosedCaption) + cc_count * 6);
 +if (s1-caption != NULL) {
 +uint8_t field1 = !!(p[4]  0x80);
 +uint8_t *cap = s1-caption-data;
 +p += 5;
 +s1-caption-count = cc_count * 2;
 +for (i = 0; i  cc_count; i++) {
 +cap[0] = (p[0] == 0xff  field1) ? 0xfc : 0xfd;
 +cap[1] = p[1];
 +cap[2] = p[2];
 +cap[3] = (p[3] == 0xff  !field1) ? 0xfc : 0xfd;
 +cap[4] = p[4];
 +cap[5] = p[5];
 +cap += 6;
 +p += 6;
 +}
 +}
 +}
 +}
  }
  

It doesn't look bad at all.

lu

___
libav-devel mailing list
libav-devel@libav.org
https://lists.libav.org/mailman/listinfo/libav-devel