[Bug 739281] video-blend: correct improper logic calculations

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=739281
  GStreamer | gst-plugins-base | unspecified

Tim-Philipp Müller <t.i.m> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #289971|none                        |reviewed
             status|                            |

--- Comment #36 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2014-11-05 13:12:31 UTC ---
(From update of attachment 289971)
Thanks, that looks very good, almost ready to commit. Just a few more minor
niggles, thanks for your patience :)

- in test_overlay_blend_rect() does the video frame need to be unmapped at the
very end?

 - typo in commit message: hegiht -> height

 - the commit message describes what the patch does (tests different x/y
overlay positions etc.), but does not really make the *intention* of this all
explicit, which is that we want to test rendering of overlays that are outside
or partially outside of the video boundaries. People can look at the patch and
see *what* it does, what's more difficult to figure out is *why* something is
done. This applies to commit messages in general btw, "fix logic error" conveys
very little information, the reader knows that this commit fixed something, but
what exactly it fixes the reader will have to 'reverse-engineer' from the patch
(as another example).

- no need to use glong here, just use normal gints (glong is more awkward to
printf in debug logs and doesn't really buy you anything since it will likely
be same size as int on 32-bit systems anyway)

- capitalise the #define, i.e. VIDEO_WIDTH and VIDEO_HEIGHT (this is just
convention, makes it easier to see in code that this is a #define and not a
variable name)

-  gst_video_frame_map() needs to be READWRITE, doesn't it?

- in test_overlay_blend() perhaps describe the test case in natural language
rather than in equations (I know I did like that too, sorry). That makes it
easier for anyone reading the code to figure out if the code does what you want
it to do.

I'll look at the actual patch/fix later.

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.