[FFmpeg-devel] [PATCH] all: replace qsort with AV_QSORT

[Ganesh Ajjanagadde]

On Sun, Oct 18, 2015 at 1:16 PM, Clément Bœsch <u at pkh.me> wrote:
> On Sun, Oct 18, 2015 at 11:12:03AM -0400, Ganesh Ajjanagadde wrote:
>> On Sun, Oct 18, 2015 at 11:03 AM, Clément Bœsch <u at pkh.me> wrote:
>> > On Sun, Oct 18, 2015 at 10:47:52AM -0400, Ganesh Ajjanagadde wrote:
>> > [...]
>> >> diff --git a/libavformat/subtitles.c b/libavformat/subtitles.c
>> >> index bb89766..16ab245 100644
>> >> --- a/libavformat/subtitles.c
>> >> +++ b/libavformat/subtitles.c
>> >> @@ -23,6 +23,7 @@
>> >>  #include "avio_internal.h"
>> >>  #include "libavutil/avassert.h"
>> >>  #include "libavutil/avstring.h"
>> >> +#include "libavutil/qsort.h"
>> >>
>> >>  void ff_text_init_avio(void *s, FFTextReader *r, AVIOContext *pb)
>> >>  {
>> >> @@ -197,9 +198,12 @@ void ff_subtitles_queue_finalize(void *log_ctx, FFDemuxSubtitlesQueue *q)
>> >>  {
>> >>      int i;
>> >>
>> >> -    qsort(q->subs, q->nb_subs, sizeof(*q->subs),
>> >> -          q->sort == SUB_SORT_TS_POS ? cmp_pkt_sub_ts_pos
>> >> -                                     : cmp_pkt_sub_pos_ts);
>> >> +    if (q->sort == SUB_SORT_TS_POS) {
>> >> +        AV_QSORT(q->subs, q->nb_subs, AVPacket, cmp_pkt_sub_ts_pos);
>> >> +    }
>> >> +    else
>> >> +        AV_QSORT(q->subs, q->nb_subs, AVPacket, cmp_pkt_sub_pos_ts);
>> >> +
>> >
>> > Weird style.
>>
>> There were two reasons for this:
>> 1. This ensures inlining of the cmp callback.
>> 2. I was not sure whether putting the cmp ternary inside the AV_QSORT
>> is safe since it is a macro. Did not want to check that, so used the
>> above.
>
> I was referring to the { } in one case but not the other, as well as the
> else not being on the same line of }.
>
>>
>> >
>> > BTW, AV_QSORT() Macro should be replaced with a do { ... } while (0) form
>> > to make this kind of code safer.
>>
>> Maybe, but that is separate from this patch.
>>
>
> Depends, it could have a functional impact depending on the way the
> if/else is done.

Carl's suggestion takes care of that with the addition of the braces.

>
>> >
>> > Also note that using these macro has an impact on final binary size which
>> > might not be worth the trouble in various cases.
>>
>> So how do you want me to measure binary size impact?
>
> [do changes]
> git stash
> make libavformat/subtitles.o
> ls -l libavformat/subtitles.o
> git stash pop
> make libavformat/subtitles.o
> ls -l libavformat/subtitles.o
>
>> Or more
>> concretely: which of the above replacements do you think is not
>> worthwhile (e.g due to small array size)?
>
> This code is not really speed relevant.

In some cases it is: quick glance at vf_deshake says it should: qsort
is being done on every frame.

>
> --
> Clément B.
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>