| Message ID | 20190208221012.29889-1-mathieu@centricular.com |
|---|---|
| State | Accepted |
| Commit | 6cfa17330317346d7ca34525003cad4d96ecf0cd |
| Headers | show |
So, should this go in? :) On 2/8/19 11:10 PM, Mathieu Duponchelle wrote: > --- > doc/encoders.texi | 3 +++ > libavcodec/mpeg12enc.c | 32 ++++++++++++++++++++++++++++++++ > libavcodec/mpegvideo.h | 2 ++ > 3 files changed, 37 insertions(+) > > diff --git a/doc/encoders.texi b/doc/encoders.texi > index e86ae69cc5..a283b9fddf 100644 > --- a/doc/encoders.texi > +++ b/doc/encoders.texi > @@ -2574,6 +2574,9 @@ Specifies the video_format written into the sequence display extension > indicating the source of the video pictures. The default is @samp{unspecified}, > can be @samp{component}, @samp{pal}, @samp{ntsc}, @samp{secam} or @samp{mac}. > For maximum compatibility, use @samp{component}. > +@item a53cc @var{boolean} > +Import closed captions (which must be ATSC compatible format) into output. > +Default is 1 (on). > @end table > > @section png > diff --git a/libavcodec/mpeg12enc.c b/libavcodec/mpeg12enc.c > index d0b458e34b..2bc5289d63 100644 > --- a/libavcodec/mpeg12enc.c > +++ b/libavcodec/mpeg12enc.c > @@ -61,6 +61,8 @@ static uint32_t mpeg1_chr_dc_uni[512]; > static uint8_t mpeg1_index_run[2][64]; > static int8_t mpeg1_max_level[2][64]; > > +#define A53_MAX_CC_COUNT 0x1f > + > static av_cold void init_uni_ac_vlc(RLTable *rl, uint8_t *uni_ac_vlc_len) > { > int i; > @@ -544,6 +546,36 @@ void ff_mpeg1_encode_picture_header(MpegEncContext *s, int picture_number) > } > } > > + if (s->codec_id == AV_CODEC_ID_MPEG2VIDEO && s->a53_cc) { > + side_data = av_frame_get_side_data(s->current_picture_ptr->f, > + AV_FRAME_DATA_A53_CC); > + if (side_data) { > + if (side_data->size <= A53_MAX_CC_COUNT * 3 && side_data->size % 3 == 0) { > + int i = 0; > + > + put_header (s, USER_START_CODE); > + > + put_bits(&s->pb, 8, 'G'); // user_identifier > + put_bits(&s->pb, 8, 'A'); > + put_bits(&s->pb, 8, '9'); > + put_bits(&s->pb, 8, '4'); > + put_bits(&s->pb, 8, 3); // user_data_type_code > + put_bits(&s->pb, 8, > + (side_data->size / 3 & A53_MAX_CC_COUNT) | 0x40); // flags, cc_count > + put_bits(&s->pb, 8, 0xff); // em_data > + > + for (i = 0; i < side_data->size; i++) > + put_bits(&s->pb, 8, side_data->data[i]); > + > + put_bits(&s->pb, 8, 0xff); // marker_bits > + } else { > + av_log(s->avctx, AV_LOG_WARNING, > + "Warning Closed Caption size (%d) can not exceed 93 bytes " > + "and must be a multiple of 3\n", side_data->size); > + } > + } > + } > + > s->mb_y = 0; > ff_mpeg1_encode_slice_header(s); > } > diff --git a/libavcodec/mpegvideo.h b/libavcodec/mpegvideo.h > index bbc6b5646a..e1ff5f97dc 100644 > --- a/libavcodec/mpegvideo.h > +++ b/libavcodec/mpegvideo.h > @@ -455,6 +455,7 @@ typedef struct MpegEncContext { > /* MPEG-2-specific - I wished not to have to support this mess. */ > int progressive_sequence; > int mpeg_f_code[2][2]; > + int a53_cc; > > // picture structure defines are loaded from mpegutils.h > int picture_structure; > @@ -663,6 +664,7 @@ FF_MPV_OPT_CMP_FUNC, \ > {"ps", "RTP payload size in bytes", FF_MPV_OFFSET(rtp_payload_size), AV_OPT_TYPE_INT, {.i64 = 0 }, INT_MIN, INT_MAX, FF_MPV_OPT_FLAGS }, \ > {"mepc", "Motion estimation bitrate penalty compensation (1.0 = 256)", FF_MPV_OFFSET(me_penalty_compensation), AV_OPT_TYPE_INT, {.i64 = 256 }, INT_MIN, INT_MAX, FF_MPV_OPT_FLAGS }, \ > {"mepre", "pre motion estimation", FF_MPV_OFFSET(me_pre), AV_OPT_TYPE_INT, {.i64 = 0 }, INT_MIN, INT_MAX, FF_MPV_OPT_FLAGS }, \ > +{"a53cc", "Use A53 Closed Captions (if available)", FF_MPV_OFFSET(a53_cc), AV_OPT_TYPE_BOOL, {.i64 = 1}, 0, 1, FF_MPV_OPT_FLAGS }, \ > > extern const AVOption ff_mpv_generic_options[]; >
On Wed, Feb 13, 2019 at 11:49:21PM +0100, Mathieu Duponchelle wrote:
> So, should this go in? :)
if someone checks it against some spec and or tests it against some
decoders, probably yes,
also might be a good idea to bump the micro version of libavcodec
thanks
[...]
On 2/14/19 10:01 PM, Michael Niedermayer wrote: > if someone checks it against some spec and or tests it against some > decoders, probably yes, The relevant part of the spec is section 6.2.3 in <http://www.atsc.org/wp-content/uploads/2015/03/a_53-Part-4-2009.pdf>, and I have indeed tested this patch against FFmpeg's mpeg2video decoder, it seems to work just fine :) Note that this commit is very similar in intent to e06114fed3afa69187b3dfc09a7a1a25cfd558b3 if that helps. > > also might be a good idea to bump the micro version of libavcodec I'm not familiar with this procedure I must admit :)
On Fri, Feb 15, 2019 at 11:17:38PM +0100, Mathieu Duponchelle wrote: > On 2/14/19 10:01 PM, Michael Niedermayer wrote: > > if someone checks it against some spec and or tests it against some > > decoders, probably yes, > > The relevant part of the spec is section 6.2.3 in > <http://www.atsc.org/wp-content/uploads/2015/03/a_53-Part-4-2009.pdf>, > and I have indeed tested this patch against FFmpeg's mpeg2video decoder, > it seems to work just fine :) It would be better to test against a decoder from a unrelated codebase Otherwise its a bit like testing your new language skills by talking with yourself. > > Note that this commit is very similar in intent to > e06114fed3afa69187b3dfc09a7a1a25cfd558b3 if that helps. > > > > > also might be a good idea to bump the micro version of libavcodec > > I'm not familiar with this procedure I must admit :) you add 1 to the value after LIBAVCODEC_VERSION_MICRO of course this could also be done by whoever commits this ... just saying someone should when this is pushed thx [...]
> It would be better to test against a decoder from a unrelated codebase > Otherwise its a bit like testing your new language skills by talking with > yourself. It should be pretty easy to just play the resulting TS in VLC and confirm the captions are present and play correctly. Devin
> It would be better to test against a decoder from a unrelated codebase > Otherwise its a bit like testing your new language skills by talking with > yourself. I have just tested with gstreamer's mpegvideoparse element, it detects the closed captions as expected: 0:00:00.602266814 27833 0x810400 DEBUG mpegvideoparse gstmpegvideoparse.c:507:parse_user_data_packet:<mpegvparse0> CEA-708 closed captions, 60 bytes
Ping? :) On 2/17/19 1:23 PM, Mathieu Duponchelle wrote: >> It would be better to test against a decoder from a unrelated codebase >> Otherwise its a bit like testing your new language skills by talking with >> yourself. > I have just tested with gstreamer's mpegvideoparse element, it detects the closed captions > as expected: > > 0:00:00.602266814 27833 0x810400 DEBUG mpegvideoparse gstmpegvideoparse.c:507:parse_user_data_packet:<mpegvparse0> CEA-708 closed captions, 60 bytes > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Hi I just finished a new codec for h.264, how to send patches to review ? This is my first piece of code for ffmpeg. //////////////////////////////////////////////////////////////////////// When you submit your patch, please use git format-patch or git send-email. ///////////////////////////////////////////////////////////////////////// I did "git format-patch", but where is the patch? My git said " git: 'send-email' is not a git command. See 'git --help'." Please help. Thanks. Yufei.
Hello, is there anything preventing from merging this patch? On 2/14/19 10:01 PM, Michael Niedermayer wrote: > On Wed, Feb 13, 2019 at 11:49:21PM +0100, Mathieu Duponchelle wrote: >> So, should this go in? :) > if someone checks it against some spec and or tests it against some > decoders, probably yes, > also might be a good idea to bump the micro version of libavcodec > > thanks > > [...] > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Thu, Mar 14, 2019 at 09:29:49PM +0100, Mathieu Duponchelle wrote:
> Hello, is there anything preventing from merging this patch?
no
will apply
thx
[...]
Yay, thanks! On 3/16/19 12:04 AM, Michael Niedermayer wrote: > no > will apply > > thx > > [...] > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
diff --git a/doc/encoders.texi b/doc/encoders.texi index e86ae69cc5..a283b9fddf 100644 --- a/doc/encoders.texi +++ b/doc/encoders.texi @@ -2574,6 +2574,9 @@ Specifies the video_format written into the sequence display extension indicating the source of the video pictures. The default is @samp{unspecified}, can be @samp{component}, @samp{pal}, @samp{ntsc}, @samp{secam} or @samp{mac}. For maximum compatibility, use @samp{component}. +@item a53cc @var{boolean} +Import closed captions (which must be ATSC compatible format) into output. +Default is 1 (on). @end table @section png diff --git a/libavcodec/mpeg12enc.c b/libavcodec/mpeg12enc.c index d0b458e34b..2bc5289d63 100644 --- a/libavcodec/mpeg12enc.c +++ b/libavcodec/mpeg12enc.c @@ -61,6 +61,8 @@ static uint32_t mpeg1_chr_dc_uni[512]; static uint8_t mpeg1_index_run[2][64]; static int8_t mpeg1_max_level[2][64]; +#define A53_MAX_CC_COUNT 0x1f + static av_cold void init_uni_ac_vlc(RLTable *rl, uint8_t *uni_ac_vlc_len) { int i; @@ -544,6 +546,36 @@ void ff_mpeg1_encode_picture_header(MpegEncContext *s, int picture_number) } } + if (s->codec_id == AV_CODEC_ID_MPEG2VIDEO && s->a53_cc) { + side_data = av_frame_get_side_data(s->current_picture_ptr->f, + AV_FRAME_DATA_A53_CC); + if (side_data) { + if (side_data->size <= A53_MAX_CC_COUNT * 3 && side_data->size % 3 == 0) { + int i = 0; + + put_header (s, USER_START_CODE); + + put_bits(&s->pb, 8, 'G'); // user_identifier + put_bits(&s->pb, 8, 'A'); + put_bits(&s->pb, 8, '9'); + put_bits(&s->pb, 8, '4'); + put_bits(&s->pb, 8, 3); // user_data_type_code + put_bits(&s->pb, 8, + (side_data->size / 3 & A53_MAX_CC_COUNT) | 0x40); // flags, cc_count + put_bits(&s->pb, 8, 0xff); // em_data + + for (i = 0; i < side_data->size; i++) + put_bits(&s->pb, 8, side_data->data[i]); + + put_bits(&s->pb, 8, 0xff); // marker_bits + } else { + av_log(s->avctx, AV_LOG_WARNING, + "Warning Closed Caption size (%d) can not exceed 93 bytes " + "and must be a multiple of 3\n", side_data->size); + } + } + } + s->mb_y = 0; ff_mpeg1_encode_slice_header(s); } diff --git a/libavcodec/mpegvideo.h b/libavcodec/mpegvideo.h index bbc6b5646a..e1ff5f97dc 100644 --- a/libavcodec/mpegvideo.h +++ b/libavcodec/mpegvideo.h @@ -455,6 +455,7 @@ typedef struct MpegEncContext { /* MPEG-2-specific - I wished not to have to support this mess. */ int progressive_sequence; int mpeg_f_code[2][2]; + int a53_cc; // picture structure defines are loaded from mpegutils.h int picture_structure; @@ -663,6 +664,7 @@ FF_MPV_OPT_CMP_FUNC, \ {"ps", "RTP payload size in bytes", FF_MPV_OFFSET(rtp_payload_size), AV_OPT_TYPE_INT, {.i64 = 0 }, INT_MIN, INT_MAX, FF_MPV_OPT_FLAGS }, \ {"mepc", "Motion estimation bitrate penalty compensation (1.0 = 256)", FF_MPV_OFFSET(me_penalty_compensation), AV_OPT_TYPE_INT, {.i64 = 256 }, INT_MIN, INT_MAX, FF_MPV_OPT_FLAGS }, \ {"mepre", "pre motion estimation", FF_MPV_OFFSET(me_pre), AV_OPT_TYPE_INT, {.i64 = 0 }, INT_MIN, INT_MAX, FF_MPV_OPT_FLAGS }, \ +{"a53cc", "Use A53 Closed Captions (if available)", FF_MPV_OFFSET(a53_cc), AV_OPT_TYPE_BOOL, {.i64 = 1}, 0, 1, FF_MPV_OPT_FLAGS }, \ extern const AVOption ff_mpv_generic_options[];