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

[Stefan Schuermans]

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.

>> +/**
>> + * @brief close all files and free all memory of MNG vo module
>> + */
>> +static void vomng_prop_reset(void)
>> +{
>> [...]
>> +        /* reset state */
>> +        vomng->b_is_init = 0;
>> +        if (vomng->p_frame_first) {
>> +            p_frame = vomng->p_frame_first;
>> +            while (p_frame) {
>> +                p_next = p_frame->p_next;
>> +                if (p_frame->p_data)
>> +                    free(p_frame->p_data);
> 
> The if is pointless, or wrong.

Removed.

>> +/**
>> + * @brief close files, free memory and delete private data of MNG von module
>> + */
>> +static void vomng_prop_cleanup(void)
>> +{
>> +    if (vomng) {
>> +        vomng_prop_reset();
>> +        if (vomng->out_file_name)
>> +            free(vomng->out_file_name);
> 
> Ditto.

Also removed.

>> +/**
>> + * @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.

>> +    /* 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".

>> +/**
>> + * @brief pre-initialize MNG vo module
>> + * @param[in] *arg arguments passed to MNG vo module (output file name)
>> + * @return 0 on success, 1 on error
>> + */
>> +static int preinit(const char *arg)
>> +{
>> +    /* set up properties structure */
>> +    if (vomng_prop_setup())
>> +        return 1;
>> +
>> +    /* get name of output file */
>> +    if (arg == NULL || strlen(arg) == 0) {
> 
> if (!arg || !*arg)

Changed.

Thanks for reviewing.

Stefan


-------------- next part --------------
A non-text attachment was scrubbed...
Name: MPlayer-svn20110511-vomng.diff
Type: text/x-patch
Size: 23348 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20110512/c881a657/attachment-0001.bin>