| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
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); >
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
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".
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
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 --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);