| Message ID | CAB0OVGpvbr1SFfSChHbMc+NJoDk3vmnE_OtVOq+ojpzcKitF5Q@mail.gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
On Mon, 17 Dec 2018 at 01:47, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > Hi! > > The Opus struct RawBitsContext is used in both the decoder and the encoder. > The fact that *position is const avoids warnings in the decoder where > it points into the bitstream. The encoder writes into the same > pointer, attached cast silences the warning on targets where AV_WB32() > does not internally cast the qualifier away. > > It is also possible to use a union if anybody prefers this: > diff --git a/libavcodec/opus_rc.h b/libavcodec/opus_rc.h > index 627f832..baad4ce 100644 > --- a/libavcodec/opus_rc.h > +++ b/libavcodec/opus_rc.h > @@ -37,9 +37,19 @@ typedef struct RawBitsContext { > uint32_t cacheval; > } RawBitsContext; > > +typedef struct RawBitsEncContext { > + uint8_t *position; > + uint32_t bytes; > + uint32_t cachelen; > + uint32_t cacheval; > +} RawBitsEncContext; > + > typedef struct OpusRangeCoder { > GetBitContext gb; > - RawBitsContext rb; > + union { > + RawBitsContext rb; > + RawBitsEncContext rbe; > + }; > uint32_t range; > uint32_t value; > uint32_t total_bits; > > and use rbe in ff_opus_rc_put_raw(). > > Please comment, Carl Eugen > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel NAK, we don't do anyonymous unions. To silence the warning the const can be just removed, but I've never seen a single warning on platforms I've tried.
2018-12-17 21:35 GMT+01:00, Rostislav Pehlivanov <atomnuker@gmail.com>: > On Mon, 17 Dec 2018 at 01:47, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > >> Hi! >> >> The Opus struct RawBitsContext is used in both the decoder and the >> encoder. >> The fact that *position is const avoids warnings in the decoder where >> it points into the bitstream. The encoder writes into the same >> pointer, attached cast silences the warning on targets where AV_WB32() >> does not internally cast the qualifier away. >> >> It is also possible to use a union if anybody prefers this: >> diff --git a/libavcodec/opus_rc.h b/libavcodec/opus_rc.h >> index 627f832..baad4ce 100644 >> --- a/libavcodec/opus_rc.h >> +++ b/libavcodec/opus_rc.h >> @@ -37,9 +37,19 @@ typedef struct RawBitsContext { >> uint32_t cacheval; >> } RawBitsContext; >> >> +typedef struct RawBitsEncContext { >> + uint8_t *position; >> + uint32_t bytes; >> + uint32_t cachelen; >> + uint32_t cacheval; >> +} RawBitsEncContext; >> + >> typedef struct OpusRangeCoder { >> GetBitContext gb; >> - RawBitsContext rb; >> + union { >> + RawBitsContext rb; >> + RawBitsEncContext rbe; >> + }; >> uint32_t range; >> uint32_t value; >> uint32_t total_bits; >> >> and use rbe in ff_opus_rc_put_raw(). > NAK, we don't do anyonymous unions. The suggested patch does not contain unions. > To silence the warning the const can be > just removed, No, the original data comes from a const pointer. > but I've never seen a single warning on platforms I've tried. It is shown on platforms where an optimized function (instead of the default macro) for AV_RW32() exists, for example Android arm. Carl Eugen
2018-12-17 21:53 GMT+01:00, Carl Eugen Hoyos <ceffmpeg@gmail.com>: > 2018-12-17 21:35 GMT+01:00, Rostislav Pehlivanov <atomnuker@gmail.com>: >> On Mon, 17 Dec 2018 at 01:47, Carl Eugen Hoyos <ceffmpeg@gmail.com> >> wrote: >> >>> Hi! >>> >>> The Opus struct RawBitsContext is used in both the decoder and the >>> encoder. >>> The fact that *position is const avoids warnings in the decoder where >>> it points into the bitstream. The encoder writes into the same >>> pointer, attached cast silences the warning on targets where AV_WB32() >>> does not internally cast the qualifier away. >>> >>> It is also possible to use a union if anybody prefers this: >>> diff --git a/libavcodec/opus_rc.h b/libavcodec/opus_rc.h >>> index 627f832..baad4ce 100644 >>> --- a/libavcodec/opus_rc.h >>> +++ b/libavcodec/opus_rc.h >>> @@ -37,9 +37,19 @@ typedef struct RawBitsContext { >>> uint32_t cacheval; >>> } RawBitsContext; >>> >>> +typedef struct RawBitsEncContext { >>> + uint8_t *position; >>> + uint32_t bytes; >>> + uint32_t cachelen; >>> + uint32_t cacheval; >>> +} RawBitsEncContext; >>> + >>> typedef struct OpusRangeCoder { >>> GetBitContext gb; >>> - RawBitsContext rb; >>> + union { >>> + RawBitsContext rb; >>> + RawBitsEncContext rbe; >>> + }; >>> uint32_t range; >>> uint32_t value; >>> uint32_t total_bits; >>> >>> and use rbe in ff_opus_rc_put_raw(). > >> NAK, we don't do anyonymous unions. > > The suggested patch does not contain unions. > >> To silence the warning the const can be >> just removed, > > No, the original data comes from a const pointer. ...when decoding. >> but I've never seen a single warning on platforms I've tried. > > It is shown on platforms where an optimized function (instead > of the default macro) for AV_RW32() exists, for example Android arm. Carl Eugen
On 12/17/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2018-12-17 21:35 GMT+01:00, Rostislav Pehlivanov <atomnuker@gmail.com>: >> On Mon, 17 Dec 2018 at 01:47, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> >>> Hi! >>> >>> The Opus struct RawBitsContext is used in both the decoder and the >>> encoder. >>> The fact that *position is const avoids warnings in the decoder where >>> it points into the bitstream. The encoder writes into the same >>> pointer, attached cast silences the warning on targets where AV_WB32() >>> does not internally cast the qualifier away. >>> >>> It is also possible to use a union if anybody prefers this: >>> diff --git a/libavcodec/opus_rc.h b/libavcodec/opus_rc.h >>> index 627f832..baad4ce 100644 >>> --- a/libavcodec/opus_rc.h >>> +++ b/libavcodec/opus_rc.h >>> @@ -37,9 +37,19 @@ typedef struct RawBitsContext { >>> uint32_t cacheval; >>> } RawBitsContext; >>> >>> +typedef struct RawBitsEncContext { >>> + uint8_t *position; >>> + uint32_t bytes; >>> + uint32_t cachelen; >>> + uint32_t cacheval; >>> +} RawBitsEncContext; >>> + >>> typedef struct OpusRangeCoder { >>> GetBitContext gb; >>> - RawBitsContext rb; >>> + union { >>> + RawBitsContext rb; >>> + RawBitsEncContext rbe; >>> + }; >>> uint32_t range; >>> uint32_t value; >>> uint32_t total_bits; >>> >>> and use rbe in ff_opus_rc_put_raw(). > >> NAK, we don't do anyonymous unions. > > The suggested patch does not contain unions. > It does, Are you author of this patch?
2018-12-17 22:02 GMT+01:00, Paul B Mahol <onemda@gmail.com>: > On 12/17/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> 2018-12-17 21:35 GMT+01:00, Rostislav Pehlivanov <atomnuker@gmail.com>: >>> On Mon, 17 Dec 2018 at 01:47, Carl Eugen Hoyos <ceffmpeg@gmail.com> >>> wrote: >>> >>>> Hi! >>>> >>>> The Opus struct RawBitsContext is used in both the decoder and the >>>> encoder. >>>> The fact that *position is const avoids warnings in the decoder where >>>> it points into the bitstream. The encoder writes into the same >>>> pointer, attached cast silences the warning on targets where AV_WB32() >>>> does not internally cast the qualifier away. >>>> >>>> It is also possible to use a union if anybody prefers this: >>>> diff --git a/libavcodec/opus_rc.h b/libavcodec/opus_rc.h >>>> index 627f832..baad4ce 100644 >>>> --- a/libavcodec/opus_rc.h >>>> +++ b/libavcodec/opus_rc.h >>>> @@ -37,9 +37,19 @@ typedef struct RawBitsContext { >>>> uint32_t cacheval; >>>> } RawBitsContext; >>>> >>>> +typedef struct RawBitsEncContext { >>>> + uint8_t *position; >>>> + uint32_t bytes; >>>> + uint32_t cachelen; >>>> + uint32_t cacheval; >>>> +} RawBitsEncContext; >>>> + >>>> typedef struct OpusRangeCoder { >>>> GetBitContext gb; >>>> - RawBitsContext rb; >>>> + union { >>>> + RawBitsContext rb; >>>> + RawBitsEncContext rbe; >>>> + }; >>>> uint32_t range; >>>> uint32_t value; >>>> uint32_t total_bits; >>>> >>>> and use rbe in ff_opus_rc_put_raw(). >> >>> NAK, we don't do anyonymous unions. >> >> The suggested patch does not contain unions. > > It does, Are you author of this patch? This is a poc to show that there is a possible (although ugly) alternative to the - imo - unavoidable cast that I attached as a git-formatted patch. Carl Eugen
On 12/17/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2018-12-17 22:02 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >> On 12/17/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>> 2018-12-17 21:35 GMT+01:00, Rostislav Pehlivanov <atomnuker@gmail.com>: >>>> On Mon, 17 Dec 2018 at 01:47, Carl Eugen Hoyos <ceffmpeg@gmail.com> >>>> wrote: >>>> >>>>> Hi! >>>>> >>>>> The Opus struct RawBitsContext is used in both the decoder and the >>>>> encoder. >>>>> The fact that *position is const avoids warnings in the decoder where >>>>> it points into the bitstream. The encoder writes into the same >>>>> pointer, attached cast silences the warning on targets where AV_WB32() >>>>> does not internally cast the qualifier away. >>>>> >>>>> It is also possible to use a union if anybody prefers this: >>>>> diff --git a/libavcodec/opus_rc.h b/libavcodec/opus_rc.h >>>>> index 627f832..baad4ce 100644 >>>>> --- a/libavcodec/opus_rc.h >>>>> +++ b/libavcodec/opus_rc.h >>>>> @@ -37,9 +37,19 @@ typedef struct RawBitsContext { >>>>> uint32_t cacheval; >>>>> } RawBitsContext; >>>>> >>>>> +typedef struct RawBitsEncContext { >>>>> + uint8_t *position; >>>>> + uint32_t bytes; >>>>> + uint32_t cachelen; >>>>> + uint32_t cacheval; >>>>> +} RawBitsEncContext; >>>>> + >>>>> typedef struct OpusRangeCoder { >>>>> GetBitContext gb; >>>>> - RawBitsContext rb; >>>>> + union { >>>>> + RawBitsContext rb; >>>>> + RawBitsEncContext rbe; >>>>> + }; >>>>> uint32_t range; >>>>> uint32_t value; >>>>> uint32_t total_bits; >>>>> >>>>> and use rbe in ff_opus_rc_put_raw(). >>> >>>> NAK, we don't do anyonymous unions. >>> >>> The suggested patch does not contain unions. >> >> It does, Are you author of this patch? > > This is a poc to show that there is a possible (although > ugly) alternative to the - imo - unavoidable cast that I > attached as a git-formatted patch. Yes, it is poc - patch of crap.
2018-12-17 2:47 GMT+01:00, Carl Eugen Hoyos <ceffmpeg@gmail.com>: > The Opus struct RawBitsContext is used in both the decoder and the encoder. > The fact that *position is const avoids warnings in the decoder where > it points into the bitstream. The encoder writes into the same > pointer, attached cast silences the warning on targets where AV_WB32() > does not internally cast the qualifier away. Ping. Carl Eugen
On 12/17/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2018-12-17 2:47 GMT+01:00, Carl Eugen Hoyos <ceffmpeg@gmail.com>: > >> The Opus struct RawBitsContext is used in both the decoder and the >> encoder. >> The fact that *position is const avoids warnings in the decoder where >> it points into the bitstream. The encoder writes into the same >> pointer, attached cast silences the warning on targets where AV_WB32() >> does not internally cast the qualifier away. > > Ping. Anonymous unions are not accepted. NAK 2X!
2018-12-17 22:26 GMT+01:00, Paul B Mahol <onemda@gmail.com>: > On 12/17/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> 2018-12-17 2:47 GMT+01:00, Carl Eugen Hoyos <ceffmpeg@gmail.com>: >> >>> The Opus struct RawBitsContext is used in both the decoder and the >>> encoder. >>> The fact that *position is const avoids warnings in the decoder where >>> it points into the bitstream. The encoder writes into the same >>> pointer, attached cast silences the warning on targets where AV_WB32() >>> does not internally cast the qualifier away. >> >> Ping. > > Anonymous unions are not accepted. NAK 2X! Again: There is no union in this patch. Please review, Carl Eugen
On 12/17/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2018-12-17 22:26 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >> On 12/17/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>> 2018-12-17 2:47 GMT+01:00, Carl Eugen Hoyos <ceffmpeg@gmail.com>: >>> >>>> The Opus struct RawBitsContext is used in both the decoder and the >>>> encoder. >>>> The fact that *position is const avoids warnings in the decoder where >>>> it points into the bitstream. The encoder writes into the same >>>> pointer, attached cast silences the warning on targets where AV_WB32() >>>> does not internally cast the qualifier away. >>> >>> Ping. >> >> Anonymous unions are not accepted. NAK 2X! > > Again: There is no union in this patch. Use "grep union" on your patch.
2018-12-17 22:29 GMT+01:00, Paul B Mahol <onemda@gmail.com>: > On 12/17/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> 2018-12-17 22:26 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >>> On 12/17/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>> 2018-12-17 2:47 GMT+01:00, Carl Eugen Hoyos <ceffmpeg@gmail.com>: >>>> >>>>> The Opus struct RawBitsContext is used in both the decoder and the >>>>> encoder. >>>>> The fact that *position is const avoids warnings in the decoder where >>>>> it points into the bitstream. The encoder writes into the same >>>>> pointer, attached cast silences the warning on targets where AV_WB32() >>>>> does not internally cast the qualifier away. >>>> >>>> Ping. >>> >>> Anonymous unions are not accepted. NAK 2X! >> >> Again: There is no union in this patch. > > Use "grep union" on your patch. $ grep union 0001-lavc-opus_rc-Cast-a-const-pointer-to-uint8_t.patch $ Carl Eugen
On 12/17/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: > 2018-12-17 22:29 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >> On 12/17/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>> 2018-12-17 22:26 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >>>> On 12/17/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>>> 2018-12-17 2:47 GMT+01:00, Carl Eugen Hoyos <ceffmpeg@gmail.com>: >>>>> >>>>>> The Opus struct RawBitsContext is used in both the decoder and the >>>>>> encoder. >>>>>> The fact that *position is const avoids warnings in the decoder where >>>>>> it points into the bitstream. The encoder writes into the same >>>>>> pointer, attached cast silences the warning on targets where AV_WB32() >>>>>> does not internally cast the qualifier away. >>>>> >>>>> Ping. >>>> >>>> Anonymous unions are not accepted. NAK 2X! >>> >>> Again: There is no union in this patch. >> >> Use "grep union" on your patch. > > $ grep union 0001-lavc-opus_rc-Cast-a-const-pointer-to-uint8_t.patch > $ Why you posted two variants of patch in single mail? This is extremely confusing.
2018-12-17 22:37 GMT+01:00, Paul B Mahol <onemda@gmail.com>: > On 12/17/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >> 2018-12-17 22:29 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >>> On 12/17/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>> 2018-12-17 22:26 GMT+01:00, Paul B Mahol <onemda@gmail.com>: >>>>> On 12/17/18, Carl Eugen Hoyos <ceffmpeg@gmail.com> wrote: >>>>>> 2018-12-17 2:47 GMT+01:00, Carl Eugen Hoyos <ceffmpeg@gmail.com>: >>>>>> >>>>>>> The Opus struct RawBitsContext is used in both the decoder and the >>>>>>> encoder. >>>>>>> The fact that *position is const avoids warnings in the decoder where >>>>>>> it points into the bitstream. The encoder writes into the same >>>>>>> pointer, attached cast silences the warning on targets where >>>>>>> AV_WB32() >>>>>>> does not internally cast the qualifier away. >>>>>> >>>>>> Ping. >>>>> >>>>> Anonymous unions are not accepted. NAK 2X! >>>> >>>> Again: There is no union in this patch. >>> >>> Use "grep union" on your patch. >> >> $ grep union 0001-lavc-opus_rc-Cast-a-const-pointer-to-uint8_t.patch >> $ > > Why you posted two variants of patch in single mail? > > This is extremely confusing. Sorry: I posted a patch and I mentioned that the only alternative that I saw is an ugly union (in case somebody doesn't like the cast). As explained in my original mail, the same definition is used by two different parts of the code with different semantics, it is not possible to simply drop the const. Carl Eugen
Paul B Mahol (2018-12-17):
> Yes, it is poc - patch of crap.
Please refrain from using that language.
2018-12-17 2:47 GMT+01:00, Carl Eugen Hoyos <ceffmpeg@gmail.com>: > The Opus struct RawBitsContext is used in both the decoder and the encoder. > The fact that *position is const avoids warnings in the decoder where > it points into the bitstream. The encoder writes into the same > pointer, attached cast silences the warning on targets where AV_WB32() > does not internally cast the qualifier away. (... like arm Android) I'll push this if there are no more comments. Carl Eugen
2018-12-17 2:47 GMT+01:00, Carl Eugen Hoyos <ceffmpeg@gmail.com>: > The Opus struct RawBitsContext is used in both the decoder and the encoder. > The fact that *position is const avoids warnings in the decoder where > it points into the bitstream. The encoder writes into the same > pointer, attached cast silences the warning on targets where AV_WB32() > does not internally cast the qualifier away. Patch applied. Carl Eugen
From 2fe5044a4532f061ef4090fd46f1ec1a3af067b0 Mon Sep 17 00:00:00 2001 From: Carl Eugen Hoyos <ceffmpeg@gmail.com> Date: Mon, 17 Dec 2018 02:36:26 +0100 Subject: [PATCH] lavc/opus_rc: Cast a const pointer to uint8_t *. Silences a warning with clang on arm: libavcodec/opus_rc.c:170:17: warning: passing 'const uint8_t *' (aka 'const unsigned char *') to parameter of type 'void *' discards qualifiers --- libavcodec/opus_rc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libavcodec/opus_rc.c b/libavcodec/opus_rc.c index 3972bb0..c432eb9 100644 --- a/libavcodec/opus_rc.c +++ b/libavcodec/opus_rc.c @@ -167,7 +167,7 @@ void ff_opus_rc_put_raw(OpusRangeCoder *rc, uint32_t val, uint32_t count) rc->rb.cachelen = (rc->rb.cachelen + to_write) % 32; if (!rc->rb.cachelen && count) { - AV_WB32(rc->rb.position, rc->rb.cacheval); + AV_WB32((uint8_t *)rc->rb.position, rc->rb.cacheval); rc->rb.bytes += 4; rc->rb.position -= 4; rc->rb.cachelen = count - to_write; -- 1.7.10.4