| Message ID | 20240916182420.188-3-shoutplenty@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] av_get_frame_filename2 parameter int -> int64_t | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/commit_msg_loongarch64 | warning | The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ". |
| andriy/commit_msg_x86 | warning | The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ". |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_x86 | warning | New warnings during build |
On 9/16/24 2:22 PM, Filip Mašić wrote: > my apologies for re-sending; i didn't spot that there was a header file. my previous email may be disregarded. > > suggested fix for ticket 11194; see ticket for more info. i haven't been able to test this since i don't have any build tools installed, but decided to submit it since it seems to be a trivial change and easy to test > > Signed-off-by: Filip Mašić <shoutplenty@gmail.com> > --- > libavformat/avformat.h | 4 ++-- > libavformat/utils.c | 4 ++-- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/libavformat/avformat.h b/libavformat/avformat.h > index 4a3fb00529..76ffb008b4 100644 > --- a/libavformat/avformat.h > +++ b/libavformat/avformat.h > @@ -2911,10 +2911,10 @@ void av_dump_format(AVFormatContext *ic, > * @return 0 if OK, -1 on format error > */ > int av_get_frame_filename2(char *buf, int buf_size, > - const char *path, int number, int flags); > + const char *path, int64_t number, int flags); > > int av_get_frame_filename(char *buf, int buf_size, > - const char *path, int number); > + const char *path, int64_t number); > This is a breaking change, because you're changing the function signature (including the size of arguments) for a function exposed as a public API. You'll need to figure out a solution that doesn't break public interfaces. - Leo Izen (Traneptora)
On Mon, 16 Sept 2024 at 19:59, Leo Izen <leo.izen@gmail.com> wrote: > On 9/16/24 2:22 PM, Filip Mašić wrote: > > my apologies for re-sending; i didn't spot that there was a header file. > my previous email may be disregarded. > > > > suggested fix for ticket 11194; see ticket for more info. i haven't been > able to test this since i don't have any build tools installed, but decided > to submit it since it seems to be a trivial change and easy to test > > > > Signed-off-by: Filip Mašić <shoutplenty@gmail.com> > > --- > > libavformat/avformat.h | 4 ++-- > > libavformat/utils.c | 4 ++-- > > 2 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/libavformat/avformat.h b/libavformat/avformat.h > > index 4a3fb00529..76ffb008b4 100644 > > --- a/libavformat/avformat.h > > +++ b/libavformat/avformat.h > > @@ -2911,10 +2911,10 @@ void av_dump_format(AVFormatContext *ic, > > * @return 0 if OK, -1 on format error > > */ > > int av_get_frame_filename2(char *buf, int buf_size, > > - const char *path, int number, int flags); > > + const char *path, int64_t number, int flags); > > > > int av_get_frame_filename(char *buf, int buf_size, > > - const char *path, int number); > > + const char *path, int64_t number); > > > > This is a breaking change, because you're changing the function > signature (including the size of arguments) for a function exposed as > a public API. You'll need to figure out a solution that doesn't break > public interfaces. > > - Leo Izen (Traneptora) > Fair, thanks for reviewing. Two questions: 1. Can a widening conversion in a parameter really be a breaking change if everywhere the function would be called, it would still work as expected with no changes, provided the implementation is correct? 2. Does this mean the solution would be to fork av_get_frame_filename2(int) again into av_get_frame_filename3(int64_t) and implement versions #1 and #2 to call #3? Come to think of it, that's probably why there are two of those functions in the first place. Seems like mess begets more mess though. – Filip
On Monday, September 16th, 2024 at 2:27 PM, Filip Mašić shoutplenty@gmail.com wrote: > On Mon, 16 Sept 2024 at 19:59, Leo Izen leo.izen@gmail.com wrote: > >> On 9/16/24 2:22 PM, Filip Mašić wrote: >> >>> my apologies for re-sending; i didn't spot that there was a header file. >>> my previous email may be disregarded. >>> >>> suggested fix for ticket 11194; see ticket for more info. i haven't been >>> able to test this since i don't have any build tools installed, but decided >>> to submit it since it seems to be a trivial change and easy to test >>> >>> Signed-off-by: Filip Mašić shoutplenty@gmail.com >>> --- >>> libavformat/avformat.h | 4 ++-- >>> libavformat/utils.c | 4 ++-- >>> 2 files changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/libavformat/avformat.h b/libavformat/avformat.h >>> index 4a3fb00529..76ffb008b4 100644 >>> --- a/libavformat/avformat.h >>> +++ b/libavformat/avformat.h >>> @@ -2911,10 +2911,10 @@ void av_dump_format(AVFormatContext *ic, >>> * @return 0 if OK, -1 on format error >>> */ >>> int av_get_frame_filename2(char *buf, int buf_size, >>> - const char *path, int number, int flags); >>> + const char *path, int64_t number, int flags); >>> >>> int av_get_frame_filename(char *buf, int buf_size, >>> - const char *path, int number); >>> + const char *path, int64_t number); >> >> This is a breaking change, because you're changing the function >> signature (including the size of arguments) for a function exposed as >> a public API. You'll need to figure out a solution that doesn't break >> public interfaces. >> >> - Leo Izen (Traneptora) > > Fair, thanks for reviewing. Two questions: > 1. Can a widening conversion in a parameter really be a breaking change if > everywhere the function would be called, it would still work as expected > with no changes, provided the implementation is correct? Yes, because this breaks programs that are already compiled against the library. This kind of breaking change is called an ABI breaking change, and requires such programs to be recompiled. This patch might also break the API in some ways, but I'm not sure.
diff --git a/libavformat/avformat.h b/libavformat/avformat.h index 4a3fb00529..76ffb008b4 100644 --- a/libavformat/avformat.h +++ b/libavformat/avformat.h @@ -2911,10 +2911,10 @@ void av_dump_format(AVFormatContext *ic, * @return 0 if OK, -1 on format error */ int av_get_frame_filename2(char *buf, int buf_size, - const char *path, int number, int flags); + const char *path, int64_t number, int flags); int av_get_frame_filename(char *buf, int buf_size, - const char *path, int number); + const char *path, int64_t number); /** * Check whether filename actually is a numbered sequence generator. diff --git a/libavformat/utils.c b/libavformat/utils.c index e9ded627ad..41acc71949 100644 --- a/libavformat/utils.c +++ b/libavformat/utils.c @@ -280,7 +280,7 @@ uint64_t ff_parse_ntp_time(uint64_t ntp_ts) return (sec * 1000000) + usec; } -int av_get_frame_filename2(char *buf, int buf_size, const char *path, int number, int flags) +int av_get_frame_filename2(char *buf, int buf_size, const char *path, int64_t number, int flags) { const char *p; char *q, buf1[20], c; @@ -338,7 +338,7 @@ fail: return -1; } -int av_get_frame_filename(char *buf, int buf_size, const char *path, int number) +int av_get_frame_filename(char *buf, int buf_size, const char *path, int64_t number) { return av_get_frame_filename2(buf, buf_size, path, number, 0); }
my apologies for re-sending; i didn't spot that there was a header file. my previous email may be disregarded. suggested fix for ticket 11194; see ticket for more info. i haven't been able to test this since i don't have any build tools installed, but decided to submit it since it seems to be a trivial change and easy to test Signed-off-by: Filip Mašić <shoutplenty@gmail.com> --- libavformat/avformat.h | 4 ++-- libavformat/utils.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-)