[FFmpeg-devel,1/5] fftools/ffmpeg: Fix forward CPB props in to out

---
 fftools/ffmpeg.c | 2 ++
 1 file changed, 2 insertions(+)

On Mon, Dec 16, 2019 at 6:39 PM Nicolas Gaullier
<nicolas.gaullier@cji.paris> wrote:
>
> ---
>  fftools/ffmpeg.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index 27f68933f8..36c207653b 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -3562,12 +3562,14 @@ static int init_output_stream(OutputStream *ost, char *error, int error_len)
>              int i;
>              for (i = 0; i < ist->st->nb_side_data; i++) {
>                  AVPacketSideData *sd = &ist->st->side_data[i];
> +                if (sd->type != AV_PKT_DATA_CPB_PROPERTIES) {
>                  uint8_t *dst = av_stream_new_side_data(ost->st, sd->type, sd->size);
>                  if (!dst)
>                      return AVERROR(ENOMEM);
>                  memcpy(dst, sd->data, sd->size);
>                  if (ist->autorotate && sd->type == AV_PKT_DATA_DISPLAYMATRIX)
>                      av_display_rotation_set((uint32_t *)dst, 0);
> +                }
>              }
>          }
>

Can you clarify why you are exlcuding CBP side-data from being copied
here? It seems to not match the commit message in my mind.
Note that from some containers, namely mov/mp4, the container can
actually provide CBP data, which means the stream CBP data would be
valid and potentially relevant.

- Hendrik

>> @@ -3562,12 +3562,14 @@ static int init_output_stream(OutputStream *ost, char *error, int error_len)

>>              int i;

>>              for (i = 0; i < ist->st->nb_side_data; i++) {

>>                  AVPacketSideData *sd = &ist->st->side_data[i];

>> +                if (sd->type != AV_PKT_DATA_CPB_PROPERTIES) {

>>                  uint8_t *dst = av_stream_new_side_data(ost->st, sd->type, sd->size);

>>                  if (!dst)

>>                      return AVERROR(ENOMEM);

>>                  memcpy(dst, sd->data, sd->size);

>>                  if (ist->autorotate && sd->type == AV_PKT_DATA_DISPLAYMATRIX)

>>                      av_display_rotation_set((uint32_t *)dst, 0);

>> +                }

>>              }

>>          }

>>

>

>Can you clarify why you are exlcuding CBP side-data from being copied here? It seems to not match the commit message in my mind.

>Note that from some containers, namely mov/mp4, the container can actually provide CBP data, which means the stream CBP data would be valid and potentially relevant.


This portion of the code is for when the stream is transcoded, so there is no relation between the input and output CPB.
But in init_output_stream_streamcopy() you can see that all side data is actually copied (it was already there, before this patchset, and I left it untouched) with no exception (=incl. CPB). And this is what I was looking for: being able to forward the input CPB size to the output, when stream-copying.

Note I only modified the CPB behaviour to restrict the scope of this patchset, but there are certainly other side data that should not be forwarded from input to output; this does not sound easy for me and maybe it should be discussed, but this patchset is consistent as is and I think it would require other patches if we want something more generic about what side data should be forwarded or not.

This is my understanding and I may have missed something of course. I have mainly tested mpeg2 encoding and stream-copying and checked fate.

Nicolas

On Mon, Dec 16, 2019 at 10:19:29PM +0000, Gaullier Nicolas wrote:
> 
> >> @@ -3562,12 +3562,14 @@ static int init_output_stream(OutputStream *ost, char *error, int error_len)
> >>              int i;
> >>              for (i = 0; i < ist->st->nb_side_data; i++) {
> >>                  AVPacketSideData *sd = &ist->st->side_data[i];
> >> +                if (sd->type != AV_PKT_DATA_CPB_PROPERTIES) {
> >>                  uint8_t *dst = av_stream_new_side_data(ost->st, sd->type, sd->size);
> >>                  if (!dst)
> >>                      return AVERROR(ENOMEM);
> >>                  memcpy(dst, sd->data, sd->size);
> >>                  if (ist->autorotate && sd->type == AV_PKT_DATA_DISPLAYMATRIX)
> >>                      av_display_rotation_set((uint32_t *)dst, 0);
> >> +                }
> >>              }
> >>          }
> >>
> >
> >Can you clarify why you are exlcuding CBP side-data from being copied here? It seems to not match the commit message in my mind.
> >Note that from some containers, namely mov/mp4, the container can actually provide CBP data, which means the stream CBP data would be valid and potentially relevant.
> 
> This portion of the code is for when the stream is transcoded, so there is no relation between the input and output CPB.
> But in init_output_stream_streamcopy() you can see that all side data is actually copied (it was already there, before this patchset, and I left it untouched) with no exception (=incl. CPB). And this is what I was looking for: being able to forward the input CPB size to the output, when stream-copying.
> 
> Note I only modified the CPB behaviour to restrict the scope of this patchset, but there are certainly other side data that should not be forwarded from input to output; this does not sound easy for me and maybe it should be discussed, but this patchset is consistent as is and I think it would require other patches if we want something more generic about what side data should be forwarded or not.
> 
> This is my understanding and I may have missed something of course. I have mainly tested mpeg2 encoding and stream-copying and checked fate.

please add this information to the commit message

thx

[...]