[MPlayer-dev-eng] [PATCH 5/5] Various rectifications uncrustify wasn't able to do.
Dan Oscarsson
Dan.Oscarsson at tieto.com
Thu May 5 16:12:10 CEST 2011
tor 2011-05-05 klockan 15:20 +0200 skrev Clément Bœsch:
> On Thu, May 05, 2011 at 03:11:47PM +0200, Diego Biurrun wrote:
> > On Thu, May 05, 2011 at 03:00:17PM +0200, Clément Bœsch wrote:
> > > On Sat, Apr 16, 2011 at 12:02:53AM +0200, Clément Bœsch wrote:
> > > > Due to uncrustify and A.I. limitations, humans are still needed
> > > > sometimes.
> > >
> > > Patch updated. No more functionnal changes, and more cosmetics, vertical
> > > align & stuff. It follows the uncrustify patch.
> > >
> > > --- a/mplayer.c
> > > +++ b/mplayer.c
> > > @@ -449,16 +449,11 @@ char *get_metadata(metadata_t type)
> > >
> > > case META_VIDEO_CODEC:
> > > - if (sh_video->format == 0x10000001)
> > > - return strdup("mpeg1");
> > > - else if (sh_video->format == 0x10000002)
> > > - return strdup("mpeg2");
> > > - else if (sh_video->format == 0x10000004)
> > > - return strdup("mpeg4");
> > > - else if (sh_video->format == 0x10000005)
> > > - return strdup("h264");
> > > - else if (sh_video->format >= 0x20202020)
> > > - return mp_asprintf("%.4s", (char *)&sh_video->format);
> > > + if (sh_video->format == 0x10000001) return strdup("mpeg1");
> > > + else if (sh_video->format == 0x10000002) return strdup("mpeg2");
> > > + else if (sh_video->format == 0x10000004) return strdup("mpeg4");
> > > + else if (sh_video->format == 0x10000005) return strdup("h264");
> > > + else if (sh_video->format >= 0x20202020) return mp_asprintf("%.4s", (char *)&sh_video->format);
> > > return mp_asprintf("0x%08X", sh_video->format);
> >
> > I prefer the former. I don't consider it a good idea to stray from the
> > automatic indentation (too much).
> >
>
> OK, too bad. I just tought the association was more obvious that way.
I prefer the single line way. Also the way below. Clearer to the eye.
Often, an if statment with a single statement, is also easiest to se if
written on one line.
Indented code is easier to read if indented code has a terminating
symbol (like always use { }) and can easier get lines added/removed.
>
> > > @@ -479,26 +474,13 @@ char *get_metadata(metadata_t type)
> > >
> > > /* check for valid demuxer */
> > > - case META_INFO_TITLE:
> > > - return get_demuxer_info("Title");
> > > -
> > > - case META_INFO_ARTIST:
> > > - return get_demuxer_info("Artist");
> > > -
> > > - case META_INFO_ALBUM:
> > > - return get_demuxer_info("Album");
> > > -
> > > - case META_INFO_YEAR:
> > > - return get_demuxer_info("Year");
> > > -
> > > - case META_INFO_COMMENT:
> > > - return get_demuxer_info("Comment");
> > > -
> > > - case META_INFO_TRACK:
> > > - return get_demuxer_info("Track");
> > > -
> > > - case META_INFO_GENRE:
> > > - return get_demuxer_info("Genre");
> > > + case META_INFO_TITLE: return get_demuxer_info("Title");
> > > + case META_INFO_ARTIST: return get_demuxer_info("Artist");
> > > + case META_INFO_ALBUM: return get_demuxer_info("Album");
> > > + case META_INFO_YEAR: return get_demuxer_info("Year");
> > > + case META_INFO_COMMENT: return get_demuxer_info("Comment");
> > > + case META_INFO_TRACK: return get_demuxer_info("Track");
> > > + case META_INFO_GENRE: return get_demuxer_info("Genre");
> >
> > same
> >
> > > @@ -516,14 +498,14 @@ static void print_file_properties(const MPContext *mpctx, const char *filename)
> > > if (mpctx->sh_video) {
> > > /* Assume FOURCC if all bytes >= 0x20 (' ') */
> > > if (mpctx->sh_video->format >= 0x20202020)
> > > - mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_VIDEO_FORMAT=%.4s\n", (char *)&mpctx->sh_video->format);
> > > + mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_VIDEO_FORMAT=%.4s\n", (char *)&mpctx->sh_video->format);
> > > else
> > > mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_VIDEO_FORMAT=0x%08X\n", mpctx->sh_video->format);
> > > - mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_VIDEO_BITRATE=%d\n", mpctx->sh_video->i_bps * 8);
> > > - mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_VIDEO_WIDTH=%d\n", mpctx->sh_video->disp_w);
> > > - mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_VIDEO_HEIGHT=%d\n", mpctx->sh_video->disp_h);
> > > - mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_VIDEO_FPS=%5.3f\n", mpctx->sh_video->fps);
> > > - mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_VIDEO_ASPECT=%1.4f\n", mpctx->sh_video->aspect);
> > > + mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_VIDEO_BITRATE=%d\n", mpctx->sh_video->i_bps * 8);
> > > + mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_VIDEO_WIDTH=%d\n", mpctx->sh_video->disp_w);
> > > + mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_VIDEO_HEIGHT=%d\n", mpctx->sh_video->disp_h);
> > > + mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_VIDEO_FPS=%5.3f\n", mpctx->sh_video->fps);
> > > + mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_VIDEO_ASPECT=%1.4f\n", mpctx->sh_video->aspect);
> > > video_start_pts = ds_get_next_pts(mpctx->d_video);
> > > }
> > > if (mpctx->sh_audio) {
> > > @@ -531,10 +513,10 @@ static void print_file_properties(const MPContext *mpctx, const char *filename)
> > > if (mpctx->sh_audio->format >= 0x20202020)
> > > mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_AUDIO_FORMAT=%.4s\n", (char *)&mpctx->sh_audio->format);
> > > else
> > > - mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_AUDIO_FORMAT=%d\n", mpctx->sh_audio->format);
> > > - mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_AUDIO_BITRATE=%d\n", mpctx->sh_audio->i_bps * 8);
> > > - mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_AUDIO_RATE=%d\n", mpctx->sh_audio->samplerate);
> > > - mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_AUDIO_NCH=%d\n", mpctx->sh_audio->channels);
> > > + mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_AUDIO_FORMAT=%d\n", mpctx->sh_audio->format);
> > > + mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_AUDIO_BITRATE=%d\n", mpctx->sh_audio->i_bps * 8);
> > > + mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_AUDIO_RATE=%d\n", mpctx->sh_audio->samplerate);
> > > + mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_AUDIO_NCH=%d\n", mpctx->sh_audio->channels);
> >
> > I don't see the point.
> >
>
> It sticks more to the final output, and the values are aligned to we can
> dicern them more I guess.
>
> > > @@ -788,9 +769,8 @@ void exit_player(enum exit_reason how)
> > > static void child_sighandler(int x)
> > > {
> > > pid_t pid;
> > > - while ((pid = waitpid(-1, NULL, WNOHANG)) > 0) ;
> > > + while ((pid = waitpid(-1, NULL, WNOHANG)) > 0);
> > > }
> >
> > This looks like a bug in uncrustify or the uncrustify configuration.
> >
>
> Yep, but I wasn't able to find a correct workaround in the settings. Feel
> free to dig in it…
>
> > > @@ -1046,14 +1026,11 @@ static int playtree_add_playlist(play_tree_t *entry)
> > > {
> > > if (!entry) {
> > > entry = mpctx->playtree_iter->tree;
> > > - if (play_tree_iter_step(mpctx->playtree_iter, 1, 0) != PLAY_TREE_ITER_ENTRY) {
> > > + if (play_tree_iter_step(mpctx->playtree_iter, 1, 0) != PLAY_TREE_ITER_ENTRY)
> > > return PT_NEXT_ENTRY;
> > > - }
> > > - if (mpctx->playtree_iter->tree == entry) { // Loop with a single file
> > > - if (play_tree_iter_up_step(mpctx->playtree_iter, 1, 0) != PLAY_TREE_ITER_ENTRY) {
> > > + if (mpctx->playtree_iter->tree == entry) // Loop with a single file
> > > + if (play_tree_iter_up_step(mpctx->playtree_iter, 1, 0) != PLAY_TREE_ITER_ENTRY)
> > > return PT_NEXT_ENTRY;
> > > - }
> > > - }
> >
> > I think uncrustify has an option for this.
> >
>
> Yes under a warning section. We don't set it up because of some issue with
> defines.
>
> > > @@ -1927,9 +1903,9 @@ static mp_image_t *mp_dvdnav_copy_mpi(mp_image_t *to_mpi,
> > > if (to_mpi &&
> > > to_mpi->w == from_mpi->w &&
> > > to_mpi->h == from_mpi->h &&
> > > - to_mpi->imgfmt == from_mpi->imgfmt)
> > > + to_mpi->imgfmt == from_mpi->imgfmt) {
> > > mpi = to_mpi;
> > > - else {
> > > + } else {
> > > if (to_mpi)
> > > free_mp_image(to_mpi);
> > > if (from_mpi->w == 0 || from_mpi->h == 0)
> > > @@ -2531,8 +2507,9 @@ static void pause_loop(void)
> > > if (term_osd && !mpctx->sh_video) {
> > > set_osd_msg(OSD_MSG_PAUSE, 1, 0, MSGTR_Paused);
> > > update_osd_msg();
> > > - } else
> > > + } else {
> > > mp_msg(MSGT_CPLAYER, MSGL_STATUS, "\n"MSGTR_Paused "\r");
> > > + }
> > > mp_msg(MSGT_IDENTIFY, MSGL_INFO, "ID_PAUSED\n");
> > > }
> > > #ifdef CONFIG_GUI
> > > @@ -2574,9 +2551,10 @@ static void pause_loop(void)
> > > set_osd_msg(OSD_MSG_PAUSE, 1, 0, MSGTR_Paused " %d%%",
> > > new_cache_fill);
> > > update_osd_msg();
> > > - } else
> > > + } else {
> > > mp_msg(MSGT_CPLAYER, MSGL_STATUS, MSGTR_Paused " %d%%\r",
> > > new_cache_fill);
> > > + }
> >
> > I see little point in doing this manually if uncrustify can achieve it.
> >
>
> See above.
>
> > > @@ -3306,19 +3280,15 @@ play_next_file:
> > >
> > > if (mpctx->stream->type == STREAMTYPE_BD) {
> > > - if (audio_lang && audio_id == -1)
> > > - audio_id = bd_aid_from_lang(mpctx->stream, audio_lang);
> > > - if (dvdsub_lang && dvdsub_id == -1)
> > > - dvdsub_id = bd_sid_from_lang(mpctx->stream, dvdsub_lang);
> > > + if (audio_lang && audio_id == -1) audio_id = bd_aid_from_lang(mpctx->stream, audio_lang);
> > > + if (dvdsub_lang && dvdsub_id == -1) dvdsub_id = bd_sid_from_lang(mpctx->stream, dvdsub_lang);
> > > }
> > >
> > > #ifdef CONFIG_DVDREAD
> > > if (mpctx->stream->type == STREAMTYPE_DVD) {
> > > current_module = "dvd lang->id";
> > > - if (audio_lang && audio_id == -1)
> > > - audio_id = dvd_aid_from_lang(mpctx->stream, audio_lang);
> > > - if (dvdsub_lang && dvdsub_id == -1)
> > > - dvdsub_id = dvd_sid_from_lang(mpctx->stream, dvdsub_lang);
> > > + if (audio_lang && audio_id == -1) audio_id = dvd_aid_from_lang(mpctx->stream, audio_lang);
> > > + if (dvdsub_lang && dvdsub_id == -1) dvdsub_id = dvd_sid_from_lang(mpctx->stream, dvdsub_lang);
> >
> > I'd skip this as well.
> >
>
> OK :(
As I said above - for me it is much clearer when using a single line.
>
> > > @@ -3878,20 +3828,20 @@ goto_enable_cache:
> > >
> > > //============================ Auto QUALITY ============================
> > >
> > > -/*Output quality adjustments:*/
> > > + /* Output quality adjustments: */
> > > if (auto_quality > 0) {
> > > current_module = "autoq";
> > > -// float total=0.000001f * (GetTimer()-aq_total_time);
> > > -// if(output_quality<auto_quality && aq_sleep_time>0.05f*total)
> > > + //float total=0.000001f * (GetTimer()-aq_total_time);
> > > + //if(output_quality<auto_quality && aq_sleep_time>0.05f*total)
> > > if (output_quality < auto_quality && aq_sleep_time > 0)
> > > ++output_quality;
> > > else
> > > -// if(output_quality>0 && aq_sleep_time<-0.05f*total)
> > > + //if(output_quality>0 && aq_sleep_time<-0.05f*total)
> > > if (output_quality > 1 && aq_sleep_time < 0)
> > > --output_quality;
> > > else if (output_quality > 0 && aq_sleep_time < -0.050f) // 50ms
> > > output_quality = 0;
> > > -// printf("total: %8.6f sleep: %8.6f q: %d\n",(0.000001f*aq_total_time),aq_sleep_time,output_quality);
> > > + //printf("total: %8.6f sleep: %8.6f q: %d\n",(0.000001f*aq_total_time),aq_sleep_time,output_quality);
> > > set_video_quality(mpctx->sh_video, output_quality);
> >
> > The style in the commented-out code is messed up :-p
> >
>
> oh please :D
>
> > Most of the rest like the comment adjustments etc. looks OK.
> >
>
> OK
>
More information about the MPlayer-dev-eng
mailing list