ffvvc / FFmpeg

VVC Decoder for ffmpeg
Other
50 stars 12 forks source link

Use -Werror and -Wpedantic in CI #238

Open frankplow opened 3 months ago

frankplow commented 3 months ago

This patchset does two things:

Note these stricter compilation settings fail with the current HEAD, due to the error:

libavcodec/vvc/intra_template.c:201:63: error: pointers to arrays with different qualifiers are incompatible in ISO C [-Werror=pedantic]
  201 |     FUNC(cclm_select_luma)(fc, x0, y0, avail_t, avail_l, cnt, pos, sel[LUMA]);

in this location and various others.

frankplow commented 3 months ago

Note these stricter compilation settings fail with the current HEAD, due to the error:

libavcodec/vvc/intra_template.c:201:63: error: pointers to arrays with different qualifiers are incompatible in ISO C [-Werror=pedantic]
  201 |     FUNC(cclm_select_luma)(fc, x0, y0, avail_t, avail_l, cnt, pos, sel[LUMA]);

in this location and various others.

This seems to be an issue with C more than anything else. The issue is discussed in N 1923 in detail, including workarounds, and a fix has been accepted in C23 as N 2607. The workarounds don't sound ideal, so perhaps we explicitly ignore this warning?

nuomi2021 commented 3 months ago

This seems to be an issue with C more than anything else. The issue is discussed in N 1923 in detail, including workarounds, and a fix has been accepted in C23 as N 2607. The workarounds don't sound ideal, so perhaps we explicitly ignore this warning?

Agree. It's an ISO C issue. :) We can ignore it.

frankplow commented 3 months ago

This seems to be an issue with C more than anything else. The issue is discussed in N 1923 in detail, including workarounds, and a fix has been accepted in C23 as N 2607. The workarounds don't sound ideal, so perhaps we explicitly ignore this warning?

Agree. It's an ISO C issue. :) We can ignore it.

Unfortunately there does not appear to be any easy way to do this. There are no command line flags for GCC to selectively disable some ISO compliance checks, either it tests if the code is ISO C compliant or not. The only workaround would be to use pragmas to selectively disable -Wpedantic for only the problematic lines, but we'd have to patch this in and the diff would break with nearly any change to the code so I don't think this is workable. I think if we want to add these checks, our best bet is to modify the code to be fully ISO C compliant in these cases.

nuomi2021 commented 3 months ago

I think if we want to add these checks, our best bet is to modify the code to be fully ISO C compliant in these cases.

If this costs too much time, it may not be worth it. Focus on fuzz may help us more.

But if you want, we can change https://github.com/ffvvc/FFmpeg/blob/e81b6d78fc2ddf8edd53a6a052713354ef8d27c2/libavcodec/vvc/vvc_intra_template.c#L241 to 1d array like pixel sel[VVC_MAX_SAMPLE_ARRAYS MAX_PICK_POS 2];

Thank you