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 <[email protected]> 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.
signature.asc
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list [email protected] http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
