[MPlayer-dev-eng] [PATCH] XviD profile support

[Robert Swain]

Hello,
On 9/10/05, Guillaume POIRIER <poirierg at gmail.com> wrote:
> On 9/10/05, Robert Swain <robert.swain at gmail.com> wrote:
> > Usage is:
> > profile=<option>
> > Restricts options and VBV according to the Simple, Advanced Simple and
> > DivX profiles.
> 
> What does VBV mean? Maybe I know, but Joe-six-pack doesn't :-)

Done, as per your recommendation.

> I think you should add smth like: "the resulting videos will have a
> greater chance to be decoded correctly by standalone and third party
> players."
> 
> Maybe a note about using that option along with "-fourcc" may be a good idea...

Done. Recommending that an appropriate fourcc should be used and
recommending dx50 as, according to someone I know who has reasonable
experience with standalones, the xvid fourcc is not always recognised
by standalones, but dx50 is mostly. If anyone else has any suggestions
for this they will be more than welcome.

> Other than that, the code seems quite right to me.
> I wonder if the big structure: const profile_t profiles[] should go to
> the .h file...

See comment to Reimar's suggestion further down...

> +       if ((profiles[proftonum(xvidenc_profile)].flags & PROFILE_MPEGQUANT) &&
> +               (xvidenc_quant_method != NULL) && !strcasecmp(xvidenc_quant_method, "mpeg"))
> +       {
> +               frame->vol_flags |= XVID_VOL_MPEGQUANT;
> 
> if MPEG quant are not supported (on H.263) by DivX, maybe you should
> add a comment about this here.

Done.

> The comments you added should, I think, use doxygen-style I think
> there is a doc about this on DOCS/tech.

After a short discussion, my feelings were - "I haven't seen anyone
else using doxygen comments, why should I?" If anyone has a good
explanation then I'm perfectly willing, I'm just unsure what should be
a doxygen comment and what should not.

> Last but not least, add a patch to the AUTHORS file! ;-P

Done. :) And thanks for your help with the documentation and such Guillaume.

On 9/10/05, Reimar Döffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de> wrote:
> On Sat, Sep 10, 2005 at 07:45:41PM +0100, Robert Swain wrote:
> > This patch adds Simple, Advanced Simple and DivX profile support.
> 
> The .h file contains a few DOS-linebreaks. Can't comment on the rest
> (except that the .h file contains rather little, maybe it can be
> avoided?).

After a little discussion about the reasons for .h files it was
decided it would be better to move the necessary definitions to the
main .c file as the definitions were not going to be used by any other
files.

Now for the fun stuff. I didn't like the proftonum() function as I
mentioned and this has been changed to a neater function,
profileFromName(). Thanks to Reimar, uau and Ivan for their input on
this.

Also in revision 0 I missed a 0 check in the fps code for the
unrestricted case. This has been rectified.

The behaviour has been altered slightly - if a profile name is
mistyped an error message will be displayed which includes the profile
name as typed.

Finally, regarding the profiles[] array and the profile_t type - a lot
of the information in this array, which I originally lifted directly
from the XviD VfW code, appears to be redundant for our use. Is it
desirable that the type and array be pruned down to only what is
necessary or should it be left as is?

Regards,
Robert Swain (superdump)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mplayer.xvid.profiles.1.diff
Type: application/octet-stream
Size: 15898 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20050912/5cf849fb/attachment.obj>