LINUX.ORG.RU
ФорумTalks

[ffmpeg] быдлокод детектед

 


0

1

Пытаюсь затянуть сабж в андроид. Попутно нарыл следующий код:

static int64_t nut_read_timestamp(AVFormatContext *s, int stream_index, int64_t *pos_arg, int64_t pos_limit){
    NUTContext *nut = s->priv_data;
    AVIOContext *bc = s->pb;
    int64_t pos, pts, back_ptr;
av_log(s, AV_LOG_DEBUG, "read_timestamp(X,%d,%"PRId64",%"PRId64")\n", stream_index, *pos_arg, pos_limit);

    pos= *pos_arg;
    do{
        pos= find_startcode(bc, SYNCPOINT_STARTCODE, pos)+1;
        if(pos < 1){
            assert(nut->next_startcode == 0);
            av_log(s, AV_LOG_ERROR, "read_timestamp failed.\n");
            return AV_NOPTS_VALUE;
        }
    }while(decode_syncpoint(nut, &pts, &back_ptr) < 0);
    *pos_arg = pos-1;
    assert(nut->last_syncpoint_pos == *pos_arg);

    av_log(s, AV_LOG_DEBUG, "return %"PRId64" %"PRId64"\n", pts,back_ptr );
    if     (stream_index == -1) return pts;
    else if(stream_index == -2) return back_ptr;

assert(0);
}

Оно как бы интуитивно понятно, что после assert(0); возвращаемое значение смысла не имеет, но компилеру то это невдомек.

Неужели было тяжело в этом месте написать чето такое, чтобы все компилеры не вопспринимали это место, как ошибку?

★★★

убери ненужное, перепиши, собери. если соберется и заработает - делай патч :)

Komintern ★★★★★
()

> Оно как бы интуитивно понятно, что после assert(0); возвращаемое значение смысла не имеет, но компилеру то это невдомек.

Когда ты соберешь программу с NDEBUG, у тебя после assert(0) будет возвращаться мусор. Твои пользователи будут наблюдать взглюки, которые хрен воспроизведешь. Компилер в данной ситуации прав.

Manhunt ★★★★★
()
Ответ на: комментарий от Manhunt

>Когда ты соберешь программу с NDEBUG, у тебя после assert(0) будет возвращаться мусор

Таки +1

Ушел писать в багтрекер

AF ★★★
() автор топика

assert(0) в данном случае означает, что при отсутствии багов данный код является недостижимым, поэтому совершенно не важно, что возвращает функция: если уж данная ветка каким-то образом попала под выполнение, код в целом скорее всего будет нерабочим, а assert подскажет разработчику (и только ему), что что-то важное в потоке выполнения программы изменилось.

Для совместимости с MSVC в таких случаях обычно ставят return, возвращающий произвольное значение, но т.к. ffmpeg заточен под gcc (и mingw в частности), то по части багрепорта ты, скорее всего, будешь совершенно справедливо послан, т.к. «быдлокода» тут нет. Обычный осознанный и неизбежный undefined behavior.

Если твой компилер ругается, то поставь после assert'a return 0 и не парься.

mannaz
()
Ответ на: комментарий от mannaz

> при отсутствии багов данный код является недостижимым

Ты правда считаешь, что в баги отсутствуют? И если они отсутствуют, то зачем было писать assert? :D

если уж данная ветка каким-то образом попала под выполнение, код в целом скорее всего будет нерабочим


Вот только из-за возврата мусора «нерабочесть» у него будет проявляться очень нестабильно: то так, то этак.

Обычный осознанный и неизбежный undefined behavior.


Избежный. Что мешает написать хотя бы abort()?

Manhunt ★★★★★
()
Ответ на: комментарий от mannaz

>Для совместимости с MSVC в таких случаях обычно ставят return, возвращающий произвольное значение, но т.к. ffmpeg заточен под gcc...

У меня этот код отказался компилить именно gcc, так что мимо!

Я бы еще стерпел хак ради производительности или экономии памяти. Но конкретно в этом месте отсутствие return ничего полезного не дает.

AF ★★★
() автор топика
Ответ на: комментарий от mannaz

Ога, а возвращается при этом смещение. Феерические глючи и несколько дней отладки гарантированы. Поэтому это именно быдлокод. И таких косяков в ffmpeg очень много.

Reset ★★★★★
()

> if (stream_index == -1) return pts;

else if(stream_index == -2) return back_ptr;

assert(0);



Хм, я-то хреново знаю Ц, но если stream_index нечаянно не -1 и не -2, то будет почти гарантированно порча данных либо сегфолт, не так ведь?

Во-первых, никто не гарантирует, что вызывающая функция не передаст ерунду. Во-вторых, если работа с другими значениями stream_index не предусмотрена, я бы вящего спокойствия ради запихнул туда еще один assert((stream_index == -1) || (stream_index == -2)). Мало ли.

shimon ★★★★★
()
Ответ на: комментарий от true_admin

А ты чем собираешь?

Target: arm-eabi
Configured with: ../../../android-toolchain/gcc-4.4.0/configure --prefix=/usr/local --target=arm-eabi --host=i686-unknown-linux-gnu --build=i686-unknown-linux-gnu --enable-languages=c,c++ --with-gmp=/home/jingyu/projects/gcc/toolchain_build/obj_bcpl/temp-install --with-mpfr=/home/jingyu/projects/gcc/toolchain_build/obj_bcpl/temp-install --disable-libssp --enable-threads --disable-nls --disable-libmudflap --disable-libgomp --disable-libstdc__-v3 --disable-sjlj-exceptions --disable-shared --disable-tls --with-float=soft --with-fpu=vfp --with-arch=armv5te --enable-target-optspace --with-abi=aapcs --with-gcc-version=4.4.0 --with-binutils-version=2.19 --with-arch=armv5te --with-sysroot=/g/users/jingyu/toolchain/cupcake_rel_root --with-gmp-version=4.2.4 --with-mpfr-version=2.4.1 --with-gdb-version=6.6 --with-multilib-list=mthumb-interwork,mandroid --program-transform-name='s,^,arm-eabi-,'
Thread model: single
gcc version 4.4.0 (GCC) 
AF ★★★
() автор топика

Я в таких местах вставляю myassert(0, «problem description») который не зависит от NDEBUG :)

true_admin ★★★★★
()
Ответ на: комментарий от AF

Перечитал твой пост, я думал у тебя в этом месте не собирается.

Но, в общем, ffmpeg весь состоит из костылей и подпорок. Плюс ко всему нет документации и его часто ломают.

true_admin ★★★★★
()
Ответ на: комментарий от Reset

несколько дней отладки гарантированы

Ну если мысль собрать кривой код без NDEBUG до тебя дойдет только на третий день, то да, безусловно. Недели отладки.

mannaz
()
Ответ на: комментарий от AF

> У меня этот код отказался компилить именно gcc, так что мимо!

Этих gcc, как грязи. Может у тебя в параметрах сборки какой-нибудь -Werror стоит. А вообще, есть конкретные стандарты для C и C++ и они конкретно ситуацию по отсутствию return в функциях описывают. GCC этим стандартам вполне следует.

mannaz
()
Ответ на: комментарий от pevzi

> А как с этим в Libav например?

думаю очень ужасно, они же его недавно только форкнули.

Чтобы не быть голословным, приведу ряд проблем с которыми пришлось столкнуться.

1) Некоторые декодеры генерят pts, некоторые нет.
2) Многие поля многих структур недокументированы, приходится лезть в исходники и смотреть какие кодеки и как их используют.
3) Есть поля которые нужны, например, только ffmpeg.c(cli-тулза которая только конвертирует видео).
4) Различные кодеки требуют различных костылей, это очень хорошо видно, опять таки, по ffmpeg.c(который, по идее, должен быть всего в пару строчек)
5) внутренняя логика программы размазана по сырцам, есть дублирующий код, есть куча непонятных мест. Например, зачем нужны stream->sample_aspect_ratio и stream->codec->sample_aspect_ratio которые ещё могут и не совпадать?
6) Некоторые поля могут быть вообще не заполнены, хотя они нужны для воспроизведения. Например какой-нить stream->avg_frame_rate. Скорее всего это опять таки связано с кривыми кодеками.

итд итп. В общем, без gdb, valgrind и постоянного лазанья в исходники ничего путного с ffmpeg и его форками не напишешь. И большинство результирующего кода это борьба с ffmpeg и обходом различных багов. Кроме того ffmpeg постоянно внутри ломают и переделывают, API нестабилен. И никогда не знаешь наверняка кто виноват в глюках-кодек или твой код в котором надо вставить вызов какой-нить недокументированной фичи.

gstreamer тоже не фонтан :(.

true_admin ★★★★★
()
Ответ на: комментарий от shimon

если stream_index нечаянно не -1 и не -2, то будет почти гарантированно порча данных либо сегфолт, не так ведь?

нет, assert(0) гарантированно вызовет abort() и всё будет хоккей

backbone ★★★★★
()
Ответ на: комментарий от backbone

но только с NDEBUG, да... видимо, неслучайно табуляция перед assert(0); пропущена, планировали убрать наверное =)

backbone ★★★★★
()
Ответ на: комментарий от backbone

> нет, assert(0) гарантированно вызовет abort() и всё будет хоккей

Так его же еще включать специально надо. abort(), ага.

shimon ★★★★★
()
Ответ на: комментарий от backbone

> в отладке же

что «в отладке же»? я не разработчик ffmpeg, я пользователь. или при сборке релиза там внезапно появится корректный код?

arsi ★★★★★
()
Ответ на: комментарий от shimon

я в пролёте =) и assert() - вообще макрос, да и спать пора, видимо =)

backbone ★★★★★
()
Ответ на: комментарий от arsi

или при сборке релиза там внезапно появится корректный код?

считается что в релизных сборках туда управление не передаётся :). Кстати, ещё одно доказательство быдлокода: магические переменные -1 и -2. Причём ещё и с отрицательным индексом, хотя все эти idx должны быть неотрицательными т.к. это индекс в массиве. Т.е. они используют переменные не по назначению и хрен где это задокументировано. Убить за это мало. Впрочем, этот код выглядит просто огрызком, возможно он нигде реально не используется(лень проверять).

Короче, я уверяю, assert это самая наименьшая проблема этого кода :)

true_admin ★★★★★
()
Ответ на: комментарий от true_admin

> Ребят, ffmpeg такая глючная, падучая штука и дырявая штука, а вас пугает какой-то assert :)

нет, меня не пугает ассерт; меня пугает код, когда я роюсь в нем, пытаясь восполнить пробелы в доке по апи ;)

arsi ★★★★★
()
Ответ на: комментарий от true_admin

> считается что в релизных сборках туда управление не передаётся :).

не, ты не понял) я и сам часто промышляю подобным, т.е. вызовом внутренних функций с заранее известным и гарантированным диапазоном значений… но!

    if     (stream_index == -1) return pts;
    else if(stream_index == -2) return back_ptr;

> else if
> … if

последнее условие однозначно подразумевает наличие других вариантов. но их нет, функция тупо выходит без возвращаемого значения. это с точки зрения компилятора, да. более-менее корректный код мог бы быть примерно таким:

assert(stream_index == -1 || stream_index == -2);
if (stream_index == -1)
	return pts;
else
	return back_ptr;

и компилятор счастлив и в дебаг-, и в релиз-режиме.

arsi ★★★★★
()
Ответ на: комментарий от true_admin

>Ребят, ffmpeg такая глючная, падучая штука и дырявая штука, а вас пугает какой-то assert :)

Не пугает, а заставляет негодовать по поводу вменяемости разработчиков

И как бы ffmpeg не был написан, замены ему нет :(

ЗЫ: А че gstreаmer не понравился? Багами или идеями?

AF ★★★
() автор топика

Давайте каждый раз натыкаясь на былокод срать на ЛОРе, я так в одну каску его могу засрать охренеете блин.

erfea ★★★★★
()
Ответ на: комментарий от mannaz

>Этих gcc, как грязи.

FFmpeg собирается только каноничным gcc? O_o

Может у тебя в параметрах сборки какой-нибудь -Werror

Если бы там был -Werror, то сборка грохнулась бы еще раньше.

GCC этим стандартам вполне следует

От к gcc у меня никаких претензий нету. Да и не в стандарте тут дело, а в здравом смысле.

AF ★★★
() автор топика
Ответ на: комментарий от arsi

Лучше тогда уж

if (stream_index == -1)
   return pts;
elif (stream_index == -2)
   return back_ptr;
else
   errx(1, "invalid argument");

Но всё равно это злейший быдлокод который возвращает picture timestamp(целое число) или УКАЗАТЕЛЬ в зависимости от ИНДЕКСА потока. Поубивав бы.

true_admin ★★★★★
()
Ответ на: комментарий от AF

> А че gstreаmer не понравился? Багами или идеями?

У меня стоит gstreamer 0.10.30(убунта 10.10), так вот далеко не всегда playbin(http://pygstdocs.berlios.de/pygst-tutorial/playbin.html) корректно определяет параметры потоков. Ну и сложно там всё. А так толком не ковырял ещё.

true_admin ★★★★★
()

Бу-га-га!

Сегодня получил ответ от разработчиков

Comment:

 The last part of the function is:



    if     (stream_index == -1) return pts;

    else if(stream_index == -2) return back_ptr;



 assert(0);

 }



 So the function will either return a value or assert, because the

 condition is unexpected, so I don't believe there is an error here. Maybe

 some graceful recovery code could be put, but if there are no samples

 raising the assert condition (and possibly it is not even possible, unless

 there are programming errors), then I believe the issue can be safely

 closed.



 Let us know if you if you don't agree or if there are samples which can

 raise the assert.
AF ★★★
() автор топика
Вы не можете добавлять комментарии в эту тему. Тема перемещена в архив.