[FFmpeg-cvslog] matroska: cleanup handling of video stereo mode

[Aurelien Jacobs]

On Tue, May 24, 2011 at 08:15:32PM +0200, Reimar Döffinger wrote:
> On Tue, May 24, 2011 at 01:13:35AM +0200, Aurelien Jacobs wrote:
> > +const char const *matroska_video_stereo_mode[] = {
> > +const char const *matroska_video_stereo_plane[] = {
> 
> That's wrong.
> "const char *", "char const *" and "const char const *"
> are all exactly the same thing.
> You meant "const char * const".

Humm, indeed.

> > +#define MATROSKA_VIDEO_STEREO_MODE_COUNT  15
> > +#define MATROSKA_VIDEO_STEREO_PLANE_COUNT  3
> 
> If you use defines, you should also use those defines
> in the array declarations, that avoids overreads and will
> also remind people that they have to change them when changing
> the array.

Good idea.
I usually try to avoid such kind of define and just use sizeof() instead
but here we need the size in a different compile unit than the one where
the table is defined.

> > +            /* export stereo mode flag as metadata tag */
> > +            if (track->video.stereo_mode && track->video.stereo_mode < MATROSKA_VIDEO_STEREO_MODE_COUNT)
> > +                av_metadata_set2(&st->metadata, "stereo_mode", matroska_video_stereo_mode[track->video.stereo_mode], 0);
> 
> It might be a good idea to warn in case of an unknown value.

Maybe, but I'm not sure it would be that useful...

> > +            /* if we have virtual track, mark the real tracks */
> > +            for (j=0; j < track->operation.combine_planes.nb_elem; j++) {
> > +                char buf[32];
> > +                if (planes[j].type < MATROSKA_VIDEO_STEREO_PLANE_COUNT)
> > +                    continue;
> 
> Huh? Did you flip the condition?!

Ouch ! My bad !
I should touch the code when I'm in a hurry... (but then I would never
touch then code :-( )

Thanks a lot for the review !!
Fixes applied.

Aurel