[RFC] libavcodec/mpeg12dec.c: CC data from skipped frames
Hello all, In mpeg_decode_a53_cc() only the A/53 part 4 CC data ("GA94") is saved between frames. The other formats incorrectly create a larger buffer than they use since https://github.com/FFmpeg/FFmpeg/commit/a705bcd763e344fac191e157ffeddc285388... because they do not append to the previous data. A/53 and SCTE-20 specify a maximum of one CC user data per picture header. (I assume the same is true for DVD and DVB 0502.) The a53_buf_ref is added to the frame in mpeg_field_start() which will only be called in decode_chunks() if not all of the picture data slices are skipped. I have observed the A/53 code create a larger buffer at the start of a file or after skipping; presumably the I-frame following a GOP header is the first frame, then one or more B-frames have all of their slices skipped. Otherwise, old_size is always 0. Is a frame created when all the slices are skipped? (If so, the CC data should be added to that frame.) Should the CC data be preserved between the frames for all formats? Thanks, Scott Theisen
In mpeg_decode_a53_cc() only the A/53 part 4 CC data ("GA94") is saved between frames. The other formats incorrectly created a larger buffer than they use since a705bcd763e344fac191e157ffeddc285388b7fa because they did not append to the previous data. A/53 and SCTE-20 specify a maximum of one CC user data per picture header. (I assume the same is true for DVD and DVB 0502.) The a53_buf_ref is added to the frame in mpeg_field_start() which will only be called in decode_chunks() if not all of the picture data slices are skipped. I have observed the A/53 code create a larger buffer at the start of a file or after skipping; presumably the I-frame following a GOP header is the first frame, then one or more B-frames have all of their slices skipped. Otherwise, old_size is always 0. --- libavcodec/mpeg12dec.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c index 208ee28b04..65cb545448 100644 --- a/libavcodec/mpeg12dec.c +++ b/libavcodec/mpeg12dec.c @@ -1971,7 +1971,7 @@ static int mpeg_decode_a53_cc(AVCodecContext *avctx, ret = av_buffer_realloc(&s1->a53_buf_ref, new_size); if (ret >= 0) { uint8_t field, cc1, cc2; - uint8_t *cap = s1->a53_buf_ref->data; + uint8_t *cap = s1->a53_buf_ref->data + old_size; memset(s1->a53_buf_ref->data + old_size, 0, cc_count * 3); for (i = 0; i < cc_count && get_bits_left(&gb) >= 26; i++) { @@ -2043,7 +2043,7 @@ static int mpeg_decode_a53_cc(AVCodecContext *avctx, ret = av_buffer_realloc(&s1->a53_buf_ref, new_size); if (ret >= 0) { uint8_t field1 = !!(p[4] & 0x80); - uint8_t *cap = s1->a53_buf_ref->data; + uint8_t *cap = s1->a53_buf_ref->data + old_size; p += 5; for (i = 0; i < cc_count; i++) { cap[0] = (p[0] == 0xff && field1) ? 0xfc : 0xfd; @@ -2109,13 +2109,14 @@ static int mpeg_decode_a53_cc(AVCodecContext *avctx, ret = av_buffer_realloc(&s1->a53_buf_ref, new_size); if (ret >= 0) { - s1->a53_buf_ref->data[0] = cc_header; - s1->a53_buf_ref->data[1] = cc_data[0]; - s1->a53_buf_ref->data[2] = cc_data[1]; + uint8_t* data = s1->a53_buf_ref->data + old_size; + data[0] = cc_header; + data[1] = cc_data[0]; + data[2] = cc_data[1]; if (cc_count == 2) { - s1->a53_buf_ref->data[3] = cc_header; - s1->a53_buf_ref->data[4] = cc_data[2]; - s1->a53_buf_ref->data[5] = cc_data[3]; + data[3] = cc_header; + data[4] = cc_data[2]; + data[5] = cc_data[3]; } } -- 2.43.0
Hi Scott, Thanks for sharing this. I had read your original email also ([RFC] libavcodec/mpeg12dec.c: CC data from skipped frames) and was thinking about the statement. The patch helps visualize the issue a bit better. I need to study this and test it to understand it better. Meantime, I will apply locally and play around with it. If any more seasoned EIA608 experts have any thoughts please share.
On 12/13/24 17:39, Marth64 wrote:
Hi Scott,
Thanks for sharing this. I had read your original email also ([RFC] libavcodec/mpeg12dec.c: CC data from skipped frames) and was thinking about the statement. The patch helps visualize the issue a bit better.
I need to study this and test it to understand it better. Meantime, I will apply locally and play around with it.
If any more seasoned EIA608 experts have any thoughts please share.
The problem is the use of av_buffer_realloc() instead of av_buffer_alloc(). | old_size | realloc(new_size) | old_size + (undefined values) | (new_size total bytes) (new_size is always > old_size) realloc copies data already in the buffer to the first old_size bytes. The SCTE-20 code sets the extra space to 0, but the others leave it undefined. However, the old_size data is then overwritten, potentially partially, while the size of the array is still new_size when there are only new_size - old_size valid bytes. I think it is reasonable to concatenate the CC data until a frame can be exported. Since I don't know if there is a frame exported when all of the video frame's data slices have been skipped (e.g. B frame with open GOP), it seemed easiest to concatenate the CC data like the A/53 code does. Regards, Scott Theisen
This makes sense to me now. I think it makes more sense to follow the A53 approach and be consistent. May I suggest the following in the patch to also improve readability in this crowded area of code: - SCTE-20 section: as you are declaring `cap` to be `s1->a53_buf_ref->data + old_size` now, the memset immediately following the declaration can be shortened to `memset(cap, 0, cc_count * 3);` - DVB 0502 section: rename `uint8_t* data` to `uint8_t *cap` when you are setting up the final payload. This makes it more consistent with the other sections. In the meantime I will test locally and await other feedback. Thank you very much.
On 12/14/24 00:39, Marth64 wrote:
This makes sense to me now. I think it makes more sense to follow the A53 approach and be consistent.
May I suggest the following in the patch to also improve readability in this crowded area of code:
- SCTE-20 section: as you are declaring `cap` to be `s1->a53_buf_ref->data + old_size` now, the memset immediately following the declaration can be shortened to `memset(cap, 0, cc_count * 3);`
I did see that but was trying to have a minimal patch; however, that is reasonable to also change.
- DVB 0502 section: rename `uint8_t* data` to `uint8_t *cap` when you are setting up the final payload. This makes it more consistent with the other sections.
I had thought about consistency, but I really didn't like the name `cap`. However, I suppose it is short for `captions`, so I will also use `cap` for better consistency. Regards, Scott Theisen
Quoting Scott Theisen (2024-12-14 04:06:45)
On 12/13/24 17:39, Marth64 wrote:
Hi Scott,
Thanks for sharing this. I had read your original email also ([RFC] libavcodec/mpeg12dec.c: CC data from skipped frames) and was thinking about the statement. The patch helps visualize the issue a bit better.
I need to study this and test it to understand it better. Meantime, I will apply locally and play around with it.
If any more seasoned EIA608 experts have any thoughts please share.
The problem is the use of av_buffer_realloc() instead of av_buffer_alloc().
| old_size | realloc(new_size) | old_size + (undefined values) | (new_size total bytes) (new_size is always > old_size) realloc copies data already in the buffer to the first old_size bytes.
The SCTE-20 code sets the extra space to 0, but the others leave it undefined.
However, the old_size data is then overwritten, potentially partially, while the size of the array is still new_size when there are only new_size - old_size valid bytes.
I think it is reasonable to concatenate the CC data until a frame can be exported. Since I don't know if there is a frame exported when all of the video frame's data slices have been skipped (e.g. B frame with open GOP),
what exactly do you mean by 'skipped' here? -- Anton Khirnov
On 12/14/24 04:12, Anton Khirnov wrote:
Quoting Scott Theisen (2024-12-14 04:06:45)
On 12/13/24 17:39, Marth64 wrote:
Hi Scott,
Thanks for sharing this. I had read your original email also ([RFC] libavcodec/mpeg12dec.c: CC data from skipped frames) and was thinking about the statement. The patch helps visualize the issue a bit better.
I need to study this and test it to understand it better. Meantime, I will apply locally and play around with it.
If any more seasoned EIA608 experts have any thoughts please share. The problem is the use of av_buffer_realloc() instead of av_buffer_alloc().
| old_size | realloc(new_size) | old_size + (undefined values) | (new_size total bytes) (new_size is always > old_size) realloc copies data already in the buffer to the first old_size bytes.
The SCTE-20 code sets the extra space to 0, but the others leave it undefined.
However, the old_size data is then overwritten, potentially partially, while the size of the array is still new_size when there are only new_size - old_size valid bytes.
I think it is reasonable to concatenate the CC data until a frame can be exported. Since I don't know if there is a frame exported when all of the video frame's data slices have been skipped (e.g. B frame with open GOP), what exactly do you mean by 'skipped' here?
In mpeg12dec.c, decode_chunks() (see lines 2400 - 2560) will skip the data slices when certain conditions are met and only when none are met will decode_chunks() call mpeg_field_start(), which attaches a53_buf_ref to the frame. For example (line 2470), ``` if (!s2->last_pic.ptr) { /* Skip B-frames if we do not have reference frames and * GOP is not closed. */ if (s2->pict_type == AV_PICTURE_TYPE_B) { if (!s->closed_gop) { skip_frame = 1; av_log(s2->avctx, AV_LOG_DEBUG, "Skipping B slice due to open GOP\n"); break; } } } ``` This skips all of the data slices of one or more coded pictures of the coded B-frames immediately following the coded I-frame which follows the GOP header when skipping or at the beginning of the file for my ATSC samples. Since mpeg_field_start() is not called, a53_buf_ref is not NULL when the CC user data for the next coded picture is found. Also, as far as I can tell, skip_frame = 1 causes slice_end() to not be called, preventing a frame from being exported. Regards, Scott Theisen
In mpeg_decode_a53_cc() only the A/53 part 4 CC data ("GA94") is saved between frames. The other formats incorrectly created a larger buffer than they use since a705bcd763e344fac191e157ffeddc285388b7fa because they did not append to the previous data. A/53 and SCTE-20 specify a maximum of one CC user data per picture header. (I assume the same is true for DVD and DVB 0502.) The a53_buf_ref is added to the frame in mpeg_field_start() which will only be called in decode_chunks() if not all of the picture data slices are skipped. I have observed the A/53 code create a larger buffer at the start of a file or after skipping; presumably the I-frame following a GOP header is the first frame, then one or more B-frames have all of their slices skipped. Otherwise, old_size is always 0. --- libavcodec/mpeg12dec.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/libavcodec/mpeg12dec.c b/libavcodec/mpeg12dec.c index 208ee28b04..3ab626accd 100644 --- a/libavcodec/mpeg12dec.c +++ b/libavcodec/mpeg12dec.c @@ -1971,9 +1971,9 @@ static int mpeg_decode_a53_cc(AVCodecContext *avctx, ret = av_buffer_realloc(&s1->a53_buf_ref, new_size); if (ret >= 0) { uint8_t field, cc1, cc2; - uint8_t *cap = s1->a53_buf_ref->data; + uint8_t *cap = s1->a53_buf_ref->data + old_size; - memset(s1->a53_buf_ref->data + old_size, 0, cc_count * 3); + memset(cap, 0, cc_count * 3); for (i = 0; i < cc_count && get_bits_left(&gb) >= 26; i++) { skip_bits(&gb, 2); // priority field = get_bits(&gb, 2); @@ -2043,7 +2043,7 @@ static int mpeg_decode_a53_cc(AVCodecContext *avctx, ret = av_buffer_realloc(&s1->a53_buf_ref, new_size); if (ret >= 0) { uint8_t field1 = !!(p[4] & 0x80); - uint8_t *cap = s1->a53_buf_ref->data; + uint8_t *cap = s1->a53_buf_ref->data + old_size; p += 5; for (i = 0; i < cc_count; i++) { cap[0] = (p[0] == 0xff && field1) ? 0xfc : 0xfd; @@ -2109,13 +2109,14 @@ static int mpeg_decode_a53_cc(AVCodecContext *avctx, ret = av_buffer_realloc(&s1->a53_buf_ref, new_size); if (ret >= 0) { - s1->a53_buf_ref->data[0] = cc_header; - s1->a53_buf_ref->data[1] = cc_data[0]; - s1->a53_buf_ref->data[2] = cc_data[1]; + uint8_t* cap = s1->a53_buf_ref->data + old_size; + cap[0] = cc_header; + cap[1] = cc_data[0]; + cap[2] = cc_data[1]; if (cc_count == 2) { - s1->a53_buf_ref->data[3] = cc_header; - s1->a53_buf_ref->data[4] = cc_data[2]; - s1->a53_buf_ref->data[5] = cc_data[3]; + cap[3] = cc_header; + cap[4] = cc_data[2]; + cap[5] = cc_data[3]; } } -- 2.43.0
Hi, This makes sense to me still after much pondering. I would like to push next week if there are no objections. Thank you very much.
Will push soon
Pushed with minor formatting change (uint8_t *cap instead of uint8_t* cap)
participants (3)
-
Anton Khirnov -
Marth64 -
Scott Theisen