[MPlayer-dev-eng] [PATCH] MNG output support for MPlayer

[Clément Bœsch]

On Thu, May 12, 2011 at 12:02:38AM +0200, Stefan Schuermans wrote:
> Clément Bœsch schrieb:
> >On Tue, May 10, 2011 at 09:36:15PM +0200, Stefan Schuermans wrote:
> >>[...]
> >>+static vo_info_t info = {
> >>+    "MNG driver",
> >>+    "mng",
> >>+    "Stefan Schuermans <stefan blinkenarea org>"
> >>+};
> >>+
> >
> >Do you mind adding attribut specifiers?
> 
> I'm not 100% sure that I understood that correctly. I think you were
> talking about including the struct member identifiers in the
> initializer (e.g. ".name ="). That's what I included now.
> 

Yes, thanks.

> >>+ * @brief configure MNG vo module
> >>+ * @param[in] width video width
> >>+ * @param[in] height video height
> >>+ * @param[in] d_width (unused)
> >>+ * @param[in] d_height (unused)
> >>+ * @param[in] flags (unused)
> >>+ * @param[in] title (unused)
> >>+ * @param[in] format video frame format
> >>+ * @return 0 on success, 1 on error
> >>+ */
> >>+static int config(uint32_t width, uint32_t height,
> >>+                  uint32_t d_width, uint32_t d_height,
> >>+                  uint32_t flags, char *title, uint32_t format)
> >>+{
> >>+    uint32_t row_stride, y;
> >>+
> >
> >Are the uint32_t really mandatory for the width/height or unsigned would
> >be fine? Specifying specific may supposed you're going to use the that
> >exact size.
> 
> All of the VO modules and the prototype of this function in
> video_out.h use uint32_t like that. So I think it is more reasonable
> to use it, too. I picked this type for row_stride and y, because
> they are directly related to the parameters width/height.
> 

OK my bad, I should have looked at the default prototype…

> >>+    /* we are initialized */
> >>+    vomng->b_is_init = 1;
> >>+
> >
> >It took me quiet a few time to realize what the b meant… please avoid that
> >notation.
> 
> Renamed to "is_init".
> 

Thank you. Of course, if you had done it for all the other i_* and p_*
that would have been awesome, but I don't really care. Your patch looks
fine.

> +    for (y = 0; y < hf; ++y)

Just a nitpick while I'm at it: y++ is preferred (don't waste your time
re-sending a patch for this). Since this is not ++C code it doesn't
affect at all the generated code.

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 490 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20110512/ab6cb0fa/attachment.asc>