mozilla / aom

Alliance for Open Media Video Codec reference implementation (Mozilla branch)
http://aomedia.org/
BSD 2-Clause "Simplified" License
144 stars 28 forks source link

break twice #1

Closed maddin200 closed 6 years ago

maddin200 commented 6 years ago

\av1\encoder\mcomp.c line 2608 duplicate break

dragetd commented 6 years ago

Hey, thanks for reporting things. In this case, you might have even tried to edit it yourself and create a PR! This workflow allows easy contributions by external people. https://help.github.com/articles/about-pull-requests/

You might want to give it a try whenever you find something in a project! :-)

That being said, in the case of https://github.com/smarter/aom/blob/5bd2d2f9dfcbe435412ff9a01d3554cac7a38f25/av1/encoder/mcomp.c#L2607 , I believe this line is intentional. The following if-block has some not-so-intuitive subpixel address calculation and thus is a bit separated.

You could try to import the project into an IDE of your choice and go through all the small things you notice and create one PR with all the small changes, calling it 'Minor code cleanups'. But it might be a case of personal preference at many places and you should not be discouraged if the maintainer sees things differently. The most logical (and thus easy to argue) things would be inconsistencies in style… every programmer has a bit of OCD and with not say no to someone cleaning those up. :P

maddin200 commented 6 years ago

Of course I used the wrong verison of file with incorrect line, here is the correct position: line 2322 procedure: int av1_full_pixel_search(const AV1_COMP cpi, MACROBLOCK x, BLOCK_SIZE bsize, MV mvp_full, int step_param, int error_per_bit, int cost_list, const MV *ref_mv, int var_max, int rd) ..... break;

break;

dragetd commented 6 years ago

Looks like someone had a 'case' and removed it, without removing the break. Indeed, something that can be cleaned up. How about creating a PR? :)

Is it only me… but when looking further down, I see some somewhat scary macros: https://github.com/smarter/aom/blob/master/av1/encoder/mcomp.c#L2342 xD

luctrudeau commented 6 years ago

I don't think that's a good idea. Might be better to update this repo to a more recent version of libaom. If I recall full_pixel_search as been removed from libaom.

maddin200 commented 6 years ago

Where is the most recent version ? Belongs it to Google ? Thanks for help Martin

luctrudeau commented 6 years ago

Current latest of libaom is on google source: https://aomedia.googlesource.com/aom/

From my understanding, this project is the version of libaom that will be integrated into Firefox. I think this project will be updated when the libaom source code changes less

maddin200 commented 6 years ago

Issues should be raised on the original repository, so I will close this one.