xiph: const correctness for avpriv_split_xiph_headers()

Message ID 1478347221-27337-1-git-send-email-diego@biurrun.de
State New
Headers show

Commit Message

Diego Biurrun Nov. 5, 2016, noon
libavcodec/vorbis_parser.c:194:42: warning: passing argument 1 of ‘avpriv_split_xiph_headers’ discards ‘const’ qualifier from pointer target type
---
 libavcodec/vorbis_parser.c | 2 +-
 libavcodec/vorbisdec.c     | 2 +-
 libavcodec/vp3.c           | 2 +-
 libavcodec/xiph.c          | 7 ++++---
 libavcodec/xiph.h          | 5 +++--
 libavformat/matroskaenc.c  | 2 +-
 libavformat/oggenc.c       | 5 +++--
 libavformat/sdp.c          | 2 +-
 8 files changed, 15 insertions(+), 12 deletions(-)

Comments

Luca Barbato Nov. 5, 2016, 3:21 p.m. | #1
On 05/11/2016 13:00, Diego Biurrun wrote:
> libavcodec/vorbis_parser.c:194:42: warning: passing argument 1 of ‘avpriv_split_xiph_headers’ discards ‘const’ qualifier from pointer target type
> ---
>  libavcodec/vorbis_parser.c | 2 +-
>  libavcodec/vorbisdec.c     | 2 +-
>  libavcodec/vp3.c           | 2 +-
>  libavcodec/xiph.c          | 7 ++++---
>  libavcodec/xiph.h          | 5 +++--
>  libavformat/matroskaenc.c  | 2 +-
>  libavformat/oggenc.c       | 5 +++--
>  libavformat/sdp.c          | 2 +-
>  8 files changed, 15 insertions(+), 12 deletions(-)
> 

If fate is happy ok.
Anton Khirnov Nov. 7, 2016, 2:22 p.m. | #2
Quoting Diego Biurrun (2016-11-05 13:00:21)
> libavcodec/vorbis_parser.c:194:42: warning: passing argument 1 of ‘avpriv_split_xiph_headers’ discards ‘const’ qualifier from pointer target type
> ---
>  libavcodec/vorbis_parser.c | 2 +-
>  libavcodec/vorbisdec.c     | 2 +-
>  libavcodec/vp3.c           | 2 +-
>  libavcodec/xiph.c          | 7 ++++---
>  libavcodec/xiph.h          | 5 +++--
>  libavformat/matroskaenc.c  | 2 +-
>  libavformat/oggenc.c       | 5 +++--
>  libavformat/sdp.c          | 2 +-
>  8 files changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/libavcodec/vorbis_parser.c b/libavcodec/vorbis_parser.c
> index 054635d..181db90 100644
> --- a/libavcodec/vorbis_parser.c
> +++ b/libavcodec/vorbis_parser.c
> @@ -184,7 +184,7 @@ bad_header:
>  static int vorbis_parse_init(AVVorbisParseContext *s,
>                               const uint8_t *extradata, int extradata_size)
>  {
> -    uint8_t *header_start[3];
> +    const uint8_t *header_start[3];
>      int header_len[3];
>      int ret;
>  
> diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
> index 8b800fd..62a3343 100644
> --- a/libavcodec/vorbisdec.c
> +++ b/libavcodec/vorbisdec.c
> @@ -1008,7 +1008,7 @@ static av_cold int vorbis_decode_init(AVCodecContext *avctx)
>      vorbis_context *vc = avctx->priv_data;
>      uint8_t *headers   = avctx->extradata;
>      int headers_len    = avctx->extradata_size;
> -    uint8_t *header_start[3];
> +    const uint8_t *header_start[3];
>      int header_len[3];
>      GetBitContext *gb = &vc->gb;
>      int hdr_type, ret;
> diff --git a/libavcodec/vp3.c b/libavcodec/vp3.c
> index 26374cc..4dbcce5 100644
> --- a/libavcodec/vp3.c
> +++ b/libavcodec/vp3.c
> @@ -2426,7 +2426,7 @@ static av_cold int theora_decode_init(AVCodecContext *avctx)
>      Vp3DecodeContext *s = avctx->priv_data;
>      GetBitContext gb;
>      int ptype;
> -    uint8_t *header_start[3];
> +    const uint8_t *header_start[3];
>      int header_len[3];
>      int i;
>  
> diff --git a/libavcodec/xiph.c b/libavcodec/xiph.c
> index 7c3c710..4601de1 100644
> --- a/libavcodec/xiph.c
> +++ b/libavcodec/xiph.c
> @@ -21,9 +21,10 @@
>  #include "libavutil/intreadwrite.h"
>  #include "xiph.h"
>  
> -int avpriv_split_xiph_headers(uint8_t *extradata, int extradata_size,
> -                          int first_header_size, uint8_t *header_start[3],
> -                          int header_len[3])
> +int avpriv_split_xiph_headers(const uint8_t *extradata, int extradata_size,
> +                              int first_header_size,
> +                              const uint8_t *header_start[3],
> +                              int header_len[3])
>  {
>      int i;
>  
> diff --git a/libavcodec/xiph.h b/libavcodec/xiph.h
> index afaece7..fc22a0e 100644
> --- a/libavcodec/xiph.h
> +++ b/libavcodec/xiph.h
> @@ -36,8 +36,9 @@
>   * @param[out] header_len The sizes of each of the three headers.
>   * @return On error a negative value is returned, on success zero.
>   */
> -int avpriv_split_xiph_headers(uint8_t *extradata, int extradata_size,
> -                              int first_header_size, uint8_t *header_start[3],
> +int avpriv_split_xiph_headers(const uint8_t *extradata, int extradata_size,
> +                              int first_header_size,
> +                              const uint8_t *header_start[3],
>                                int header_len[3]);
>  
>  #endif /* AVCODEC_XIPH_H */
> diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> index cfced72..cb2f97b 100644
> --- a/libavformat/matroskaenc.c
> +++ b/libavformat/matroskaenc.c
> @@ -442,7 +442,7 @@ static int64_t mkv_write_cues(AVIOContext *pb, mkv_cues *cues, int num_tracks)
>  
>  static int put_xiph_codecpriv(AVFormatContext *s, AVIOContext *pb, AVCodecParameters *par)
>  {
> -    uint8_t *header_start[3];
> +    const uint8_t *header_start[3];
>      int header_len[3];
>      int first_header_size;
>      int j;
> diff --git a/libavformat/oggenc.c b/libavformat/oggenc.c
> index 2fef74a..f00cd3e 100644
> --- a/libavformat/oggenc.c
> +++ b/libavformat/oggenc.c
> @@ -494,8 +494,9 @@ static int ogg_write_header(AVFormatContext *s)
>              int framing_bit = st->codecpar->codec_id == AV_CODEC_ID_VORBIS ? 1 : 0;
>  
>              if (avpriv_split_xiph_headers(st->codecpar->extradata, st->codecpar->extradata_size,
> -                                      st->codecpar->codec_id == AV_CODEC_ID_VORBIS ? 30 : 42,
> -                                      oggstream->header, oggstream->header_len) < 0) {
> +                                          st->codecpar->codec_id == AV_CODEC_ID_VORBIS ? 30 : 42,
> +                                          (const uint8_t **)oggstream->header,
> +                                          oggstream->header_len) < 0) {

If we still had -Wcast-qual enabled by default, gcc would (very
correctly) warn about casting away const, since you end up casting
pointers-to-const to unqualified pointers.

So this patch just exchanges one warning for another.
Diego Biurrun Nov. 7, 2016, 7:47 p.m. | #3
On Mon, Nov 07, 2016 at 03:22:03PM +0100, Anton Khirnov wrote:
> Quoting Diego Biurrun (2016-11-05 13:00:21)
> > libavcodec/vorbis_parser.c:194:42: warning: passing argument 1 of ‘avpriv_split_xiph_headers’ discards ‘const’ qualifier from pointer target type
> > ---
> >  libavcodec/vorbis_parser.c | 2 +-
> >  libavcodec/vorbisdec.c     | 2 +-
> >  libavcodec/vp3.c           | 2 +-
> >  libavcodec/xiph.c          | 7 ++++---
> >  libavcodec/xiph.h          | 5 +++--
> >  libavformat/matroskaenc.c  | 2 +-
> >  libavformat/oggenc.c       | 5 +++--
> >  libavformat/sdp.c          | 2 +-
> >  8 files changed, 15 insertions(+), 12 deletions(-)
> > 
> > diff --git a/libavcodec/vorbis_parser.c b/libavcodec/vorbis_parser.c
> > index 054635d..181db90 100644
> > --- a/libavcodec/vorbis_parser.c
> > +++ b/libavcodec/vorbis_parser.c
> > @@ -184,7 +184,7 @@ bad_header:
> >  static int vorbis_parse_init(AVVorbisParseContext *s,
> >                               const uint8_t *extradata, int extradata_size)
> >  {
> > -    uint8_t *header_start[3];
> > +    const uint8_t *header_start[3];
> >      int header_len[3];
> >      int ret;
> >  
> > diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
> > index 8b800fd..62a3343 100644
> > --- a/libavcodec/vorbisdec.c
> > +++ b/libavcodec/vorbisdec.c
> > @@ -1008,7 +1008,7 @@ static av_cold int vorbis_decode_init(AVCodecContext *avctx)
> >      vorbis_context *vc = avctx->priv_data;
> >      uint8_t *headers   = avctx->extradata;
> >      int headers_len    = avctx->extradata_size;
> > -    uint8_t *header_start[3];
> > +    const uint8_t *header_start[3];
> >      int header_len[3];
> >      GetBitContext *gb = &vc->gb;
> >      int hdr_type, ret;
> > diff --git a/libavcodec/vp3.c b/libavcodec/vp3.c
> > index 26374cc..4dbcce5 100644
> > --- a/libavcodec/vp3.c
> > +++ b/libavcodec/vp3.c
> > @@ -2426,7 +2426,7 @@ static av_cold int theora_decode_init(AVCodecContext *avctx)
> >      Vp3DecodeContext *s = avctx->priv_data;
> >      GetBitContext gb;
> >      int ptype;
> > -    uint8_t *header_start[3];
> > +    const uint8_t *header_start[3];
> >      int header_len[3];
> >      int i;
> >  
> > diff --git a/libavcodec/xiph.c b/libavcodec/xiph.c
> > index 7c3c710..4601de1 100644
> > --- a/libavcodec/xiph.c
> > +++ b/libavcodec/xiph.c
> > @@ -21,9 +21,10 @@
> >  #include "libavutil/intreadwrite.h"
> >  #include "xiph.h"
> >  
> > -int avpriv_split_xiph_headers(uint8_t *extradata, int extradata_size,
> > -                          int first_header_size, uint8_t *header_start[3],
> > -                          int header_len[3])
> > +int avpriv_split_xiph_headers(const uint8_t *extradata, int extradata_size,
> > +                              int first_header_size,
> > +                              const uint8_t *header_start[3],
> > +                              int header_len[3])
> >  {
> >      int i;
> >  
> > diff --git a/libavcodec/xiph.h b/libavcodec/xiph.h
> > index afaece7..fc22a0e 100644
> > --- a/libavcodec/xiph.h
> > +++ b/libavcodec/xiph.h
> > @@ -36,8 +36,9 @@
> >   * @param[out] header_len The sizes of each of the three headers.
> >   * @return On error a negative value is returned, on success zero.
> >   */
> > -int avpriv_split_xiph_headers(uint8_t *extradata, int extradata_size,
> > -                              int first_header_size, uint8_t *header_start[3],
> > +int avpriv_split_xiph_headers(const uint8_t *extradata, int extradata_size,
> > +                              int first_header_size,
> > +                              const uint8_t *header_start[3],
> >                                int header_len[3]);
> >  
> >  #endif /* AVCODEC_XIPH_H */
> > diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
> > index cfced72..cb2f97b 100644
> > --- a/libavformat/matroskaenc.c
> > +++ b/libavformat/matroskaenc.c
> > @@ -442,7 +442,7 @@ static int64_t mkv_write_cues(AVIOContext *pb, mkv_cues *cues, int num_tracks)
> >  
> >  static int put_xiph_codecpriv(AVFormatContext *s, AVIOContext *pb, AVCodecParameters *par)
> >  {
> > -    uint8_t *header_start[3];
> > +    const uint8_t *header_start[3];
> >      int header_len[3];
> >      int first_header_size;
> >      int j;
> > diff --git a/libavformat/oggenc.c b/libavformat/oggenc.c
> > index 2fef74a..f00cd3e 100644
> > --- a/libavformat/oggenc.c
> > +++ b/libavformat/oggenc.c
> > @@ -494,8 +494,9 @@ static int ogg_write_header(AVFormatContext *s)
> >              int framing_bit = st->codecpar->codec_id == AV_CODEC_ID_VORBIS ? 1 : 0;
> >  
> >              if (avpriv_split_xiph_headers(st->codecpar->extradata, st->codecpar->extradata_size,
> > -                                      st->codecpar->codec_id == AV_CODEC_ID_VORBIS ? 30 : 42,
> > -                                      oggstream->header, oggstream->header_len) < 0) {
> > +                                          st->codecpar->codec_id == AV_CODEC_ID_VORBIS ? 30 : 42,
> > +                                          (const uint8_t **)oggstream->header,
> > +                                          oggstream->header_len) < 0) {
> 
> If we still had -Wcast-qual enabled by default, gcc would (very
> correctly) warn about casting away const, since you end up casting
> pointers-to-const to unqualified pointers.

-Wcast-qual is not necessary, w/o the cast and default warning settings I get:

/home/diego/src/libav/libavformat/oggenc.c: In function ‘ogg_write_header’:
/home/diego/src/libav/libavformat/oggenc.c:498:43: warning: passing argument 4 of ‘avpriv_split_xiph_headers’ from incompatible pointer type
                                           oggstream->header,
                                           ^
In file included from /home/diego/src/libav/libavformat/oggenc.c:28:0:
/home/diego/src/libav/libavcodec/xiph.h:39:5: note: expected ‘const uint8_t **’ but argument is of type ‘uint8_t **’
 int avpriv_split_xiph_headers(const uint8_t *extradata, int extradata_size,
     ^

An annoying warning about passing non-const data where const is expected.
I am not casting away const, but adding const. Where is the problem?

> So this patch just exchanges one warning for another.

It constifies a bunch of variables and function parameters in the process,
which is progress IMO.

Diego
Anton Khirnov Nov. 8, 2016, 11:21 a.m. | #4
Quoting Diego Biurrun (2016-11-07 20:47:22)
> On Mon, Nov 07, 2016 at 03:22:03PM +0100, Anton Khirnov wrote:
> > Quoting Diego Biurrun (2016-11-05 13:00:21)
> > > diff --git a/libavformat/oggenc.c b/libavformat/oggenc.c
> > > index 2fef74a..f00cd3e 100644
> > > --- a/libavformat/oggenc.c
> > > +++ b/libavformat/oggenc.c
> > > @@ -494,8 +494,9 @@ static int ogg_write_header(AVFormatContext *s)
> > >              int framing_bit = st->codecpar->codec_id == AV_CODEC_ID_VORBIS ? 1 : 0;
> > >  
> > >              if (avpriv_split_xiph_headers(st->codecpar->extradata, st->codecpar->extradata_size,
> > > -                                      st->codecpar->codec_id == AV_CODEC_ID_VORBIS ? 30 : 42,
> > > -                                      oggstream->header, oggstream->header_len) < 0) {
> > > +                                          st->codecpar->codec_id == AV_CODEC_ID_VORBIS ? 30 : 42,
> > > +                                          (const uint8_t **)oggstream->header,
> > > +                                          oggstream->header_len) < 0) {
> > 
> > If we still had -Wcast-qual enabled by default, gcc would (very
> > correctly) warn about casting away const, since you end up casting
> > pointers-to-const to unqualified pointers.
> 
> -Wcast-qual is not necessary, w/o the cast and default warning settings I get:
> 
> /home/diego/src/libav/libavformat/oggenc.c: In function ‘ogg_write_header’:
> /home/diego/src/libav/libavformat/oggenc.c:498:43: warning: passing argument 4 of ‘avpriv_split_xiph_headers’ from incompatible pointer type
>                                            oggstream->header,
>                                            ^
> In file included from /home/diego/src/libav/libavformat/oggenc.c:28:0:
> /home/diego/src/libav/libavcodec/xiph.h:39:5: note: expected ‘const uint8_t **’ but argument is of type ‘uint8_t **’
>  int avpriv_split_xiph_headers(const uint8_t *extradata, int extradata_size,
>      ^
> 
> An annoying warning about passing non-const data where const is expected.
> I am not casting away const, but adding const. Where is the problem?

I think you are misunderstanding what the warning is about. After your
patch, the header_start parameter is 'const uint8_t *[3]'. What this
means is that the caller passes in a pointer to an array, and the
function fills this array with const pointers, i.e. pointers which the
caller can use only for reading, not modification.

In the call quoted above, the caller is passing in an array of non-const
pointers. They are cast to const to avoid warnings (which may not work
depending on the compiler and warning flags), but in any case we end up
with an array of non-const pointers, through which we _are_ allowed to
modify the data. So in effect what happens is that
avpriv_split_xiph_headers() gives us const pointers and we cast the
qualifier away to get non-const pointers.

> 
> > So this patch just exchanges one warning for another.
> 
> It constifies a bunch of variables and function parameters in the process,
> which is progress IMO.

I disagree. In fact I think it makes the problem worse, since the
warnings won't show up on many configurations (on the compilers I
tested, I have to add -Wcast-qual, otherwise they are perfectly silent),
despite the issue still being there.

Patch

diff --git a/libavcodec/vorbis_parser.c b/libavcodec/vorbis_parser.c
index 054635d..181db90 100644
--- a/libavcodec/vorbis_parser.c
+++ b/libavcodec/vorbis_parser.c
@@ -184,7 +184,7 @@  bad_header:
 static int vorbis_parse_init(AVVorbisParseContext *s,
                              const uint8_t *extradata, int extradata_size)
 {
-    uint8_t *header_start[3];
+    const uint8_t *header_start[3];
     int header_len[3];
     int ret;
 
diff --git a/libavcodec/vorbisdec.c b/libavcodec/vorbisdec.c
index 8b800fd..62a3343 100644
--- a/libavcodec/vorbisdec.c
+++ b/libavcodec/vorbisdec.c
@@ -1008,7 +1008,7 @@  static av_cold int vorbis_decode_init(AVCodecContext *avctx)
     vorbis_context *vc = avctx->priv_data;
     uint8_t *headers   = avctx->extradata;
     int headers_len    = avctx->extradata_size;
-    uint8_t *header_start[3];
+    const uint8_t *header_start[3];
     int header_len[3];
     GetBitContext *gb = &vc->gb;
     int hdr_type, ret;
diff --git a/libavcodec/vp3.c b/libavcodec/vp3.c
index 26374cc..4dbcce5 100644
--- a/libavcodec/vp3.c
+++ b/libavcodec/vp3.c
@@ -2426,7 +2426,7 @@  static av_cold int theora_decode_init(AVCodecContext *avctx)
     Vp3DecodeContext *s = avctx->priv_data;
     GetBitContext gb;
     int ptype;
-    uint8_t *header_start[3];
+    const uint8_t *header_start[3];
     int header_len[3];
     int i;
 
diff --git a/libavcodec/xiph.c b/libavcodec/xiph.c
index 7c3c710..4601de1 100644
--- a/libavcodec/xiph.c
+++ b/libavcodec/xiph.c
@@ -21,9 +21,10 @@ 
 #include "libavutil/intreadwrite.h"
 #include "xiph.h"
 
-int avpriv_split_xiph_headers(uint8_t *extradata, int extradata_size,
-                          int first_header_size, uint8_t *header_start[3],
-                          int header_len[3])
+int avpriv_split_xiph_headers(const uint8_t *extradata, int extradata_size,
+                              int first_header_size,
+                              const uint8_t *header_start[3],
+                              int header_len[3])
 {
     int i;
 
diff --git a/libavcodec/xiph.h b/libavcodec/xiph.h
index afaece7..fc22a0e 100644
--- a/libavcodec/xiph.h
+++ b/libavcodec/xiph.h
@@ -36,8 +36,9 @@ 
  * @param[out] header_len The sizes of each of the three headers.
  * @return On error a negative value is returned, on success zero.
  */
-int avpriv_split_xiph_headers(uint8_t *extradata, int extradata_size,
-                              int first_header_size, uint8_t *header_start[3],
+int avpriv_split_xiph_headers(const uint8_t *extradata, int extradata_size,
+                              int first_header_size,
+                              const uint8_t *header_start[3],
                               int header_len[3]);
 
 #endif /* AVCODEC_XIPH_H */
diff --git a/libavformat/matroskaenc.c b/libavformat/matroskaenc.c
index cfced72..cb2f97b 100644
--- a/libavformat/matroskaenc.c
+++ b/libavformat/matroskaenc.c
@@ -442,7 +442,7 @@  static int64_t mkv_write_cues(AVIOContext *pb, mkv_cues *cues, int num_tracks)
 
 static int put_xiph_codecpriv(AVFormatContext *s, AVIOContext *pb, AVCodecParameters *par)
 {
-    uint8_t *header_start[3];
+    const uint8_t *header_start[3];
     int header_len[3];
     int first_header_size;
     int j;
diff --git a/libavformat/oggenc.c b/libavformat/oggenc.c
index 2fef74a..f00cd3e 100644
--- a/libavformat/oggenc.c
+++ b/libavformat/oggenc.c
@@ -494,8 +494,9 @@  static int ogg_write_header(AVFormatContext *s)
             int framing_bit = st->codecpar->codec_id == AV_CODEC_ID_VORBIS ? 1 : 0;
 
             if (avpriv_split_xiph_headers(st->codecpar->extradata, st->codecpar->extradata_size,
-                                      st->codecpar->codec_id == AV_CODEC_ID_VORBIS ? 30 : 42,
-                                      oggstream->header, oggstream->header_len) < 0) {
+                                          st->codecpar->codec_id == AV_CODEC_ID_VORBIS ? 30 : 42,
+                                          (const uint8_t **)oggstream->header,
+                                          oggstream->header_len) < 0) {
                 av_log(s, AV_LOG_ERROR, "Extradata corrupted\n");
                 av_freep(&st->priv_data);
                 return -1;
diff --git a/libavformat/sdp.c b/libavformat/sdp.c
index be6c95d..aa84881 100644
--- a/libavformat/sdp.c
+++ b/libavformat/sdp.c
@@ -348,7 +348,7 @@  static char *extradata2config(AVFormatContext *s, AVCodecParameters *par)
 static char *xiph_extradata2config(AVFormatContext *s, AVCodecParameters *par)
 {
     char *config, *encoded_config;
-    uint8_t *header_start[3];
+    const uint8_t *header_start[3];
     int headers_len, header_len[3], config_len;
     int first_header_size;