| Message ID | 20230419195243.2974-23-anton@khirnov.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,01/25] fftools/ffmpeg_filter: drop write-only FilterGraph.reconfiguration | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
Anton Khirnov (12023-04-19): > Start by moving OutputStream.filtered_frame to it, which really belong > to the filtergraph rather than the output stream. What is the point of this? fftools/ffmpeg_filter: is not part of a library, concepts of public/private do not apply, just put everything you need in FilterGraph.
Quoting Nicolas George (2023-04-24 12:20:00) > Anton Khirnov (12023-04-19): > > Start by moving OutputStream.filtered_frame to it, which really belong > > to the filtergraph rather than the output stream. > > What is the point of this? fftools/ffmpeg_filter: is not part of a > library, concepts of public/private do not apply, just put everything > you need in FilterGraph. Separating internal state from public interfaces is a fundamental notion of object-oriented programming and is completely orthogonal to that object being a part of a library or not.
Anton Khirnov (12023-04-24): > Separating internal state from public interfaces is a fundamental notion > of object-oriented programming and is completely orthogonal to that > object being a part of a library or not. Using object-oriented programming to its fullest when it is not needed is a fundamental notion of mediocre programming. This patch only adds noise.
Quoting Nicolas George (2023-04-24 13:19:49) > Anton Khirnov (12023-04-24): > > Separating internal state from public interfaces is a fundamental notion > > of object-oriented programming and is completely orthogonal to that > > object being a part of a library or not. > > Using object-oriented programming to its fullest when it is not needed > is a fundamental notion of mediocre programming. This patch only adds > noise. We'll have to disagree about this then. I belive lack of proper structure is currently the biggest problem in ffmpeg CLI.
Anton Khirnov (12023-04-24): > We'll have to disagree about this then. I belive lack of proper > structure is currently the biggest problem in ffmpeg CLI. I do not disagree on this. But this patch does not help, it just makes the code locally less readable. If it is to be useful at all, it is at least severely premature.
Quoting Nicolas George (2023-04-24 13:59:43) > Anton Khirnov (12023-04-24): > > We'll have to disagree about this then. I belive lack of proper > > structure is currently the biggest problem in ffmpeg CLI. > > I do not disagree on this. But this patch does not help, it just makes > the code locally less readable. If it is to be useful at all, it is at > least severely premature. What exactly is less readable? One variable gets its scope reduced, that is a win in my book. And obviously I'm not stopping there, other things will be moved to this private struct in future patches, I just prefer to avoid giant patchsets when possible. Also note that exactly the same pattern is used in ffmpeg_demux, ffmpeg_mux, and ffmpeg_enc (and soon in ffmpeg_dec).
Anton Khirnov (12023-04-24): > What exactly is less readable? One variable gets its scope reduced, that > is a win in my book. Having to remember if a field is in structure x or structure y s a net loss that you do not see only because you just wrote the code and have everything freshly in mind. > And obviously I'm not stopping there, other things > will be moved to this private struct in future patches, Then move them into FilterGraph itself. If at some point later it makes sense to separate this structure to make the code clearer, we can do it at that time. If it happens, I predict the split will not be along the same “public”/“private” distinction you are trying to make right now. > I just prefer to > avoid giant patchsets when possible. Yes, please do, giant patchsets are rude. > Also note that exactly the same pattern is used in ffmpeg_demux, > ffmpeg_mux, and ffmpeg_enc (and soon in ffmpeg_dec). If I had noticed it at the time, I would have objected the same way.
Quoting Nicolas George (2023-04-24 14:13:45) > Anton Khirnov (12023-04-24): > > What exactly is less readable? One variable gets its scope reduced, that > > is a win in my book. > > Having to remember if a field is in structure x or structure y s a net > loss that you do not see only because you just wrote the code and have > everything freshly in mind. So when I wanted to make changes to libavfilter recently, you claimed your familiarity with the code makes you more qualified to judge readability. Now my familiarity with the code makes me LESS qualified. Curious. We've also been moving private state to private data for many years now and none of your conjectured concerns materialized, to the contrary code became easier to maintain. > > And obviously I'm not stopping there, other things > > will be moved to this private struct in future patches, > > Then move them into FilterGraph itself. If at some point later it makes > sense to separate this structure to make the code clearer, we can do it > at that time. If it happens, I predict the split will not be along the > same “public”/“private” distinction you are trying to make right now. Now that would be pure noise. I already know FilterGraph will have private data, I intend to move more things into it in the very next patchset. > > Also note that exactly the same pattern is used in ffmpeg_demux, > > ffmpeg_mux, and ffmpeg_enc (and soon in ffmpeg_dec). > > If I had noticed it at the time, I would have objected the same way. I have no idea what are you even objecting to. What is even controversial about not exposing state that does not need to be exposed?
Anton Khirnov (12023-04-24): > So when I wanted to make changes to libavfilter recently, you claimed > your familiarity with the code makes you more qualified to judge > readability. Now my familiarity with the code makes me LESS qualified. > Curious. There is a difference between long-term knowledge of a large part of the code and a short-term acquaintance with a limited slice of the code, I hope you realize. > We've also been moving private state to private data for many years now > and none of your conjectured concerns materialized, to the contrary code > became easier to maintain. Untrue. For example, every instance of FFFormatContext in the code gives places where the code has become that much more annoying to maintain. Maybe the same code has become more maintainable at the same time due to other changes, but the fact remains that these changes make it harder to work on the code. > Now that would be pure noise. The only noise here is all the fgp_from_fg() you want to liter over the code and the extra variables it requires. > I have no idea what are you even objecting to. What is even > controversial about not exposing state that does not need to be exposed? I have explained time and again: I oppose to any change that requires us to remember or check which structure a given field belongs to when it is not already obvious by its semantic. And again, there is nothing exposed to hide here.
On Mon, Apr 24, 2023 at 8:32 PM Nicolas George <george@nsup.org> wrote: > Anton Khirnov (12023-04-24): > > So when I wanted to make changes to libavfilter recently, you claimed > > your familiarity with the code makes you more qualified to judge > > readability. Now my familiarity with the code makes me LESS qualified. > > Curious. > > There is a difference between long-term knowledge of a large part of the > code and a short-term acquaintance with a limited slice of the code, I > hope you realize. > > > We've also been moving private state to private data for many years now > > and none of your conjectured concerns materialized, to the contrary code > > became easier to maintain. > > Untrue. For example, every instance of FFFormatContext in the code gives > places where the code has become that much more annoying to maintain. > Maybe the same code has become more maintainable at the same time due to > other changes, but the fact remains that these changes make it harder to > work on the code. > > > Now that would be pure noise. > > The only noise here is all the fgp_from_fg() you want to liter over the > code and the extra variables it requires. > > > I have no idea what are you even objecting to. What is even > > controversial about not exposing state that does not need to be exposed? > > I have explained time and again: I oppose to any change that requires us > to remember or check which structure a given field belongs to when it is > not already obvious by its semantic. > > And again, there is nothing exposed to hide here. > Why should anybody here listen to your entries here? When was last time you contributed anything marginally useful? > > -- > Nicolas George > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
On 4/24/2023 3:33 PM, Paul B Mahol wrote: > On Mon, Apr 24, 2023 at 8:32 PM Nicolas George <george@nsup.org> wrote: > >> Anton Khirnov (12023-04-24): >>> So when I wanted to make changes to libavfilter recently, you claimed >>> your familiarity with the code makes you more qualified to judge >>> readability. Now my familiarity with the code makes me LESS qualified. >>> Curious. >> >> There is a difference between long-term knowledge of a large part of the >> code and a short-term acquaintance with a limited slice of the code, I >> hope you realize. >> >>> We've also been moving private state to private data for many years now >>> and none of your conjectured concerns materialized, to the contrary code >>> became easier to maintain. >> >> Untrue. For example, every instance of FFFormatContext in the code gives >> places where the code has become that much more annoying to maintain. >> Maybe the same code has become more maintainable at the same time due to >> other changes, but the fact remains that these changes make it harder to >> work on the code. >> >>> Now that would be pure noise. >> >> The only noise here is all the fgp_from_fg() you want to liter over the >> code and the extra variables it requires. >> >>> I have no idea what are you even objecting to. What is even >>> controversial about not exposing state that does not need to be exposed? >> >> I have explained time and again: I oppose to any change that requires us >> to remember or check which structure a given field belongs to when it is >> not already obvious by its semantic. >> >> And again, there is nothing exposed to hide here. >> > > Why should anybody here listen to your entries here? > When was last time you contributed anything marginally useful? Lets limit things to technical arguments, please.
On Mon, Apr 24, 2023 at 8:37 PM James Almer <jamrial@gmail.com> wrote: > On 4/24/2023 3:33 PM, Paul B Mahol wrote: > > On Mon, Apr 24, 2023 at 8:32 PM Nicolas George <george@nsup.org> wrote: > > > >> Anton Khirnov (12023-04-24): > >>> So when I wanted to make changes to libavfilter recently, you claimed > >>> your familiarity with the code makes you more qualified to judge > >>> readability. Now my familiarity with the code makes me LESS qualified. > >>> Curious. > >> > >> There is a difference between long-term knowledge of a large part of the > >> code and a short-term acquaintance with a limited slice of the code, I > >> hope you realize. > >> > >>> We've also been moving private state to private data for many years now > >>> and none of your conjectured concerns materialized, to the contrary > code > >>> became easier to maintain. > >> > >> Untrue. For example, every instance of FFFormatContext in the code gives > >> places where the code has become that much more annoying to maintain. > >> Maybe the same code has become more maintainable at the same time due to > >> other changes, but the fact remains that these changes make it harder to > >> work on the code. > >> > >>> Now that would be pure noise. > >> > >> The only noise here is all the fgp_from_fg() you want to liter over the > >> code and the extra variables it requires. > >> > >>> I have no idea what are you even objecting to. What is even > >>> controversial about not exposing state that does not need to be > exposed? > >> > >> I have explained time and again: I oppose to any change that requires us > >> to remember or check which structure a given field belongs to when it is > >> not already obvious by its semantic. > >> > >> And again, there is nothing exposed to hide here. > >> > > > > Why should anybody here listen to your entries here? > > When was last time you contributed anything marginally useful? > > Lets limit things to technical arguments, please. > Nicolas want one monumental structure like old AVCodecContext and still current MPV structure. There is nothing technical in this discussion, from start, at all. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
Quoting Nicolas George (2023-04-24 20:31:49) > Anton Khirnov (12023-04-24): > > So when I wanted to make changes to libavfilter recently, you claimed > > your familiarity with the code makes you more qualified to judge > > readability. Now my familiarity with the code makes me LESS qualified. > > Curious. > > There is a difference between long-term knowledge of a large part of the > code and a short-term acquaintance with a limited slice of the code, I > hope you realize. I have no idea what you mean by this. > > We've also been moving private state to private data for many years now > > and none of your conjectured concerns materialized, to the contrary code > > became easier to maintain. > > Untrue. For example, every instance of FFFormatContext in the code gives > places where the code has become that much more annoying to maintain. > Maybe the same code has become more maintainable at the same time due to > other changes, but the fact remains that these changes make it harder to > work on the code. > true > fact You keep using these words. I don't think they mean what you think they mean. The only fact here is that the quoted paragraph is YOUR PERSONAL OPINION, which ZERO other developers expressed support for. On the other hand, the approach under discussion has explicit support from multiple highly active developers. > > Now that would be pure noise. > > The only noise here is all the fgp_from_fg() you want to liter over the > code and the extra variables it requires. > > > I have no idea what are you even objecting to. What is even > > controversial about not exposing state that does not need to be exposed? > > I have explained time and again: I oppose to any change that requires us > to remember or check which structure a given field belongs to when it is > not already obvious by its semantic. You are too late, many such changes have already been pushed in the last several years. Nobody except you opposes them and the likelihood of reversing them is extremely low. This whole thread is a pointless waste of time that could be spend doing something actually useful.
Anton Khirnov (12023-04-24): > I have no idea what you mean by this. Try a little harder. > You keep using these words. I don't think they mean what you think they > mean. Yeah, Princess Bride references are a real boost to weak arguments. > The only fact here is that the quoted paragraph is YOUR PERSONAL > OPINION, which ZERO other developers expressed support for. Or maybe I am the only one not fed up of arguing with walls. Will you dispute that each split of a structure is extra code in many places? Will you discuss that having to remember which structure a field belongs to is an extra effort? > On the other hand, the approach under discussion has explicit support > from multiple highly active developers. I have not seen this specific aspect of the issue discussed. > You are too late, many such changes have already been pushed in the last > several years. Nobody except you opposes them and the likelihood of > reversing them is extremely low. This whole thread is a pointless waste > of time that could be spend doing something actually useful. It is never too late to stop making things worse.
Quoting Nicolas George (2023-04-24 21:24:20) > Anton Khirnov (12023-04-24): > > I have no idea what you mean by this. > > Try a little harder. No actual explanation then. So I suppose what you mean is that different rules apply to you and everyone else. > > The only fact here is that the quoted paragraph is YOUR PERSONAL > > OPINION, which ZERO other developers expressed support for. > > Or maybe I am the only one not fed up of arguing with walls. There is zero evidence of any active developer agreeing with you on this. Feel welcome to prove me wrong. > Will you dispute that each split of a structure is extra code in many > places? I will, the overhead code required is trivial compared to the code being wrapped. > Will you discuss that having to remember which structure a field belongs > to is an extra effort? I will, not having to consider whether a field is accessed from other parts of the code significantly reduces the mental load required. That is the whole point of information hiding.
Anton Khirnov (12023-04-24): > No actual explanation then. So I suppose what you mean is that different > rules apply to you and everyone else. No. I have said what I mean, I will not let you distort it to suit your needs. > I will, the overhead code required is trivial compared to the code being > wrapped. So there is overhead code. Thank you. > > Will you discuss that having to remember which structure a field belongs > > to is an extra effort? > I will, not having to consider whether a field is accessed from other parts > of the code significantly reduces the mental load required. That is the > whole point of information hiding. I see you have not actually refuted my point, so I consider it acted too. So, we have established this patch incurs a cost. Now you need to prove it brings a benefit. Not a hypothetical benefit like “whole point of information hiding”, but an actual immediate benefit.
diff --git a/fftools/ffmpeg.h b/fftools/ffmpeg.h index 95591f4bba..71f348da01 100644 --- a/fftools/ffmpeg.h +++ b/fftools/ffmpeg.h @@ -603,7 +603,6 @@ typedef struct OutputStream { Encoder *enc; AVCodecContext *enc_ctx; - AVFrame *filtered_frame; AVPacket *pkt; int64_t last_dropped; diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c index b26160b375..edfefc9d45 100644 --- a/fftools/ffmpeg_filter.c +++ b/fftools/ffmpeg_filter.c @@ -38,6 +38,18 @@ #include "libavutil/samplefmt.h" #include "libavutil/timestamp.h" +typedef struct FilterGraphPriv { + FilterGraph fg; + + // frame for temporarily holding output from the filtergraph + AVFrame *frame; +} FilterGraphPriv; + +static FilterGraphPriv *fgp_from_fg(FilterGraph *fg) +{ + return (FilterGraphPriv*)fg; +} + // FIXME: YUV420P etc. are actually supported with full color range, // yet the latter information isn't available here. static const enum AVPixelFormat *get_compliance_normal_pix_fmts(const AVCodec *codec, const enum AVPixelFormat default_formats[]) @@ -192,9 +204,11 @@ static OutputFilter *ofilter_alloc(FilterGraph *fg) void fg_free(FilterGraph **pfg) { FilterGraph *fg = *pfg; + FilterGraphPriv *fgp; if (!fg) return; + fgp = fgp_from_fg(fg); avfilter_graph_free(&fg->graph); for (int j = 0; j < fg->nb_inputs; j++) { @@ -230,17 +244,23 @@ void fg_free(FilterGraph **pfg) av_freep(&fg->outputs); av_freep(&fg->graph_desc); + av_frame_free(&fgp->frame); + av_freep(pfg); } FilterGraph *fg_create(char *graph_desc) { - FilterGraph *fg; + FilterGraphPriv *fgp = allocate_array_elem(&filtergraphs, sizeof(*fgp), &nb_filtergraphs); + FilterGraph *fg = &fgp->fg; - fg = ALLOC_ARRAY_ELEM(filtergraphs, nb_filtergraphs); fg->index = nb_filtergraphs - 1; fg->graph_desc = graph_desc; + fgp->frame = av_frame_alloc(); + if (!fgp->frame) + report_and_exit(AVERROR(ENOMEM)); + return fg; } @@ -1348,18 +1368,19 @@ int filtergraph_is_simple(FilterGraph *fg) int reap_filters(int flush) { - AVFrame *filtered_frame = NULL; - /* Reap all buffers present in the buffer sinks */ for (OutputStream *ost = ost_iter(NULL); ost; ost = ost_iter(ost)) { + FilterGraphPriv *fgp; + AVFrame *filtered_frame; AVFilterContext *filter; int ret = 0; if (!ost->filter || !ost->filter->graph->graph) continue; filter = ost->filter->filter; + fgp = fgp_from_fg(ost->filter->graph); - filtered_frame = ost->filtered_frame; + filtered_frame = fgp->frame; while (1) { ret = av_buffersink_get_frame_flags(filter, filtered_frame, diff --git a/fftools/ffmpeg_mux.c b/fftools/ffmpeg_mux.c index 368a7000a9..2a9db199c1 100644 --- a/fftools/ffmpeg_mux.c +++ b/fftools/ffmpeg_mux.c @@ -839,7 +839,6 @@ static void ost_free(OutputStream **post) av_bsf_free(&ms->bsf_ctx); - av_frame_free(&ost->filtered_frame); av_packet_free(&ost->pkt); av_dict_free(&ost->encoder_opts); diff --git a/fftools/ffmpeg_mux_init.c b/fftools/ffmpeg_mux_init.c index 7a2db9f0e8..2c0e2faf4a 100644 --- a/fftools/ffmpeg_mux_init.c +++ b/fftools/ffmpeg_mux_init.c @@ -1026,10 +1026,6 @@ static OutputStream *ost_add(Muxer *mux, const OptionsContext *o, av_strlcat(ms->log_name, "/copy", sizeof(ms->log_name)); } - ost->filtered_frame = av_frame_alloc(); - if (!ost->filtered_frame) - report_and_exit(AVERROR(ENOMEM)); - ost->pkt = av_packet_alloc(); if (!ost->pkt) report_and_exit(AVERROR(ENOMEM));