| Message ID | 20200731112241.8948-6-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | 4656c1771b4ac12d9df9812390551f32fda181bc |
| Headers | show |
| Series | [FFmpeg-devel,1/8] avcodec/smacker: Remove write-only and unused variables | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On 7/31/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavcodec/smacker.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/libavcodec/smacker.c b/libavcodec/smacker.c > index 8a4d88cfed..4b1f0f1b7c 100644 > --- a/libavcodec/smacker.c > +++ b/libavcodec/smacker.c > @@ -251,17 +251,18 @@ static int smacker_decode_header_tree(SmackVContext > *smk, GetBitContext *gb, int > err = AVERROR(ENOMEM); > goto error; > } > + *recodes = huff.values; > > res = smacker_decode_bigtree(gb, &huff, &ctx, 0); > - if (res < 0) > + if (res < 0) { > err = res; > + goto error; > + } > skip_bits1(gb); > if(ctx.last[0] == -1) ctx.last[0] = huff.current++; > if(ctx.last[1] == -1) ctx.last[1] = huff.current++; > if(ctx.last[2] == -1) ctx.last[2] = huff.current++; > > - *recodes = huff.values; Commit log does not explain this change at all. And it looks wrong at first look. > - > error: > for (int i = 0; i < 2; i++) { > if (vlc[i].table) > -- > 2.20.1 > > _______________________________________________ > 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".
Paul B Mahol: > On 7/31/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavcodec/smacker.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/libavcodec/smacker.c b/libavcodec/smacker.c >> index 8a4d88cfed..4b1f0f1b7c 100644 >> --- a/libavcodec/smacker.c >> +++ b/libavcodec/smacker.c >> @@ -251,17 +251,18 @@ static int smacker_decode_header_tree(SmackVContext >> *smk, GetBitContext *gb, int >> err = AVERROR(ENOMEM); >> goto error; >> } >> + *recodes = huff.values; >> >> res = smacker_decode_bigtree(gb, &huff, &ctx, 0); >> - if (res < 0) >> + if (res < 0) { >> err = res; >> + goto error; >> + } >> skip_bits1(gb); >> if(ctx.last[0] == -1) ctx.last[0] = huff.current++; >> if(ctx.last[1] == -1) ctx.last[1] = huff.current++; >> if(ctx.last[2] == -1) ctx.last[2] = huff.current++; >> >> - *recodes = huff.values; > > Commit log does not explain this change at all. > And it looks wrong at first look. > huff.values is freshly allocated and if an error happens after its allocation, it either needs to be stored permanently to not go out of scope or it needs to be freed in this function. The latter option would lead to redundant code (one can just reuse the decoder's close function), so neither the old nor my proposed new code does this. The old code solves this problem by not jumping to error in case smacker_decode_bigtree() fails, my new code solves this problem by storing the array immediately after its allocation (before smacker_decode_bigtree()). (Another option would be to store the array after the error: label, but I think storing something as soon as possible is better practice). - Andreas >> - >> error: >> for (int i = 0; i < 2; i++) { >> if (vlc[i].table) >> -- >> 2.20.1 >>
On 8/19/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: > Paul B Mahol: >> On 7/31/20, Andreas Rheinhardt <andreas.rheinhardt@gmail.com> wrote: >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >>> --- >>> libavcodec/smacker.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/libavcodec/smacker.c b/libavcodec/smacker.c >>> index 8a4d88cfed..4b1f0f1b7c 100644 >>> --- a/libavcodec/smacker.c >>> +++ b/libavcodec/smacker.c >>> @@ -251,17 +251,18 @@ static int smacker_decode_header_tree(SmackVContext >>> *smk, GetBitContext *gb, int >>> err = AVERROR(ENOMEM); >>> goto error; >>> } >>> + *recodes = huff.values; >>> >>> res = smacker_decode_bigtree(gb, &huff, &ctx, 0); >>> - if (res < 0) >>> + if (res < 0) { >>> err = res; >>> + goto error; >>> + } >>> skip_bits1(gb); >>> if(ctx.last[0] == -1) ctx.last[0] = huff.current++; >>> if(ctx.last[1] == -1) ctx.last[1] = huff.current++; >>> if(ctx.last[2] == -1) ctx.last[2] = huff.current++; >>> >>> - *recodes = huff.values; >> >> Commit log does not explain this change at all. >> And it looks wrong at first look. >> > > huff.values is freshly allocated and if an error happens after its > allocation, it either needs to be stored permanently to not go out of > scope or it needs to be freed in this function. The latter option would > lead to redundant code (one can just reuse the decoder's close > function), so neither the old nor my proposed new code does this. The > old code solves this problem by not jumping to error in case > smacker_decode_bigtree() fails, my new code solves this problem by > storing the array immediately after its allocation (before > smacker_decode_bigtree()). (Another option would be to store the array > after the error: label, but I think storing something as soon as > possible is better practice). Add this explanation in commit log and patch is fine. > > - Andreas > >>> - >>> error: >>> for (int i = 0; i < 2; i++) { >>> if (vlc[i].table) >>> -- >>> 2.20.1 >>> > > _______________________________________________ > 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".
diff --git a/libavcodec/smacker.c b/libavcodec/smacker.c index 8a4d88cfed..4b1f0f1b7c 100644 --- a/libavcodec/smacker.c +++ b/libavcodec/smacker.c @@ -251,17 +251,18 @@ static int smacker_decode_header_tree(SmackVContext *smk, GetBitContext *gb, int err = AVERROR(ENOMEM); goto error; } + *recodes = huff.values; res = smacker_decode_bigtree(gb, &huff, &ctx, 0); - if (res < 0) + if (res < 0) { err = res; + goto error; + } skip_bits1(gb); if(ctx.last[0] == -1) ctx.last[0] = huff.current++; if(ctx.last[1] == -1) ctx.last[1] = huff.current++; if(ctx.last[2] == -1) ctx.last[2] = huff.current++; - *recodes = huff.values; - error: for (int i = 0; i < 2; i++) { if (vlc[i].table)
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavcodec/smacker.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)