О программистах и пользователях

lexa - 03/Сен/2017 15:26

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

Вот есть такая библиотека декодирования RAW, RawSpeed. Хорошая, в том смысле что быстрая, местами сильно быстрее LibRaw, местами - не сильно. Ее сначала разрабатывал Клаус Пост, а потом, весной где-то, передал текущей команде darktable.

И вот ее теперешний майнтайнер берет и одним движением вставляет дополнительную проверку на размер изображения, например вот эту: https://github.com/darktable-org/rawspeed/commit/22fe377b34c955a9cc5006bfd8755dd8ba2b9ab9

Что произошло:

  1. Одним движением сломана совместимость с Nikon D850, когда будет добавляться поддержка этой камеры - придется править код (и это будет единственное изменение). Она уже сломана (без этой проверки D850 декодируется).
  2. Практически гарантированно поломана совместимость с другими будущими камерами - потому что вставлены лимиты, отвечающие камерам сегодняшним, а рост мегапикселей гарантированно продолжится дальше.

Нет, я не поленился и задал вопрос, "а зачем ТАК", ответ был следующим:

It is a balance between

  • allowing efficient fuzzing (which is revealing ridiculous amount of security problems),
  • not introducing any fuzzer-specific restrictions that make processing of the normal raws impossible,
  • not bitrotting those restrictions.

Ну и дальше там состоялась целая дискуссия, можете почитать (она под другим изменением, аналогичным, для ORF-файлов; это изменение сейчас ничего не ломает, но с гарантией сломает когда выйдет очередной Олик).

Надо сказать, что в RawSpeed вообще любят хардкодить константы, но до сего дня они их хардкодили в данные (cameras.xml), в частности так поступали с уровнем черного (даже для тех камер, где его можно взять из метаданных), что плохо и вообще и в частности:

  • вообще: зачем хардкодить что-то пусть даже и в данные, если оно есть в метадате? Выйдет новая камера - и вам нужны будут все примеры на всех режимах, а если брать из метадаты - то нет.
  • в частности: в природе есть камеры у которых уровень черного вот прямо на скаку меняется (Панасоники, к примеру), если вы хардкодите, то у вас будет совершенно ненужная нелинейность в тенях.

Ну а хардкодинг прямо в коде (и не в таблицах) - я, конечно, вижу не впервые, но считал это совсем какой-то плохой практикой. Теперь так носят? Чтобы фаззеру сделать проще? Серьезно?

Но хочу я сказать и более глобальную вещь тоже:

  • Если ничего не хардкодить, а стараться читать из метадаты.
  • То (многие, далеко не все) новые камеры просто работают со старым софтом, без каких-то его модификаций.
  • К сожалению, полностью так нельзя сделать т.к. те же цветовые данные - часто внешние, в метадате может не быть.
  • Но хотя бы частично можно.

И вот то, что разработчики (в данном случае - darktable) к этому не стремятся, а стремятся ровно к обратному - мне сильно удивительно. Ведь есть ненулевое количество таких пользователей, которые только-что купили совершенно новую камеру, а старый софт с ней никак не работает. И вот тут есть возможность их заполучить. Что, darktable/RawSpeed не нужны пользователи? Ну-ну....

Comments

ib - 03/Сен/2017 16:59

Кроме "купил новую камеру" есть важный (для некоторых из нас, по крайней мере) граничный случай: получил от компании мула на тестирование. Мул - это камера, которая выглядит достаточно похоже на серийную модель, но с отдельными элементами, которые тестируются для будущей модели. Такими элементом часто бывает сенсор :) Мулы и pre-production camera samples обычно пишут raw, но софта для его процессинга часто нет (случай c тем же D850). Куды крестьянину податься? Кто-то пошлет образцы в Adobe, но если поймают - накажут. А вот поддержка в third-party software - позволяет как избежать риска быть пойманным, так и вообще оставаться порядочным исполнителем соглашения о неразглашении.

adipocerebrum - 03/Сен/2017 18:03

> NefDecoder::decodeRawInternal() {
> + if (width == 0 || height == 0 || width > 7424 || height > 4928)

А что, там на каждый формат - вот так вот прямо внутри декодилки везде константы захардкодены?

lexa - 03/Сен/2017 18:22

Вы не поверите, но да:
https://github.com/darktable-org/rawspeed/commits/develop?after=6c2f70d1...

(не на все, конечно. DNG нет)

AlexeyD - 03/Сен/2017 18:26

C точки зрения софтверного разработчика - это идиотизм...

adipocerebrum - 03/Сен/2017 18:26

Дать бы ему молотком по голове...

lexa - 03/Сен/2017 18:26

Я, пожалуй, не готов такое видеть в коде даже в виде таблицы.

Я готов видеть такое в данных (cameras.xml) в виде камера=>список допустимых размеров (хотя и там бывают приколы, вероятно, после апдейта фирмвары), но с отдельным ключиком "что делать, если таки не" (а не просто исключение бросать).

А в таком виде - это просто ужас на крыльях.

И что удивительнее всего - этот ужас привнесен человеком, который *вроде бы* привносит в проект хорошие практики. CI, фаззер, C++14, всякие там правильные проверки. И вообще видно, что улучшает код.
Только вот иногда такое улучшение противоречит практике.

AlexeyD - 03/Сен/2017 18:22

Не ну а чему удивляться - большинство opensource примерно так же делают ("облегчают" себе жизнь в ненужных местах).

lexa - 03/Сен/2017 18:27

hardcoded-константы в куче мест - это усложнение жизни.

AlexeyD - 03/Сен/2017 18:33

Конечно - поэтому и написал в кавычках

adipocerebrum - 03/Сен/2017 18:29

Хехе... судя по моему куцему опыту, проприетарщики от опенсорсников в этом не отличаются.
Более того, у них еще больше развиты скиллы заметания какашек под ковёр, а так же "хуяк-хуяк, и в продакшен".

lexa - 03/Сен/2017 18:32

Ну я (кстати) согласен.

Если код виден людям, то многие вещи просто стыдно делать, это как в лифте пукать.

AlexeyD - 03/Сен/2017 18:39

Смотря как процесс организован. Я например своим разработчикам на code review за такое руки отбиваю.

lexa - 03/Сен/2017 18:54

Ну вот если обстоятельства сложились (дедлайн был вчера, правка хуяк-хуяк решает проблему, а разумный способ - долгий) - то поди раз в пятилетку то можно....

Roman Lebedev (not verified) - 03/Сен/2017 18:32

https://github.com/darktable-org/rawspeed/blob/develop/LICENSE#L435-L456

NO WARRANTY

15. BECAUSE THE LIBRARY IS LICENSED FREE OF CHARGE, THERE IS NO
WARRANTY FOR THE LIBRARY, TO THE EXTENT PERMITTED BY APPLICABLE LAW.
EXCEPT WHEN OTHERWISE STATED IN WRITING THE COPYRIGHT HOLDERS AND/OR
OTHER PARTIES PROVIDE THE LIBRARY "AS IS" WITHOUT WARRANTY OF ANY
KIND, EITHER EXPRESSED OR IMPLIED, INCLUDING, BUT NOT LIMITED TO, THE
IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
PURPOSE. THE ENTIRE RISK AS TO THE QUALITY AND PERFORMANCE OF THE
LIBRARY IS WITH YOU. SHOULD THE LIBRARY PROVE DEFECTIVE, YOU ASSUME
THE COST OF ALL NECESSARY SERVICING, REPAIR OR CORRECTION.

16. IN NO EVENT UNLESS REQUIRED BY APPLICABLE LAW OR AGREED TO IN
WRITING WILL ANY COPYRIGHT HOLDER, OR ANY OTHER PARTY WHO MAY MODIFY
AND/OR REDISTRIBUTE THE LIBRARY AS PERMITTED ABOVE, BE LIABLE TO YOU
FOR DAMAGES, INCLUDING ANY GENERAL, SPECIAL, INCIDENTAL OR
CONSEQUENTIAL DAMAGES ARISING OUT OF THE USE OR INABILITY TO USE THE
LIBRARY (INCLUDING BUT NOT LIMITED TO LOSS OF DATA OR DATA BEING
RENDERED INACCURATE OR LOSSES SUSTAINED BY YOU OR THIRD PARTIES OR A
FAILURE OF THE LIBRARY TO OPERATE WITH ANY OTHER SOFTWARE), EVEN IF
SUCH HOLDER OR OTHER PARTY HAS BEEN ADVISED OF THE POSSIBILITY OF SUCH
DAMAGES.

AlexeyD - 03/Сен/2017 18:37

Это понятно. Но вот Вы если библиотеку делаете то надеетесь что ее использовать другие будут иначе смысл всего этого? Если делаете так делайте полноценно а не спустя рукава.

lexa - 03/Сен/2017 18:49

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

Сколько вот практику разработки, так уж получилось что на вашем примере (опенсорс, что уж там).

И раз уж мы можем тут обсуждать на русском (мой же блог), то мне проще выразиться аккуратнее, чем на английском (где я ньюансы могу попутать).

А хочу я сказать простые вещи (и раз уж вы подвернулись прочитать это на русском - выскажу уж все, что я думаю про RawSpeed), часть их написана выше, но я повторю уж в одном месте.

  1. Ну хардкодить константы - ну вообще плохо. Просто вот плохая практика. M_PI еще можно (да и то), а уж хардкодить то, что прямо вот на днях менять (предельные размеры NEF) - ну вообще ни в какие ворота. Еще как-то можно было бы принять это в виде таблички, общей на библиотеку, но не в виде отдельных правок, по одной на формат, да и то. В виде cameras.xml - ну вообще прекрасно. Это, я впрочем, вам уже писал.
  2. Ломать совместимость (с D850) - даже если камера формально не поддерживается - чтобы фаззер был счастлив - ну это вообще какая-то зацикленность на процессе разработки, а не на пользователе. А приоритетом должен быть все-таки пользователь.
    Почему поддержка "несуществующей" камеры может быть полезна - написано в первом комментарии под моим текстом. Это не говоря о том, что пользователи с удовольствием ковыряют опубликованные тестерами RAW от свежих камер - и если вы (как разрабочик) дали им такое средство - они с бОльшей вероятностью переключатся на ваш продукт.
    Понятно, что опенсорс-разработчики (многие) стараются не для пользователей, но блин....
  3. Про хардкодинг других значений - написано выше.
    Если вас интересует результат, а не процесс - разбирать метадату все-таки правильнее, чем хардкодить, тогда с изрядной вероятностью новые камеры будут работать сразу.
  4. Вообще же, я с большим огорчением смотрю на эволюцию проекта, потому что многие вещи делаются - как мне кажется со стороны - ради процесса разработки, а не ради результата (а результат - удовлетворение пользователей, а не разработчиков). То есть понятна радость C++-программиста, что он может взять и написать все на C++14, но это ограничивает круг допустимых компиляторов - и, следовательно, круг пользователей.

А смотрю в ваш код (и огорчаюсь) я по той простой причине, что проект мне не безразличен, он и исходно очень хороший и много всего туда привнесено нового-хорошего, в период "после Клауса". То есть вся вот эта критика, начиная с сегодняшнего крика души в комментариях на github/в моем блоге - это от огорчения, что вот хорошую вещь - для совершенно вторичной цели тестирования фаззером - прямо на глазах портят.

Цель же - не пройти все круги испытаний фаззером, а дать пользователям инструмент (darktable) с которым они могут работать. Фаззер - для end-user-ских приложений все-таки вторичен.

AlexeyD - 03/Сен/2017 19:01

> Фаззер - для end-user-ских приложений все-таки вторичен.
Как end-user скажу - меня он вообще не интересует. Это средство разаботки и тестирования по-моему.

lexa - 03/Сен/2017 19:06

Он крайне полезен вообще (то есть в LibRaw нормально так выловили проблем им).

Но, конечно, для raw-процессинга, пока речь не идет о сервисе, который принимает произвольные файлы из интернета, а о юзере (с собственными файлами же), он даже не вторичен, а четвертичен.

lexa - 03/Сен/2017 19:05

Мне тут передают ваши цитаты из IRC через фейсбук :). У меня нету IRC не могу проверить и ответить тоже не могу.

Таки да, риск обидеть хорошего человека - сработал.

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

AlexeyD - 03/Сен/2017 19:29

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

lexa - 03/Сен/2017 19:38

Ну в этом смысле, да, я (потенциальный) пользователь, пора эту версию прикручивать как-то аккуратно, там декодеры ljpeg реально быстрее.

ib - 03/Сен/2017 21:01

А представьте себе, что выслушивают фотографы, не позаботившиеся выставить свет так, чтобы клиенту нравилось. Мягкий вариант - "а почему на Вашем снимке два носа?" И ничего, извиняемся и переснимаем, со срочностью. Объяснение "это свет бабочкой, стиль такой" не проканает.

lexa - 03/Сен/2017 21:07

Два носа в бесконечное количество раз лучше отсутствия носа!

adipocerebrum - 03/Сен/2017 20:08

> многие вещи делаются - как мне кажется со стороны - ради процесса разработки, а не ради результата (а результат - удовлетворение пользователей, а не разработчиков)

А это уже не первый год мода такая - программизм ради программизма.
"О, новая версия фреймворка %s! Давайте всё под него переделаем. О! Гугель-шмугель выкатил бета-версию языка %s - давайте весь проект на нём перепишем! Что?! Как это интерфейс неудобный?! Он же сгенерирован с помощью %s! У программистов ушло на 0.05 человеко/часа меньше времени, чем ежели был бы %s!
О! Давайте используем виджеты из библиотеки %s! Да пофиг, что бесплатная лицензия только для некоммерческого использования - как они узнают-то?"

lexa - 04/Сен/2017 11:01

А вторая сторона "будем использовать только Turbo C++, потому что деды так делали". Или Фортран-4.

Новые технологиии осваивать надо, опенсорсная библиотека с (очень) небольшим количеством пользователей - это прекрасный вариант для такого освоения. Тут я ничего против сказать не могу (хотя некомпилируемость теми версиями компилятора MS, которые я использую в production - огорчает, но я переживу).

Я возражаю именно против частных некорректных практик (собственно, она одна, хардкодинг, сначала в данных, а вот теперь и в коде)

mtve (not verified) - 04/Сен/2017 12:23

а нельзя ли просто захардкодить в 100 раз бОльшие константы, что бы и фаззер был счастлив и человеки? что бы например W*H был больше i32.

lexa - 04/Сен/2017 13:19

Ну если под i32 имеется в виду u32, то это в меру бессмысленное упражнение будет, потому что размеры в файле (часто/обычно, лень проверять) - 16 битные и больше чем 2^32 не будет в любом случае.

Сама по себе проверка для файлов "из камер" имеет смысл (размеры если и меняются с обновлением FW, то это бывает ну сильно редко), но тогда уже полноценная, make+model=>список размеров (хотя вот возможно что двуформатные камеры, типа пентаксов, имеют два списка размеров, никогда не задумывался). Я имею в виду "на практике имеет смысл", чтобы битые файлы отвергать как можно раньше.

Второе применение такого лимита - это (слегка) застраховаться от out of memory (в случае битых файлов). Тогда можно какой-то вменяемый лимит поставить, ну там 300Mpix или гигапиксель, и забыть про это место на несколько лет (пока какой-нибудь олимпус не сделает склейку панорам в камере в raw).

Меня в данной конкретной истории изумляет не то, что это вот было сделано (проверка имеет смысл, повторяю), а способ реализации проверки.

lexa - 04/Сен/2017 13:21

Заметим в скобках, что таблица размеров для make-model поломает часто применяемый пользователями способ таки скормить файл (от новой камеры) конвертору (который эту новую камеру не поддерживает и отвергает "по списку поддерживаемых") - меняют make/model в EXIF и начинает жрать.

Что, наверное, тоже плохо. То есть такая проверка должна отключаться на рантайме (и end-user sw должно иметь для этого рукоятку)

adipocerebrum - 04/Сен/2017 15:30

Можно же сделать так, чтоб не поломало. "Нет камеры в БД - не проверяем размер". Ну, или проверяем, но на совсем уж дикие значения. Например, чтоб ширина/высота не превышали более чем в три раза размеры от самой развесистой камеры того же производителя.

lexa - 04/Сен/2017 15:40

Ну вот свежий пример D850. Чтобы открыть тамошние NEF-ы адобовскими тулами - меняют модель на D750 или на D500.

В обоих случаях - если мы проверяем размеры - файл будет отвергнут т.к. размер не соответствует D750/D500

mtve (not verified) - 04/Сен/2017 13:28

> 16 битные и больше чем 2^32 не будет в любом случае.

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

я предложил ограничить примерно на 20-ти битах, как раз что бы перемножение размеров было больше 31-го бита, но если Вы считаете что можно ограничить ниже, не буду спорить. основывался на том что разрешение человеческого зрения вроде примерно на границе 16, так что лучше с запасом, и с учётом того что у роботом может вполне быть больше.

lexa - 04/Сен/2017 15:51

Насколько я знаю, используемый фаззер дает лимит в 2Gb на фаззимую программу.
Исходя из 16-битных пикселей и двукратного запаса - конкретно для фаззера надо ставить мегапикселей 500, не больше.
Исходя из задачи "пофаззить побыстрее" (т.е. не раздувать перебор по переменным width/height) - ну вот хрен его знает.
Любое искусственное ограничение (которого в реальном коде не будет) - отрезает возможные проверки слишком рано и не факт что это хорошо.

Теперь почему "в реальном коде не будет". Ну потому, что (см. соседние комментарии) резать по одному фиксированному размеру для данного формата (ну там NEF - не крупнее чем D810) или по списку размеров - не очень хорошо, если мы осознанно даем юзеру возможность поправить у файла make/model и посмотреть что будет (впрочем, у софта, который пользуется hardcoded-метаданными от RawSpeed - будет не очень хорошо).

В реальном коде имеет смысл ставить (динамически) ограничение на максимальный размер (в мегапикселях), исходя ну вот из потенциально доступной памяти (и, скажем, в 32-битном FRV стоит стандартное ограничение в 40Mpix, правда юзер может его поменять)

mikeuz - 04/Сен/2017 17:39

ИМХО, это сделано для ускорения работы с каталогами, которые содержат снимки с нескольких камер.
Теоретически, не такая уж редкая ситуация.

lexa - 04/Сен/2017 17:54

Полет вашей мысли мне непонятен.