On Mon, Sep 9, 2013 at 3:41 PM, Hendrik Leppkes <[email protected]> wrote: > On Mon, Sep 9, 2013 at 3:37 PM, Vittorio Giovara <[email protected] >> wrote: > >> --- >> I performed some more benchmarking and the results at higher bitrates show >> that the current implementation is better (by a tiny amount anyway). So I >> rebased and reimplemented my change just as code simplification, without >> modifying the inline attribute of the internal function. >> >> Cheers, >> Vittorio >> >> libavcodec/mpeg12enc.c | 9 +++------ >> 1 file changed, 3 insertions(+), 6 deletions(-) >> >> diff --git a/libavcodec/mpeg12enc.c b/libavcodec/mpeg12enc.c >> index 2010d2a..a616f08 100644 >> --- a/libavcodec/mpeg12enc.c >> +++ b/libavcodec/mpeg12enc.c >> @@ -602,13 +602,13 @@ next_coef: >> >> static av_always_inline void mpeg1_encode_mb_internal(MpegEncContext *s, >> int16_t >> block[6][64], >> - int motion_x, int >> motion_y, >> - int mb_block_count) >> + int motion_x, int >> motion_y) >> { >> int i, cbp; >> const int mb_x = s->mb_x; >> const int mb_y = s->mb_y; >> const int first_mb = mb_x == s->resync_mb_x && mb_y == s->resync_mb_y; >> + const int mb_block_count = s->chroma_format == CHROMA_420 ? 6 : 8; >> >> /* compute cbp */ >> cbp = 0; >> @@ -881,10 +881,7 @@ static av_always_inline void >> mpeg1_encode_mb_internal(MpegEncContext *s, >> void ff_mpeg1_encode_mb(MpegEncContext *s, int16_t block[6][64], >> int motion_x, int motion_y) >> { >> - if (s->chroma_format == CHROMA_420) >> - mpeg1_encode_mb_internal(s, block, motion_x, motion_y, 6); >> - else >> - mpeg1_encode_mb_internal(s, block, motion_x, motion_y, 8); >> + return mpeg1_encode_mb_internal(s, block, motion_x, motion_y); >> } >> >> av_cold void ff_mpeg1_encode_init(MpegEncContext *s) >> -- >> 1.7.9.5 >> > > The whole point of this function was to make mb_block_count a constant when > inlined, which your patch destroys. > I see no real point in this "improvement".
Well, I believe that the benefit/impact of inline is not to 'constantize' something but rather copying blocks of code instead of calling the function. In the current implementation, the assembly would create a ff_mpeg1_encode_mb which contains mpeg1_encode_mb_internal twice. No matter of const for the last argument, you always create a function that is twice as big than with my "improvement", which is slower to load and harder to optimize (again, despite the const-ing of the last argument). If you need proof for this check the file size: without: -rw-rw-r-- 1 vittorio vittorio 46800628 Sep 9 16:34 libavcodec/libavcodec.a with: -rw-rw-r-- 1 vittorio vittorio 46765784 Sep 9 16:35 libavcodec/libavcodec.a 30k of saved codesize from a 1 line change is not too bad, I'd say, on the contrary it is an "improvement". And the speed impact is negligible (both in favour and against). Cheers, Vittorio > _______________________________________________ > libav-devel mailing list > [email protected] > https://lists.libav.org/mailman/listinfo/libav-devel _______________________________________________ libav-devel mailing list [email protected] https://lists.libav.org/mailman/listinfo/libav-devel
