[FFmpeg-devel] [PATCH 01/30] avcodec/mpegvideo_enc: Don't set qscale_table value

[Ramiro Polla]

On Tue, Mar 4, 2025 at 12:37 AM Andreas Rheinhardt
<andreas.rheinhardt at outlook.com> wrote:
> Ramiro Polla:
> > On Mon, Mar 3, 2025 at 2:12 PM Andreas Rheinhardt
> > <andreas.rheinhardt at outlook.com> wrote:
> >>
> >> Andreas Rheinhardt:
> >>> Patches attached; the first few were already sent last year [1], but I
> >>> have refrained from pushing them because Michael seems to have trouble
> >>> testing them due to a conflict [2] that existed by the time he tried to
> >>> test it.
> >>>
> >>> - Andreas
> >>>
> >>> [1]: https://ffmpeg.org/pipermail/ffmpeg-devel/2024-July/330467.html
> >>> [2]: https://ffmpeg.org/pipermail/ffmpeg-devel/2024-September/333737.html
> >>>
> >> Will apply this patchset tomorrow unless there are objections.
> >
> > A few nits:
> >
[...]
> >
> > - [PATCH 06/30] tests/fate/vcodec: Test alternate_scan
> > Can't we add a new test for alternate_scan instead? It's not that
> > important though.
> >
>
> Why? I don't see a downside to my approach (everything that is already
> covered will still be covered, plus a bit more). But there is one to
> yours: Several more tests. I run fate because of its coverage and not to
> run fate, so if one can achieve the same coverage with less tests (or
> with faster tests), then less tests are preferable.

That sounds reasonable. I sometimes ask myself how important each of
the flags are in some tests, but in this case, along with the commit
message, it's simple enough to understand, and the rest of the tests
still use normal scan.

> > - [PATCH 10/30] avcodec/h261dec: Inline constant
> > Could you please add to the commit message that y_dc_scale was set in
> > mpeg1_decode_picture(), and maybe even that it will be removed in the
> > following commit?
>
> y_dc_scale was not set in mpeg1_decode_picture() for the H.261 decoder
> (because that function is only for the MPEG-1/2 decoders). It was
> effectively set in ff_set_qscale() (the calls to which (in H.261) I will
> remove in my next patchset); notice that the dc_scale table (namely
> ff_mpeg1_dc_scale_table) is constant (namely 8) here.

Thanks for the explanation. Could you add some of that information to
the commit message (basically where that 8 comes from)?