[FFmpeg-devel] [PATCH] libavfilter: temporarily remove DNN framework and vf_sr filter

[Rostislav Pehlivanov]

On 27 July 2018 at 17:48, Pedro Arthur <bygrandao at gmail.com> wrote:

> 2018-07-27 12:07 GMT-03:00 Rostislav Pehlivanov <atomnuker at gmail.com>:
> > On 27 July 2018 at 15:11, James Almer <jamrial at gmail.com> wrote:
> >
> >> On 7/27/2018 8:04 AM, Rostislav Pehlivanov wrote:
> >> > Reference - IRC and j-b's earlier email.
> >> > Coding style issues:
> >> > DNNModel* ff_dnn_load_model_native(const char* model_filename)
> >> >
> >> > We never ever do stupid things like put the asterix first. The author
> of
> >> > the patch should have known better and the patch should have been
> >> checked.
> >> > Even a glance could have told you its wrong.
> >>
> >> Tone it down. It's a style issue. New contributors don't always know
> >> things like that and we always tell them to fix it. It's the reviewer
> >> who should have pointed it out, and if they missed it then it's
> >> harmless. It's not like he used a public suffix like av_, where it can
> >> be a problem.
> >>
> >
> > It shows the code wasn't reviewed properly. These style issues are
> > propagated throughout all the DNN framework, which was many thousands of
> > lines of code and shows that not even a glimpse was spent on the actual
> > code. We don't commit such huge patches without at least some form of
> > review, even if months have passed and no one has bothered to yet.
> The style issue is my fault, I did not ensure the student were aware
> of the coding style.
> That does not mean that other aspects of the code were not properly
> reviewed, I spent a good amount of time reviewing and testing the
> code.
>
The code style issue is easily fixable, the student will already
> provide a fix, but if it is too much urgent I could fix it myself in a
> few minutes.
>

Doesn't matter how easy it is to fix, its too late, the code landed, you
pushed it and you didn't even bother to look at it. This is unacceptable,
especially for such a big patchset, and unfair to the rest of developers.
Ping the patchset, and keep pinging it until someone reviews it. Don't make
it your authority to do so especially as its new code for which no
maintainers exist.
And the coding style is just the tip, there are dozens of things I disagree
with, such as the custom float to 8 bit conversions, when filters already
exist which take in floats (zscale).


I don't see how reverting the code and asking for "proper" review,
> which no one (in almost 3 months) besides me did, will do any good.
> After reverting will you be compromised (or anyone else) to review it?
> or the code will get forgotten waiting for review until no one cares
> about it?
>

Yes, I'll review it myself.


It is not fair to propose a patch, and push it within 24h, reverting
> the student work of almost 3 months where no one has complained before
> of issues to be fixed.
>

What isn't fair is that this literal unreviewed code dump is sitting in our
repository at all, without review from any developer, but only with the
reassurance that it compiles and that it mostly does the things it should.
I don't want to keep pointing out issues upon issues with code in our
repository, a nice clean start is what's needed, which means like I said a
temporary revert. I'd rather have that than a tens of patches needed to
actually fix the whole thing, when a few well formatted emails would
suffice.


The code style issue will be addressed.
> We already provide a repo with training scripts, and it will be
> referenced in the code.
>
> Objectively, what else needs to be fixed? let me know and give the
> student at least a few days so he can provide a fix.


Remove the internal tables. All of them.