[MPlayer-dev-eng] [PATCH 5/5] Various rectifications uncrustify wasn't able to do.
Clément Bœsch
ubitux at gmail.com
Thu May 5 15:20:58 CEST 2011
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.
> > @@ -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 :(
> > @@ -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
--
Clément B.
More information about the MPlayer-dev-eng
mailing list