| Message ID | CAB0OVGrKWT47ReRuesNN36+y-w+D5U8F45eAFVYoFhTVNeKpdw@mail.gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On 2018-01-18 23:34, Carl Eugen Hoyos wrote: > Hi! > > Attached patch fixes a warning, I suspect it makes the code more correct. > > Please comment, Carl Eugen > > --- a/libavcodec/svq3.c > +++ b/libavcodec/svq3.c > @@ -1048,12 +1048,12 @@ static int svq3_decode_slice_header(AVCodecContext *avctx) > } > memcpy(s->slice_buf, s->gb.buffer + s->gb.index / 8, slice_bytes); > > - init_get_bits(&s->gb_slice, s->slice_buf, slice_bits); Unrelated change? > if (s->watermark_key) { > - uint32_t header = AV_RL32(&s->gb_slice.buffer[1]); > - AV_WL32(&s->gb_slice.buffer[1], header ^ s->watermark_key); Strange that this didn't manage to break anything, or that the compiler let it through > + uint32_t header = AV_RL32(&s->slice_buf[1]); > + AV_WL32(&s->slice_buf[1], header ^ s->watermark_key); Considering the memcpy() above, either this or the old code must be wrong. My guess is the old code must have been wrong, since to fiddle the same bits this AV_WL32() would need to set &s->slice_buf[1 - s->gb.index / 8]... /Tomas
2018-01-19 16:54 GMT+01:00 Tomas Härdin <tjoppen@acc.umu.se>: > On 2018-01-18 23:34, Carl Eugen Hoyos wrote: >> >> Hi! >> >> Attached patch fixes a warning, I suspect it makes the code more correct. >> >> Please comment, Carl Eugen >> > >> --- a/libavcodec/svq3.c >> +++ b/libavcodec/svq3.c >> @@ -1048,12 +1048,12 @@ static int svq3_decode_slice_header(AVCodecContext >> *avctx) >> } >> memcpy(s->slice_buf, s->gb.buffer + s->gb.index / 8, >> slice_bytes); >> - init_get_bits(&s->gb_slice, s->slice_buf, slice_bits); > > Unrelated change? No, it is an intended move. >> if (s->watermark_key) { >> - uint32_t header = AV_RL32(&s->gb_slice.buffer[1]); >> - AV_WL32(&s->gb_slice.buffer[1], header ^ s->watermark_key); > > Strange that this didn't manage to break anything, or that the compiler > let it through A compiler's warning that gets fixed is the reason for this patch but I apparently misunderstand your comment. >> + uint32_t header = AV_RL32(&s->slice_buf[1]); >> + AV_WL32(&s->slice_buf[1], header ^ s->watermark_key); > > Considering the memcpy() above, either this or the old code must be wrong. Why do you think so? (I do consider the old code "wrong", that is why I sent this patch.) > My guess is the old code must have been wrong, since to fiddle > the same bits this AV_WL32() would need to set > &s->slice_buf[1 - s->gb.index / 8]... I was hoping that the same bits get fiddled given that I call the same macro / function... Carl Eugen
fre 2018-01-19 klockan 19:19 +0100 skrev Carl Eugen Hoyos: > 2018-01-19 16:54 GMT+01:00 Tomas Härdin <tjoppen@acc.umu.se>: > > On 2018-01-18 23:34, Carl Eugen Hoyos wrote: > > > > > > Hi! > > > > > > Attached patch fixes a warning, I suspect it makes the code more > > > correct. > > > > > > Please comment, Carl Eugen > > > > > > --- a/libavcodec/svq3.c > > > +++ b/libavcodec/svq3.c > > > @@ -1048,12 +1048,12 @@ static int > > > svq3_decode_slice_header(AVCodecContext > > > *avctx) > > > } > > > memcpy(s->slice_buf, s->gb.buffer + s->gb.index / 8, > > > slice_bytes); > > > - init_get_bits(&s->gb_slice, s->slice_buf, slice_bits); > > > > Unrelated change? > > No, it is an intended move. > > > > if (s->watermark_key) { > > > - uint32_t header = AV_RL32(&s->gb_slice.buffer[1]); > > > - AV_WL32(&s->gb_slice.buffer[1], header ^ s- > > > >watermark_key); > > > > Strange that this didn't manage to break anything, or that the > > compiler > > let it through > > A compiler's warning that gets fixed is the reason for this patch but > I apparently misunderstand your comment. > > > > + uint32_t header = AV_RL32(&s->slice_buf[1]); > > > + AV_WL32(&s->slice_buf[1], header ^ s- > > > >watermark_key); > > > > Considering the memcpy() above, either this or the old code must be > > wrong. > > Why do you think so? > (I do consider the old code "wrong", that is why I sent this patch.) > > > My guess is the old code must have been wrong, since to fiddle > > the same bits this AV_WL32() would need to set > > &s->slice_buf[1 - s->gb.index / 8]... > > I was hoping that the same bits get fiddled given that I call the > same macro / function... No I was confusing myself. Your patch looks fine. With these changes slice_buf is twiddled into the right state *before* init_get_bits(), which makes sense. /Tomas
2018-01-21 16:10 GMT+01:00 Tomas Härdin <tjoppen@acc.umu.se>: > fre 2018-01-19 klockan 19:19 +0100 skrev Carl Eugen Hoyos: >> 2018-01-19 16:54 GMT+01:00 Tomas Härdin <tjoppen@acc.umu.se>: >> > On 2018-01-18 23:34, Carl Eugen Hoyos wrote: >> > > >> > > Hi! >> > > >> > > Attached patch fixes a warning, I suspect it makes the code more >> > > correct. >> > > >> > > Please comment, Carl Eugen >> > > >> > > --- a/libavcodec/svq3.c >> > > +++ b/libavcodec/svq3.c >> > > @@ -1048,12 +1048,12 @@ static int >> > > svq3_decode_slice_header(AVCodecContext >> > > *avctx) >> > > } >> > > memcpy(s->slice_buf, s->gb.buffer + s->gb.index / 8, >> > > slice_bytes); >> > > - init_get_bits(&s->gb_slice, s->slice_buf, slice_bits); >> > >> > Unrelated change? >> >> No, it is an intended move. >> >> > > if (s->watermark_key) { >> > > - uint32_t header = AV_RL32(&s->gb_slice.buffer[1]); >> > > - AV_WL32(&s->gb_slice.buffer[1], header ^ s- >> > > >watermark_key); >> > >> > Strange that this didn't manage to break anything, or that the >> > compiler >> > let it through >> >> A compiler's warning that gets fixed is the reason for this patch but >> I apparently misunderstand your comment. >> >> > > + uint32_t header = AV_RL32(&s->slice_buf[1]); >> > > + AV_WL32(&s->slice_buf[1], header ^ s- >> > > >watermark_key); >> > >> > Considering the memcpy() above, either this or the old code must be >> > wrong. >> >> Why do you think so? >> (I do consider the old code "wrong", that is why I sent this patch.) >> >> > My guess is the old code must have been wrong, since to fiddle >> > the same bits this AV_WL32() would need to set >> > &s->slice_buf[1 - s->gb.index / 8]... >> >> I was hoping that the same bits get fiddled given that I call the >> same macro / function... > > No I was confusing myself. Your patch looks fine. With these changes > slice_buf is twiddled into the right state *before* init_get_bits(), > which makes sense. Patch applied, thank you! Carl Eugen
From 0460da037b91bbc4d0fbca3b756f5c7d345bbbbc Mon Sep 17 00:00:00 2001 From: Carl Eugen Hoyos <ceffmpeg@gmail.com> Date: Thu, 18 Jan 2018 23:31:48 +0100 Subject: [PATCH] lavc/svq3: Do not write into memory defined as const. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes a warning on ppc: libavcodec/svq3.c:1055:21: warning: passing argument 1 of ‘av_write_bswap32’ discards 'const' qualifier from pointer target type --- libavcodec/svq3.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libavcodec/svq3.c b/libavcodec/svq3.c index a937b2f..fc17081 100644 --- a/libavcodec/svq3.c +++ b/libavcodec/svq3.c @@ -1048,12 +1048,12 @@ static int svq3_decode_slice_header(AVCodecContext *avctx) } memcpy(s->slice_buf, s->gb.buffer + s->gb.index / 8, slice_bytes); - init_get_bits(&s->gb_slice, s->slice_buf, slice_bits); - if (s->watermark_key) { - uint32_t header = AV_RL32(&s->gb_slice.buffer[1]); - AV_WL32(&s->gb_slice.buffer[1], header ^ s->watermark_key); + uint32_t header = AV_RL32(&s->slice_buf[1]); + AV_WL32(&s->slice_buf[1], header ^ s->watermark_key); } + init_get_bits(&s->gb_slice, s->slice_buf, slice_bits); + if (length > 0) { memmove(s->slice_buf, &s->slice_buf[slice_length], length - 1); } -- 1.7.10.4