[MPlayer-dev-eng] [PATCH] mencoder vobsub out patch

[Michael Krasnicki]

Guillaume,

> +static vobsub_out_t *
> +vobsub_out_create_struct()
> +{
> +   vobsub_out_t *vob = malloc(sizeof(vobsub_out_t));

The vobsub_out_t gets freed when function vobsub_out_close() is 
called in mencoder.c. I think that is fine but I will put some 
temporary debug in the code and check. BTW, I did not create
vobsub_out_t and vobsub_out_close(). That already exists in the
CVS version. I just added more data members to this struct.

But, as I was reviewing my code, I did find a different spot
were the vobsub_out_t.overflow member can freed more than once
because I do not reset the pointer to NULL. Ooops. I will fix
that and post another patch over the weekend.

Also, I can replace each set of 8 spaces at the beginning of each
line with a tab if you want. This should make it consistent with
the other code in that file.

> Maybe you could add doxygen comments about this function, and the
> others that you add.

Where can I find an example file in the mplayer source with the 
type of documentation that you are looking for?

Regards,
Michael

--- Guillaume POIRIER <poirierg at gmail.com> wrote:

> Hi,
> 
> On 9/7/05, Michael Krasnicki <kraz_subscribe at yahoo.com> wrote:
> > Hi,
> > 
> > As request, attached please find my patch in unified diff
> > format. Please let me know if the patch requires any
> > additional changes.
> 
> +static vobsub_out_t *
> +vobsub_out_create_struct()
> +{
> +   vobsub_out_t *vob = malloc(sizeof(vobsub_out_t));
> 
> It does seem like you do free this pointer when need, but I'm not
> quite sure. Could you double check that that code won't leak?
> 
> Maybe you could add doxygen comments about this function, and the
> others that you add.
> 
> 
>      filename = malloc(strlen(basename) + 5);
>      if (filename) {
> -	result = malloc(sizeof(vobsub_out_t));
> -	result->fsub = NULL;
> -	result->fidx = NULL;
> -	result->aid = 0;
> +        result = vobsub_out_create_struct();
> +        result->forced_subs_only = forced_subs_only;
> 
> 
> The indentation style is not consistent. The removed lines used tabs,
> when those 2 added line use spaces. I don't think that's all that
> important as the rest of the file is not consistent either.
> 
> As you can see, all those are very minor nits. That code does seem to
> do what it's supposed to, though I have not tested it much.
> 
> Guillaume
> -- 
> Reading doesn't hurt, really!
>   -- Dominik 'Rathann' Mierzejewski
> 
> _______________________________________________
> MPlayer-dev-eng mailing list
> MPlayer-dev-eng at mplayerhq.hu
> http://mplayerhq.hu/mailman/listinfo/mplayer-dev-eng
> 

__________________________________________________
Do You Yahoo!?
Tired of spam?  Yahoo! Mail has the best spam protection around 
http://mail.yahoo.com