HW #1

[Surkov Petr]

Пока что без доп. задания, потом, может, выполню его.

[Sokolov Viacheslav]

Кажется, пока что не работают прямоугольные изображения. Проще всего проверять так: делаете не 1 поворот, а 4, изображение должно остаться неизменным.

[Sokolov Viacheslav]

Для промежуточной попытки exit - ОК, но лучше его не использовать. Мотивацию позже допишу на Вики.

Желательно также вывести пользователю источник ошибки, а не только факт наличия проблемы.

fwrite может не состояться.

В bmp.h не хватает включения заголовочных файлов (например, size_t определен в stddef.h)

Непонятно, почему в функции save_bmp есть такие строчки
207 free(bmp->header);
208 free(bmp->info);
209 free(bmp->data);

На мой взгляд, логичнее было бы из функций crop, rotate возвращать Bmp*, а принимать const Bmp* const

Антоним к слову add - remove

65 for (size_t j = 0; j < alignment; j++) {
66 new_data[real_width * i + 3 * bmp->width + j] = 0;
67 }

здесь проще memset использовать

производительнее было бы не переходить от одной модели к другой, но в задании этого не требуется. Но можно хотя бы избежать лишних операций, если alignment == 0.
Кроме того, стоит поддерживать в самой структуре Bmp ее состояние - выравнена или нет. Это позволит контролировать, что в очередной функции выполняется предположение о выравнивании, необходимое для работы.

158 bmp->data_size = (3 * w + get_alignment(bmp)) * h;
Сейчас не очень прозрачная логика data_size. Где-то он совпадает с размером реально аллоцированных данных, где-то - нет.

Обращаю внимание, что Height бывает отрицательный.

Я все же вижу, что на моем тесте несколько байт отличаются от того, как должно быть.

[Sokolov Viacheslav]

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

[Sokolov Viacheslav]

Я все же вижу, что на моем тесте несколько байт отличаются от того, как должно быть.

Причину понял: biXPelsPerMeter и biYPelsPerMeter нужно пересчитывать при повороте.

[Sokolov Viacheslav]

Да и biSizeImage не обновляется в заголовке

[Sokolov Viacheslav]

Насчет biXPelsPerMeter и biYPelsPerMeter я кажется ошибся, их модифицировать, видимо, все же не нужно. А вот biSizeImage важен.

[Surkov Petr]

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

[Sokolov Viacheslav]

biSizeImage 0 - ок, но лучше все же выставить его в get_img_size (исключительно ради моего удобства; в задании это не прописано, но при проверке используется)

про exit: на wiki все не успеваю написать, вкратце - ОК, если делается из main.c; из bmp - плохо. Если делать, то с освобождением всех ресурсов.

close_bmp - не очень удачное название (непонятно, что именно делает). Можно, например, free_bmp_internals.

Сейчас можно легко сократить пиковое потребление памяти.

get_size_t проще реализовать вот так

return *(size_t*)(bytes);

(set_size_t аналогично)

возможно, лучше именовать не set_width_and_height, а, скажем, set_sizes (потому что не только линейные размеры меняются)

Возможно, в add_alignment лучше поставить assert(!bmp->has_alignment);
(в remove_alignment аналогично)

79 assert(read_bytes == HEADER_SIZE);
этот и аналогичные ассерты ставить некорректно: мало ли что может произойти во время чтения с диска. Такие ситуации нужно обрабатывать.

106 if (bmp->has_alignment) {
107 printf("Can't crop because file was loaded incorrectly");
108 exit(-1);
109 }
здесь и в аналогичных местах лучше поставить assert - это ошибка программиста, соответственно ее не надо пытаться обрабатывать.

crop-rotate сейчас делает не то, что нужно (попробуйте вырезать небольшое изображение)

[Sokolov Viacheslav]

203 if (bmp->header) {
204 free(bmp->header);
205 }

free корректно работает с NULL

не хватает проверок на not null int* error

atoi возвращает int, а в функции crop принимается uint32_t