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

Clément Bœsch u at pkh.me
Sun Oct 18 19:16:23 CEST 2015


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.

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

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151018/44d2423b/attachment.sig>


More information about the ffmpeg-devel mailing list