chromeos / cros-codecs

BSD 3-Clause "New" or "Revised" License
30 stars 11 forks source link

Some fixes for interlaced streams #32

Closed Gnurou closed 1 year ago

Gnurou commented 1 year ago

These are some very minor fixes and simplifications. The last commit is the one that I am the most worried about, the original code looked wrong but maybe I am missing its intent. Daniel, WDYT?

dwlsalmeida commented 1 year ago

@Gnurou LGTM

This is the exact same as this path:

        if pic_mut.is_second_field() {
            let first_field_rc = pic_mut.other_field_unchecked();
            drop(pic_mut);
            let mut first_field = first_field_rc.borrow_mut();
            first_field.set_second_field_to(&picture);
        } else {
            drop(pic_mut);
        }

For maintainability reasons, I think we should get rid of set_first_field_to and set_second_field_to. For consistency, if we are linking First -> Second, then it should automatically link Second->First.

Something like


fn set_other_field(&self, is_second_field: bool) {
   // Link A->B, B->A and then:
  self.is_second_field = is_second_field;
}
Gnurou commented 1 year ago

How about something like

fn link_fields(first: &Rc<RefCell<Self>>, second: &Rc<RefCell<Self>>) {
   // link first -> second and second -> first, then
   second.is_second_field = true;
}

?

dwlsalmeida commented 1 year ago

@Gnurou this does look much better, yes.

Gnurou commented 1 year ago
fn link_fields(first: &Rc<RefCell<Self>>, second: &Rc<RefCell<Self>>)

Unfortunately doing this would require to wrap the picture into a Rc<RefCell> earlier in the code, which is a larger change than I thought. It's probably sound to do so though, but let me check some for more alternatives.