[FFmpeg-devel] mpeg12enc: Use all Closed Captions side data

Submitted by Mathieu Duponchelle on March 22, 2019, 11:22 p.m.

Details

Message ID 20190322232205.30420-1-mathieu@centricular.com
State New
Headers show

Commit Message

Mathieu Duponchelle March 22, 2019, 11:22 p.m.
It is perfectly valid to have multiple CC Picture User Data
for the same frame. Instead of using the first side_data
potentially present with the A53_CC type, iterate over all
side_data.
---
 libavcodec/mpeg12enc.c | 56 +++++++++++++++++++++++-------------------
 1 file changed, 31 insertions(+), 25 deletions(-)

Comments

Mathieu Duponchelle April 9, 2019, 11 p.m.
Anyone? :)

On 3/23/19 12:22 AM, Mathieu Duponchelle wrote:
> It is perfectly valid to have multiple CC Picture User Data
> for the same frame. Instead of using the first side_data
> potentially present with the A53_CC type, iterate over all
> side_data.
> ---
>  libavcodec/mpeg12enc.c | 56 +++++++++++++++++++++++-------------------
>  1 file changed, 31 insertions(+), 25 deletions(-)
>
> diff --git a/libavcodec/mpeg12enc.c b/libavcodec/mpeg12enc.c
> index 2bc5289d63..0162939399 100644
> --- a/libavcodec/mpeg12enc.c
> +++ b/libavcodec/mpeg12enc.c
> @@ -547,31 +547,37 @@ 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);
> +        int i;
> +
> +        for (i = 0; i < s->current_picture_ptr->f->nb_side_data; i++) {
> +            side_data = s->current_picture_ptr->f->side_data[i];
> +            if (side_data->type != AV_FRAME_DATA_A53_CC)
> +              continue;
> +
> +            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);
> +                }
>              }
>          }
>      }
Carl Eugen Hoyos April 9, 2019, 11:14 p.m.
2019-03-23 0:22 GMT+01:00, Mathieu Duponchelle <mathieu@centricular.com>:
> It is perfectly valid to have multiple CC Picture User Data
> for the same frame. Instead of using the first side_data
> potentially present with the A53_CC type, iterate over all
> side_data.

Please separate the functional changes in your patch from
the cosmetics, ie do not re-indent in the same patch to make
reviews easier.

Sorry for the late reply, Carl Eugen
Mathieu Duponchelle April 10, 2019, 11:26 a.m.
No problem, note that the added indentation is because a loop
was added :)

On 4/10/19 1:14 AM, Carl Eugen Hoyos wrote:
> 2019-03-23 0:22 GMT+01:00, Mathieu Duponchelle <mathieu@centricular.com>:
>> It is perfectly valid to have multiple CC Picture User Data
>> for the same frame. Instead of using the first side_data
>> potentially present with the A53_CC type, iterate over all
>> side_data.
> Please separate the functional changes in your patch from
> the cosmetics, ie do not re-indent in the same patch to make
> reviews easier.
>
> Sorry for the late reply, Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Mathieu Duponchelle April 19, 2019, 12:46 p.m.
Hello ? :)

On 4/10/19 1:26 PM, Mathieu Duponchelle wrote:
> No problem, note that the added indentation is because a loop
> was added :)
>
> On 4/10/19 1:14 AM, Carl Eugen Hoyos wrote:
>> 2019-03-23 0:22 GMT+01:00, Mathieu Duponchelle <mathieu@centricular.com>:
>>> It is perfectly valid to have multiple CC Picture User Data
>>> for the same frame. Instead of using the first side_data
>>> potentially present with the A53_CC type, iterate over all
>>> side_data.
>> Please separate the functional changes in your patch from
>> the cosmetics, ie do not re-indent in the same patch to make
>> reviews easier.
>>
>> Sorry for the late reply, Carl Eugen
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Mathieu Duponchelle May 13, 2019, 1:37 p.m.
Ping!

On 4/19/19 2:46 PM, Mathieu Duponchelle wrote:
> Hello ? :)
>
> On 4/10/19 1:26 PM, Mathieu Duponchelle wrote:
>> No problem, note that the added indentation is because a loop
>> was added :)
>>
>> On 4/10/19 1:14 AM, Carl Eugen Hoyos wrote:
>>> 2019-03-23 0:22 GMT+01:00, Mathieu Duponchelle <mathieu@centricular.com>:
>>>> It is perfectly valid to have multiple CC Picture User Data
>>>> for the same frame. Instead of using the first side_data
>>>> potentially present with the A53_CC type, iterate over all
>>>> side_data.
>>> Please separate the functional changes in your patch from
>>> the cosmetics, ie do not re-indent in the same patch to make
>>> reviews easier.
>>>
>>> Sorry for the late reply, Carl Eugen
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> To unsubscribe, visit link above, or email
>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Carl Eugen Hoyos May 13, 2019, 1:39 p.m.
Am Mi., 10. Apr. 2019 um 13:26 Uhr schrieb Mathieu Duponchelle
<mathieu@centricular.com>:

> No problem

I don't see an updated patch.

Carl Eugen
Paul B Mahol May 17, 2019, 8:44 a.m.
On 5/17/19, Mathieu Duponchelle <mathieu@centricular.com> wrote:
> There isn't one, as I said the added indentation is because of the new loop!

To get this committed to tree you need to comply to review requests.

>
> On 5/13/19 3:39 PM, Carl Eugen Hoyos wrote:
>> Am Mi., 10. Apr. 2019 um 13:26 Uhr schrieb Mathieu Duponchelle
>> <mathieu@centricular.com>:
>>
>>> No problem
>> I don't see an updated patch.
>>
>> Carl Eugen
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Devin Heitmueller May 19, 2019, 5 p.m.
Hello Paul,

On Fri, May 17, 2019 at 4:44 AM Paul B Mahol <onemda@gmail.com> wrote:
>
> On 5/17/19, Mathieu Duponchelle <mathieu@centricular.com> wrote:
> > There isn't one, as I said the added indentation is because of the new loop!
>
> To get this committed to tree you need to comply to review requests.

I think Mathieu's point is that the code indentation change was not
cosmetic - it's because the code in question is now inside a for loop,
and thus it needed to be indented another level.

Are you suggesting he should make a patch which results in the
indentation being wrong, and then submit a second patch which fixes
the incorrect indentation introduced by the first patch?

I'm confident that Mathieu is trying to comply with review requests so
he can get his code merged.  In this particular case, Carl raised his
concern about the indentation, Mathieu responded by suggesting that
given there was a functional change the re-indent was correct, and
then there was silence (i.e. neither agreement nor disagreement).

I'm also asking because I have work which I'm hoping to get upstreamed
as well at some point, and I'm sure I've got similar things in my
patches.  And having spent 20+ years reviewing patches on lots of
other open source projects, I wouldn't have thought twice about
accepting the patch as-is (given the change in indentation is a result
of a functional change and is not cosmetic).  In my experience, this
particular patch isn't an example of what maintainers mean when they
say "don't mix cosmetic whitespace changes with functional changes".

Regards,

Devin
Mathieu Duponchelle May 20, 2019, 7:22 a.m.
Hello,

Have you read the patch? The request is to split out cosmetic indentation
to a separate commit, however the added indentation is not cosmetic, but
simply due to a new inner loop ..

Best,
Mathieu

On 5/17/19 10:44 AM, Paul B Mahol wrote:
> On 5/17/19, Mathieu Duponchelle <mathieu@centricular.com> wrote:
>> There isn't one, as I said the added indentation is because of the new loop!
> To get this committed to tree you need to comply to review requests.
>
>> On 5/13/19 3:39 PM, Carl Eugen Hoyos wrote:
>>> Am Mi., 10. Apr. 2019 um 13:26 Uhr schrieb Mathieu Duponchelle
>>> <mathieu@centricular.com>:
>>>
>>>> No problem
>>> I don't see an updated patch.
>>>
>>> Carl Eugen
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> To unsubscribe, visit link above, or email
>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Mathieu Duponchelle May 20, 2019, 7:23 a.m.
Thanks :)

On 5/19/19 7:00 PM, Devin Heitmueller wrote:
> Hello Paul,
>
> On Fri, May 17, 2019 at 4:44 AM Paul B Mahol <onemda@gmail.com> wrote:
>> On 5/17/19, Mathieu Duponchelle <mathieu@centricular.com> wrote:
>>> There isn't one, as I said the added indentation is because of the new loop!
>> To get this committed to tree you need to comply to review requests.
> I think Mathieu's point is that the code indentation change was not
> cosmetic - it's because the code in question is now inside a for loop,
> and thus it needed to be indented another level.
>
> Are you suggesting he should make a patch which results in the
> indentation being wrong, and then submit a second patch which fixes
> the incorrect indentation introduced by the first patch?
>
> I'm confident that Mathieu is trying to comply with review requests so
> he can get his code merged.  In this particular case, Carl raised his
> concern about the indentation, Mathieu responded by suggesting that
> given there was a functional change the re-indent was correct, and
> then there was silence (i.e. neither agreement nor disagreement).
>
> I'm also asking because I have work which I'm hoping to get upstreamed
> as well at some point, and I'm sure I've got similar things in my
> patches.  And having spent 20+ years reviewing patches on lots of
> other open source projects, I wouldn't have thought twice about
> accepting the patch as-is (given the change in indentation is a result
> of a functional change and is not cosmetic).  In my experience, this
> particular patch isn't an example of what maintainers mean when they
> say "don't mix cosmetic whitespace changes with functional changes".
>
> Regards,
>
> Devin
>
Reimar Döffinger May 20, 2019, 7:44 a.m.
On 20.05.2019, at 09:23, Mathieu Duponchelle <mathieu@centricular.com> wrote:

> Thanks :)
> 
> On 5/19/19 7:00 PM, Devin Heitmueller wrote:
>> Hello Paul,
>> 
>> On Fri, May 17, 2019 at 4:44 AM Paul B Mahol <onemda@gmail.com> wrote:
>>> On 5/17/19, Mathieu Duponchelle <mathieu@centricular.com> wrote:
>>>> There isn't one, as I said the added indentation is because of the new loop!
>>> To get this committed to tree you need to comply to review requests.
>> I think Mathieu's point is that the code indentation change was not
>> cosmetic - it's because the code in question is now inside a for loop,
>> and thus it needed to be indented another level.
>> 
>> Are you suggesting he should make a patch which results in the
>> indentation being wrong, and then submit a second patch which fixes
>> the incorrect indentation introduced by the first patch?

I think it should probably be up to the maintainer, but possibly yes.
A lot of review still happens primarily by email, and if you re-indent code
it becomes impossible to see what, if anything, you changed in that block.
Enabling reviews of the patch via email means not re-indenting even if
it becomes "wrong".
Not everyone does reviews that way though, so some maintainers nowadays might prefer it differently?
Hendrik Leppkes May 20, 2019, 8:05 a.m.
On Mon, May 20, 2019 at 9:44 AM Reimar Döffinger
<Reimar.Doeffinger@gmx.de> wrote:
>
>
>
> On 20.05.2019, at 09:23, Mathieu Duponchelle <mathieu@centricular.com> wrote:
>
> > Thanks :)
> >
> > On 5/19/19 7:00 PM, Devin Heitmueller wrote:
> >> Hello Paul,
> >>
> >> On Fri, May 17, 2019 at 4:44 AM Paul B Mahol <onemda@gmail.com> wrote:
> >>> On 5/17/19, Mathieu Duponchelle <mathieu@centricular.com> wrote:
> >>>> There isn't one, as I said the added indentation is because of the new loop!
> >>> To get this committed to tree you need to comply to review requests.
> >> I think Mathieu's point is that the code indentation change was not
> >> cosmetic - it's because the code in question is now inside a for loop,
> >> and thus it needed to be indented another level.
> >>
> >> Are you suggesting he should make a patch which results in the
> >> indentation being wrong, and then submit a second patch which fixes
> >> the incorrect indentation introduced by the first patch?
>
> I think it should probably be up to the maintainer, but possibly yes.
> A lot of review still happens primarily by email, and if you re-indent code
> it becomes impossible to see what, if anything, you changed in that block.
> Enabling reviews of the patch via email means not re-indenting even if
> it becomes "wrong".
> Not everyone does reviews that way though, so some maintainers nowadays might prefer it differently?

My main concern with such requests is that its added effort to even
make these patches, since many developers will just automatically
indent where appropriate (as it should be), or even have tooling to do
it for them.
Personally, I would have to use another editor to "un-indent" the
code, since my main editor automatically corrects this stuff for me in
newly written code. As such, I'll personally never comply with such
requests, because I think its silly, and it would be extraordinary
effort to even do it, first restoring original indent with a second
editor.

In so many cases the "cosmetic" second commit was also quite simply
forgotten, resulting in an ugly codebase. Nevermind the added noise in
the commit log.

I don't think its that much to ask for a reviewer to also use a tool
that can ignore whitespace. Maybe we can teach patchwork to offer a
button to view diffs without whitespace, if people don't want to use a
simple tool.

- Hendrik
Reimar Döffinger May 20, 2019, 5 p.m.
On Mon, May 20, 2019 at 10:05:49AM +0200, Hendrik Leppkes wrote:
> On Mon, May 20, 2019 at 9:44 AM Reimar Döffinger
> <Reimar.Doeffinger@gmx.de> wrote:
> > On 20.05.2019, at 09:23, Mathieu Duponchelle <mathieu@centricular.com> wrote:
> > > On 5/19/19 7:00 PM, Devin Heitmueller wrote:
> > >> Are you suggesting he should make a patch which results in the
> > >> indentation being wrong, and then submit a second patch which fixes
> > >> the incorrect indentation introduced by the first patch?
> >
> > I think it should probably be up to the maintainer, but possibly yes.
> > A lot of review still happens primarily by email, and if you re-indent code
> > it becomes impossible to see what, if anything, you changed in that block.
> > Enabling reviews of the patch via email means not re-indenting even if
> > it becomes "wrong".
> > Not everyone does reviews that way though, so some maintainers nowadays might prefer it differently?
>
> My main concern with such requests is that its added effort to even
> make these patches, since many developers will just automatically
> indent where appropriate (as it should be), or even have tooling to do
> it for them.
> Personally, I would have to use another editor to "un-indent" the
> code, since my main editor automatically corrects this stuff for me in
> newly written code. As such, I'll personally never comply with such
> requests, because I think its silly, and it would be extraordinary
> effort to even do it, first restoring original indent with a second
> editor.

There's many ways to do it, but the reviewability issue is already
solved by e.g. sending the patch as generated by diff -w (have not
tried, but git send-email seems to accept that option), so I
think you are overstating the effort a bit :)
There is also the point that this should not be an issue normally:
- It doesn't matter if it's just a small block of code.
- If it's a large block of code, many times it means the code
should have been refactored long ago and put into a separate function.
So there often is the option of "volunteering" for some code cleanup
first :)

> I don't think its that much to ask for a reviewer to also use a tool
> that can ignore whitespace. Maybe we can teach patchwork to offer a
> button to view diffs without whitespace, if people don't want to use a
> simple tool.

FFmpeg used to have far too few reviewers.
If that is still the case, I think it's reasonable to put reviewer
convenience over submitter convenience.
I don't see how patchwork is really useful: finding the patch
there is already a pain, and we don't use it for reviews AT ALL,
we use email.
Now there might be an argument if we should have alternative/additional
review tools to email, but it's a bit of a separate question.
Personally, my suggestion would be that if you re-indented a lot
and unless you know the most likely potential reviewers do not care,
send a patch without whitespace changes at least in addition.
I mean, I don't have a super-strong opinion, but if I have to
dig through whitespace changes I'll probably just not review a
patch, because far too often all I have at hand to do them is
an email client...

Best regards,
Reimar Döffinger
Mathieu Duponchelle May 29, 2019, 1:44 p.m.
As suggested elsewhere, here's the output of git diff -w, I hope that helps :)

diff --git a/libavcodec/mpeg12enc.c b/libavcodec/mpeg12enc.c

index 2bc5289d63..0162939399 100644

--- a/libavcodec/mpeg12enc.c

+++ b/libavcodec/mpeg12enc.c

@@ -547,8 +547,13 @@ 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);

+        int i;

+

+        for (i = 0; i < s->current_picture_ptr->f->nb_side_data; i++) {

+            side_data = s->current_picture_ptr->f->side_data[i];

+            if (side_data->type != AV_FRAME_DATA_A53_CC)

+              continue;

+

             if (side_data) {

                 if (side_data->size <= A53_MAX_CC_COUNT * 3 && side_data->size % 3 == 0) {

                     int i = 0;

@@ -575,6 +580,7 @@ void ff_mpeg1_encode_picture_header(MpegEncContext *s, int picture_number)

                 }

             }

         }

+    }

 

     s->mb_y = 0;

     ff_mpeg1_encode_slice_header(s);


On 5/13/19 3:39 PM, Carl Eugen Hoyos wrote:
> Am Mi., 10. Apr. 2019 um 13:26 Uhr schrieb Mathieu Duponchelle
> <mathieu@centricular.com>:
>
>> No problem
> I don't see an updated patch.
>
> Carl Eugen
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Mathieu Duponchelle June 7, 2019, 12:05 a.m.
Sorry for pinging again, but at this point I don't know what more I can do to get this (necessary)
fix upstream :)

On 5/29/19 3:44 PM, Mathieu Duponchelle wrote:
> As suggested elsewhere, here's the output of git diff -w, I hope that helps :)
>
> diff --git a/libavcodec/mpeg12enc.c b/libavcodec/mpeg12enc.c
>
> index 2bc5289d63..0162939399 100644
>
> --- a/libavcodec/mpeg12enc.c
>
> +++ b/libavcodec/mpeg12enc.c
>
> @@ -547,8 +547,13 @@ 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);
>
> +        int i;
>
> +
>
> +        for (i = 0; i < s->current_picture_ptr->f->nb_side_data; i++) {
>
> +            side_data = s->current_picture_ptr->f->side_data[i];
>
> +            if (side_data->type != AV_FRAME_DATA_A53_CC)
>
> +              continue;
>
> +
>
>              if (side_data) {
>
>                  if (side_data->size <= A53_MAX_CC_COUNT * 3 && side_data->size % 3 == 0) {
>
>                      int i = 0;
>
> @@ -575,6 +580,7 @@ void ff_mpeg1_encode_picture_header(MpegEncContext *s, int picture_number)
>
>                  }
>
>              }
>
>          }
>
> +    }
>
>  
>
>      s->mb_y = 0;
>
>      ff_mpeg1_encode_slice_header(s);
>
>
> On 5/13/19 3:39 PM, Carl Eugen Hoyos wrote:
>> Am Mi., 10. Apr. 2019 um 13:26 Uhr schrieb Mathieu Duponchelle
>> <mathieu@centricular.com>:
>>
>>> No problem
>> I don't see an updated patch.
>>
>> Carl Eugen
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Patch hide | download patch | download mbox

diff --git a/libavcodec/mpeg12enc.c b/libavcodec/mpeg12enc.c
index 2bc5289d63..0162939399 100644
--- a/libavcodec/mpeg12enc.c
+++ b/libavcodec/mpeg12enc.c
@@ -547,31 +547,37 @@  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);
+        int i;
+
+        for (i = 0; i < s->current_picture_ptr->f->nb_side_data; i++) {
+            side_data = s->current_picture_ptr->f->side_data[i];
+            if (side_data->type != AV_FRAME_DATA_A53_CC)
+              continue;
+
+            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);
+                }
             }
         }
     }