Информация об изменениях

Сообщение Re[17]: Сишный каст - зло? от 31.07.2023 11:23

Изменено 31.07.2023 11:27 so5team

Re[17]: Сишный каст - зло?
Здравствуйте, B0FEE664, Вы писали:

BFE>>>Рад, что вы ждёте конструктивной критики.

S>>Для конструктивной нужно, чтобы критик понимал предмет разговора. Так что нет, не жду.
BFE>Т.е. вы думаете, что ваш код никто не поймёт?

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

BFE>>>Похоже, что image_bytes объявлен как указатель на const, если это так — то это зачем?

S>>Да, это const std::byte*, приходящий извне.
BFE>Но зачем нарушать контракт про "const"?

Потому, что в FFMPEG нужно иметь неконстантный указатель.

BFE>>>Почему обращение к fresh_frame->data по индексу, а не по итератору?

S>>Потому что это Си-ный массив. Имитировать итераторы через вызовы std::begin/end не вижу смысла.
BFE>Почему "эмитировать"? То, что в данном случае итератор — это указатель, ничего не меняет.

Потому, что мне здесь "итераторы" в каком-либо виде не нужны. Если вам кажется, что нужны, то, наверное, для этого есть какие-то доводы?

BFE>Это дело вкуса.


Скорее опыта.

BFE>То, что assert-ы не работают в release — опасное заблуждение.


Если assert-ы работают в релизе, то это уже не assert-ы.

S>>Во-вторых, это гарантируется в принципе Upd: гарантируется, что (std::size(fresh_frame->data) >= std::size(data_plane_sizes)). By design, так сказать.

BFE>Если By design, то где static_assert?

В Караганде, вероятно. Потому что by design. Т.к. если в FFMPEG внезапно сделают так, что в AVFrame::data будет меньше элементов, чем в аргументе linesizes для av_image_fill_plane_sizes, то нужно будет что-то менять в консерватории. Может быть законченный параноик будет напишет (не один) static_assert чтобы защититься от такой гипотетической ситуации, то я подобной паранойей еще не страдаю.

Во по поводу Си-шных кастов паранойя есть, т.к. там опастности не такие гипотетические.

BFE>>>Зачем столько вызовов ptr_cast? Не проще ли завести один указатель?

S>>Потому что исходный указатель, image_bytes, постоянно изменяется (это даже в приведенном кусочке видно). А по ходу дела его приходится приводить к std::uint8_t* (да, без const, такова специфика используемой внешней библиотеки). Можно было бы, конечно, вести сразу два указателя, image_bytes и второй, который имеет тип std::uint8_t. Но какой смысл дублировать одно и то же?
BFE>Чтобы не полагаться на оптимизатор.

Оптимизатор-то здесь причем? Или вы думаете, что на reinterpret_cast+const_cast есть какие-то накладные расходы в run-time?

BFE>>>Уверены, что []( void *, std::uint8_t * ) -> void {} не ведёт к утечке памяти? (>24 байта за вызов)

S>>Мне сложно представить откуда здесь может взяться утечка, т.к. этот фрагмент -- это получение указателя на функцию, которая ничего не делает. Данный указатель требуется для av_buffer_create в качестве "деаллокатора".
BFE>А то я не вижу!

Раз высказываете такие опасения, значит не видите. Или видите что-то свое.

S>>Но т.к. деаллокатор здесь не нужен, то и функция пуста.

BFE>Вот и хотелось бы понять почему деаллокатор не нужен.

Потому что это деаллокатор не для AVBuffer, а для AVBuffer::data.
Вызывается этот деаллокатор здесь (через здесь):
static void buffer_replace(AVBufferRef **dst, AVBufferRef **src)
 {
     AVBuffer *b;
  
     b = (*dst)->buffer;
  
     if (src) {
         **dst = **src;
         av_freep(src);
     } else
         av_freep(dst);
  
     if (atomic_fetch_sub_explicit(&b->refcount, 1, memory_order_acq_rel) == 1) {
         /* b->free below might already free the structure containing *b,
          * so we have to read the flag now to avoid use-after-free. */
         int free_avbuffer = !(b->flags_internal & BUFFER_FLAG_NO_FREE);
         b->free(b->opaque, b->data);
         if (free_avbuffer)
             av_free(b);
     }
 }


BFE>Таким образом: вы всё ещё уверены, что здесь нет утечки памяти?


Я ни в чем на 100% не уверен, но в данном случае очень надеюсь.
Re[17]: Сишный каст - зло?
Здравствуйте, B0FEE664, Вы писали:

BFE>>>Рад, что вы ждёте конструктивной критики.

S>>Для конструктивной нужно, чтобы критик понимал предмет разговора. Так что нет, не жду.
BFE>Т.е. вы думаете, что ваш код никто не поймёт?

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

BFE>>>Похоже, что image_bytes объявлен как указатель на const, если это так — то это зачем?

S>>Да, это const std::byte*, приходящий извне.
BFE>Но зачем нарушать контракт про "const"?

Потому, что в FFMPEG нужно иметь неконстантный указатель.

BFE>>>Почему обращение к fresh_frame->data по индексу, а не по итератору?

S>>Потому что это Си-ный массив. Имитировать итераторы через вызовы std::begin/end не вижу смысла.
BFE>Почему "эмитировать"? То, что в данном случае итератор — это указатель, ничего не меняет.

Потому, что мне здесь "итераторы" в каком-либо виде не нужны. Если вам кажется, что нужны, то, наверное, для этого есть какие-то доводы?

BFE>Это дело вкуса.


Скорее опыта.

BFE>То, что assert-ы не работают в release — опасное заблуждение.


Если assert-ы работают в релизе, то это уже не assert-ы.

S>>Во-вторых, это гарантируется в принципе Upd: гарантируется, что (std::size(fresh_frame->data) >= std::size(data_plane_sizes)). By design, так сказать.

BFE>Если By design, то где static_assert?

В Караганде, вероятно. Потому что by design. Т.к. если в FFMPEG внезапно сделают так, что в AVFrame::data будет меньше элементов, чем в аргументе linesizes для av_image_fill_plane_sizes, то нужно будет что-то менять в консерватории. Может быть законченный параноик будет напишет (не один) static_assert чтобы защититься от такой гипотетической ситуации, но я подобной паранойей еще не страдаю.

Вот по поводу Си-шных кастов паранойя есть, т.к. там опасности не такие гипотетические.

BFE>>>Зачем столько вызовов ptr_cast? Не проще ли завести один указатель?

S>>Потому что исходный указатель, image_bytes, постоянно изменяется (это даже в приведенном кусочке видно). А по ходу дела его приходится приводить к std::uint8_t* (да, без const, такова специфика используемой внешней библиотеки). Можно было бы, конечно, вести сразу два указателя, image_bytes и второй, который имеет тип std::uint8_t. Но какой смысл дублировать одно и то же?
BFE>Чтобы не полагаться на оптимизатор.

Оптимизатор-то здесь причем? Или вы думаете, что на reinterpret_cast+const_cast есть какие-то накладные расходы в run-time?

BFE>>>Уверены, что []( void *, std::uint8_t * ) -> void {} не ведёт к утечке памяти? (>24 байта за вызов)

S>>Мне сложно представить откуда здесь может взяться утечка, т.к. этот фрагмент -- это получение указателя на функцию, которая ничего не делает. Данный указатель требуется для av_buffer_create в качестве "деаллокатора".
BFE>А то я не вижу!

Раз высказываете такие опасения, значит не видите. Или видите что-то свое.

S>>Но т.к. деаллокатор здесь не нужен, то и функция пуста.

BFE>Вот и хотелось бы понять почему деаллокатор не нужен.

Потому что это деаллокатор не для AVBuffer, а для AVBuffer::data.
Вызывается этот деаллокатор здесь (через здесь):
static void buffer_replace(AVBufferRef **dst, AVBufferRef **src)
 {
     AVBuffer *b;
  
     b = (*dst)->buffer;
  
     if (src) {
         **dst = **src;
         av_freep(src);
     } else
         av_freep(dst);
  
     if (atomic_fetch_sub_explicit(&b->refcount, 1, memory_order_acq_rel) == 1) {
         /* b->free below might already free the structure containing *b,
          * so we have to read the flag now to avoid use-after-free. */
         int free_avbuffer = !(b->flags_internal & BUFFER_FLAG_NO_FREE);
         b->free(b->opaque, b->data);
         if (free_avbuffer)
             av_free(b);
     }
 }


BFE>Таким образом: вы всё ещё уверены, что здесь нет утечки памяти?


Я ни в чем на 100% не уверен, но в данном случае очень надеюсь.