diff mbox series

[FFmpeg-devel,01/18] mpeg4videodec: do not copy a range of fields at once

Message ID 20200313102850.23913-1-anton@khirnov.net
State Superseded
Headers show
Series [FFmpeg-devel,01/18] mpeg4videodec: do not copy a range of fields at once
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Anton Khirnov March 13, 2020, 10:28 a.m. UTC
This is extremely fragile against reordering and hides what is actually
being copied. Copy all the fields manually instead.
---
 libavcodec/mpeg4videodec.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

Comments

James Almer March 13, 2020, 3:07 p.m. UTC | #1
On 3/13/2020 7:28 AM, Anton Khirnov wrote:
> This is extremely fragile against reordering and hides what is actually
> being copied. Copy all the fields manually instead.
> ---
>  libavcodec/mpeg4videodec.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c
> index cc03486646..a51985d51b 100644
> --- a/libavcodec/mpeg4videodec.c
> +++ b/libavcodec/mpeg4videodec.c
> @@ -3460,7 +3460,32 @@ static int mpeg4_update_thread_context(AVCodecContext *dst,
>      if (ret < 0)
>          return ret;
>  
> -    memcpy(((uint8_t*)s) + sizeof(MpegEncContext), ((uint8_t*)s1) + sizeof(MpegEncContext), sizeof(Mpeg4DecContext) - sizeof(MpegEncContext));
> +    s->time_increment_bits       = s1->time_increment_bits;
> +    s->shape                     = s1->shape;
> +    s->vol_sprite_usage          = s1->vol_sprite_usage;
> +    s->sprite_brightness_change  = s1->sprite_brightness_change;
> +    s->num_sprite_warping_points = s1->num_sprite_warping_points;
> +    s->rvlc                      = s1->rvlc;
> +    s->resync_marker             = s1->resync_marker;
> +    s->t_frame                   = s1->t_frame;
> +    s->new_pred                  = s1->new_pred;
> +    s->enhancement_type          = s1->enhancement_type;
> +    s->scalability               = s1->scalability;
> +    s->use_intra_dc_vlc          = s1->use_intra_dc_vlc;
> +    s->intra_dc_threshold        = s1->intra_dc_threshold;
> +    s->divx_version              = s1->divx_version;
> +    s->divx_build                = s1->divx_build;
> +    s->xvid_build                = s1->xvid_build;
> +    s->lavc_build                = s1->lavc_build;
> +    s->showed_packed_warning     = s1->showed_packed_warning;
> +    s->vol_control_parameters    = s1->vol_control_parameters;
> +    s->cplx_estimation_trash_i   = s1->cplx_estimation_trash_i;
> +    s->cplx_estimation_trash_p   = s1->cplx_estimation_trash_p;
> +    s->cplx_estimation_trash_b   = s1->cplx_estimation_trash_b;
> +    s->rgb                       = s1->rgb;
> +
> +    memcpy(s->sprite_shift, s1->sprite_shift, sizeof(s1->sprite_shift));
> +    memcpy(s->sprite_traj,  s1->sprite_traj,  sizeof(s1->sprite_traj));

IMO, add a comment about this, in case someone unaware in the future
sees this block of code and feels clever after figuring it could be done
with a single memcpy().

>  
>      if (CONFIG_MPEG4_DECODER && !init && s1->xvid_build >= 0)
>          ff_xvid_idct_init(&s->m.idsp, dst);
>
Carl Eugen Hoyos March 14, 2020, 11:03 a.m. UTC | #2
Am Fr., 13. März 2020 um 11:30 Uhr schrieb Anton Khirnov <anton@khirnov.net>:
>
> This is extremely fragile against reordering and hides what is actually
> being copied. Copy all the fields manually instead.
> ---
>  libavcodec/mpeg4videodec.c | 27 ++++++++++++++++++++++++++-
>  1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c
> index cc03486646..a51985d51b 100644
> --- a/libavcodec/mpeg4videodec.c
> +++ b/libavcodec/mpeg4videodec.c
> @@ -3460,7 +3460,32 @@ static int mpeg4_update_thread_context(AVCodecContext *dst,
>      if (ret < 0)
>          return ret;
>
> -    memcpy(((uint8_t*)s) + sizeof(MpegEncContext), ((uint8_t*)s1) + sizeof(MpegEncContext), sizeof(Mpeg4DecContext) - sizeof(MpegEncContext));
> +    s->time_increment_bits       = s1->time_increment_bits;
> +    s->shape                     = s1->shape;
> +    s->vol_sprite_usage          = s1->vol_sprite_usage;
> +    s->sprite_brightness_change  = s1->sprite_brightness_change;
> +    s->num_sprite_warping_points = s1->num_sprite_warping_points;
> +    s->rvlc                      = s1->rvlc;
> +    s->resync_marker             = s1->resync_marker;
> +    s->t_frame                   = s1->t_frame;
> +    s->new_pred                  = s1->new_pred;
> +    s->enhancement_type          = s1->enhancement_type;
> +    s->scalability               = s1->scalability;
> +    s->use_intra_dc_vlc          = s1->use_intra_dc_vlc;
> +    s->intra_dc_threshold        = s1->intra_dc_threshold;
> +    s->divx_version              = s1->divx_version;
> +    s->divx_build                = s1->divx_build;
> +    s->xvid_build                = s1->xvid_build;
> +    s->lavc_build                = s1->lavc_build;
> +    s->showed_packed_warning     = s1->showed_packed_warning;
> +    s->vol_control_parameters    = s1->vol_control_parameters;
> +    s->cplx_estimation_trash_i   = s1->cplx_estimation_trash_i;
> +    s->cplx_estimation_trash_p   = s1->cplx_estimation_trash_p;
> +    s->cplx_estimation_trash_b   = s1->cplx_estimation_trash_b;
> +    s->rgb                       = s1->rgb;
> +
> +    memcpy(s->sprite_shift, s1->sprite_shift, sizeof(s1->sprite_shift));
> +    memcpy(s->sprite_traj,  s1->sprite_traj,  sizeof(s1->sprite_traj));

Am I really the only one who finds the code more "fragile" after this change?

Carl Eugen
Paul B Mahol March 14, 2020, 11:58 a.m. UTC | #3
On 3/14/20, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> Am Fr., 13. März 2020 um 11:30 Uhr schrieb Anton Khirnov
> <anton@khirnov.net>:
>>
>> This is extremely fragile against reordering and hides what is actually
>> being copied. Copy all the fields manually instead.
>> ---
>>  libavcodec/mpeg4videodec.c | 27 ++++++++++++++++++++++++++-
>>  1 file changed, 26 insertions(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c
>> index cc03486646..a51985d51b 100644
>> --- a/libavcodec/mpeg4videodec.c
>> +++ b/libavcodec/mpeg4videodec.c
>> @@ -3460,7 +3460,32 @@ static int
>> mpeg4_update_thread_context(AVCodecContext *dst,
>>      if (ret < 0)
>>          return ret;
>>
>> -    memcpy(((uint8_t*)s) + sizeof(MpegEncContext), ((uint8_t*)s1) +
>> sizeof(MpegEncContext), sizeof(Mpeg4DecContext) - sizeof(MpegEncContext));
>> +    s->time_increment_bits       = s1->time_increment_bits;
>> +    s->shape                     = s1->shape;
>> +    s->vol_sprite_usage          = s1->vol_sprite_usage;
>> +    s->sprite_brightness_change  = s1->sprite_brightness_change;
>> +    s->num_sprite_warping_points = s1->num_sprite_warping_points;
>> +    s->rvlc                      = s1->rvlc;
>> +    s->resync_marker             = s1->resync_marker;
>> +    s->t_frame                   = s1->t_frame;
>> +    s->new_pred                  = s1->new_pred;
>> +    s->enhancement_type          = s1->enhancement_type;
>> +    s->scalability               = s1->scalability;
>> +    s->use_intra_dc_vlc          = s1->use_intra_dc_vlc;
>> +    s->intra_dc_threshold        = s1->intra_dc_threshold;
>> +    s->divx_version              = s1->divx_version;
>> +    s->divx_build                = s1->divx_build;
>> +    s->xvid_build                = s1->xvid_build;
>> +    s->lavc_build                = s1->lavc_build;
>> +    s->showed_packed_warning     = s1->showed_packed_warning;
>> +    s->vol_control_parameters    = s1->vol_control_parameters;
>> +    s->cplx_estimation_trash_i   = s1->cplx_estimation_trash_i;
>> +    s->cplx_estimation_trash_p   = s1->cplx_estimation_trash_p;
>> +    s->cplx_estimation_trash_b   = s1->cplx_estimation_trash_b;
>> +    s->rgb                       = s1->rgb;
>> +
>> +    memcpy(s->sprite_shift, s1->sprite_shift, sizeof(s1->sprite_shift));
>> +    memcpy(s->sprite_traj,  s1->sprite_traj,  sizeof(s1->sprite_traj));
>
> Am I really the only one who finds the code more "fragile" after this
> change?

Elaborate how it is "fragile"?

>
> 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".
Carl Eugen Hoyos March 14, 2020, 12:42 p.m. UTC | #4
Am Sa., 14. März 2020 um 12:58 Uhr schrieb Paul B Mahol <onemda@gmail.com>:
>
> On 3/14/20, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> > Am Fr., 13. März 2020 um 11:30 Uhr schrieb Anton Khirnov
> > <anton@khirnov.net>:
> >>
> >> This is extremely fragile against reordering and hides what is actually
> >> being copied. Copy all the fields manually instead.
> >> ---
> >>  libavcodec/mpeg4videodec.c | 27 ++++++++++++++++++++++++++-
> >>  1 file changed, 26 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c
> >> index cc03486646..a51985d51b 100644
> >> --- a/libavcodec/mpeg4videodec.c
> >> +++ b/libavcodec/mpeg4videodec.c
> >> @@ -3460,7 +3460,32 @@ static int
> >> mpeg4_update_thread_context(AVCodecContext *dst,
> >>      if (ret < 0)
> >>          return ret;
> >>
> >> -    memcpy(((uint8_t*)s) + sizeof(MpegEncContext), ((uint8_t*)s1) +
> >> sizeof(MpegEncContext), sizeof(Mpeg4DecContext) - sizeof(MpegEncContext));
> >> +    s->time_increment_bits       = s1->time_increment_bits;
> >> +    s->shape                     = s1->shape;
> >> +    s->vol_sprite_usage          = s1->vol_sprite_usage;
> >> +    s->sprite_brightness_change  = s1->sprite_brightness_change;
> >> +    s->num_sprite_warping_points = s1->num_sprite_warping_points;
> >> +    s->rvlc                      = s1->rvlc;
> >> +    s->resync_marker             = s1->resync_marker;
> >> +    s->t_frame                   = s1->t_frame;
> >> +    s->new_pred                  = s1->new_pred;
> >> +    s->enhancement_type          = s1->enhancement_type;
> >> +    s->scalability               = s1->scalability;
> >> +    s->use_intra_dc_vlc          = s1->use_intra_dc_vlc;
> >> +    s->intra_dc_threshold        = s1->intra_dc_threshold;
> >> +    s->divx_version              = s1->divx_version;
> >> +    s->divx_build                = s1->divx_build;
> >> +    s->xvid_build                = s1->xvid_build;
> >> +    s->lavc_build                = s1->lavc_build;
> >> +    s->showed_packed_warning     = s1->showed_packed_warning;
> >> +    s->vol_control_parameters    = s1->vol_control_parameters;
> >> +    s->cplx_estimation_trash_i   = s1->cplx_estimation_trash_i;
> >> +    s->cplx_estimation_trash_p   = s1->cplx_estimation_trash_p;
> >> +    s->cplx_estimation_trash_b   = s1->cplx_estimation_trash_b;
> >> +    s->rgb                       = s1->rgb;
> >> +
> >> +    memcpy(s->sprite_shift, s1->sprite_shift, sizeof(s1->sprite_shift));
> >> +    memcpy(s->sprite_traj,  s1->sprite_traj,  sizeof(s1->sprite_traj));
> >
> > Am I really the only one who finds the code more "fragile" after this
> > change?
>
> Elaborate how it is "fragile"?

Because new fields might be forgotten.

Carl Eugen
Anton Khirnov March 15, 2020, 5:03 p.m. UTC | #5
Quoting Carl Eugen Hoyos (2020-03-14 13:42:33)
> Am Sa., 14. März 2020 um 12:58 Uhr schrieb Paul B Mahol <onemda@gmail.com>:
> >
> > On 3/14/20, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote:
> > > Am Fr., 13. März 2020 um 11:30 Uhr schrieb Anton Khirnov
> > > <anton@khirnov.net>:
> > >
> > > Am I really the only one who finds the code more "fragile" after this
> > > change?
> >
> > Elaborate how it is "fragile"?
> 
> Because new fields might be forgotten.

Not all fields are necessarily to be copied. Whether to copy any given
field is a decision that should be done explicitly when it is added.
diff mbox series

Patch

diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c
index cc03486646..a51985d51b 100644
--- a/libavcodec/mpeg4videodec.c
+++ b/libavcodec/mpeg4videodec.c
@@ -3460,7 +3460,32 @@  static int mpeg4_update_thread_context(AVCodecContext *dst,
     if (ret < 0)
         return ret;
 
-    memcpy(((uint8_t*)s) + sizeof(MpegEncContext), ((uint8_t*)s1) + sizeof(MpegEncContext), sizeof(Mpeg4DecContext) - sizeof(MpegEncContext));
+    s->time_increment_bits       = s1->time_increment_bits;
+    s->shape                     = s1->shape;
+    s->vol_sprite_usage          = s1->vol_sprite_usage;
+    s->sprite_brightness_change  = s1->sprite_brightness_change;
+    s->num_sprite_warping_points = s1->num_sprite_warping_points;
+    s->rvlc                      = s1->rvlc;
+    s->resync_marker             = s1->resync_marker;
+    s->t_frame                   = s1->t_frame;
+    s->new_pred                  = s1->new_pred;
+    s->enhancement_type          = s1->enhancement_type;
+    s->scalability               = s1->scalability;
+    s->use_intra_dc_vlc          = s1->use_intra_dc_vlc;
+    s->intra_dc_threshold        = s1->intra_dc_threshold;
+    s->divx_version              = s1->divx_version;
+    s->divx_build                = s1->divx_build;
+    s->xvid_build                = s1->xvid_build;
+    s->lavc_build                = s1->lavc_build;
+    s->showed_packed_warning     = s1->showed_packed_warning;
+    s->vol_control_parameters    = s1->vol_control_parameters;
+    s->cplx_estimation_trash_i   = s1->cplx_estimation_trash_i;
+    s->cplx_estimation_trash_p   = s1->cplx_estimation_trash_p;
+    s->cplx_estimation_trash_b   = s1->cplx_estimation_trash_b;
+    s->rgb                       = s1->rgb;
+
+    memcpy(s->sprite_shift, s1->sprite_shift, sizeof(s1->sprite_shift));
+    memcpy(s->sprite_traj,  s1->sprite_traj,  sizeof(s1->sprite_traj));
 
     if (CONFIG_MPEG4_DECODER && !init && s1->xvid_build >= 0)
         ff_xvid_idct_init(&s->m.idsp, dst);