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

[Andreas Rheinhardt]

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)?

Will do.

- Andreas