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

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Ramiro Polla:
> Hi Andreas,
> 
> On Mon, Mar 3, 2025 at 2:12 PM Andreas Rheinhardt
> <andreas.rheinhardt@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 01/30] avcodec/mpegvideo_enc: Don't set qscale_table value prematurely
> There's a slight phrasing issue in the first paragraph of the commit
> message: "it copies the keeps several sets".

Will remove the "copies the".

> 
> - [PATCH 03/30] avcodec/motion_est: Avoid branches for put(_no_rnd) selection
> Could you please add to the commit message that [hq]pel_put are
> already being set in ff_me_init_pic()?

Will do.

> 
> - [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.

> - [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.

> 
> - [PATCH 15/30] avcodec/mips/mpegvideo_mmi: Don't check alternate_scan
> unnecessarily
> Thanks!
> 
> - [PATCH 20/30] avcodec/vc1: Add max_b_frames field to VC1Context
> Could you please copy the comment for the new field in vc1.h as well?

Will copy it with the "for encoding" removed. The latter part is why I
did not copy it; IMO the name speaks for itself.

> 
> - [PATCH 26/30] avcodec/mpeg4videodec: Avoid copying packed bitstream data
> Could you please note in the commit message that this was done using
> referenced AVBuffers?

Will do.

Will also drop patch 16/30 (removing h263_aic for RV20), because said
value affects how intra_ac_vlc_length is set in ff_h263_encode_init().
(Given that h263_aic alternates for RV.20 I wonder whether
intra_ac_vlc_length should not also be set on a per-frame basis.)

- Andreas

_______________________________________________
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".