[PATCH] Get rid of the "main" label
Hi, I'd like a feedback on this patch: there is still a "main" label in mplayer.c. This is patch to get rid of it. The code first check if there is a video context (is it the correct name for sh_video?). If there is, the video chain is reset. After that, if neither sh_video nor sh_audio are set, it goes to the next file. Then, two things are done, but only if there was a video context at first, so I just moved this in the "main" code where sh_video is checked. It seems no previous code depends on the spu state and the font load flag. Btw, mplayer.c really needs at least a reindent; the current state is quiet a mess… -- Clément B. Not sent from a jesusPhone.
On Sat, Dec 25, 2010 at 05:29:32PM +0100, Clément Bœsch wrote:
Hi,
I'd like a feedback on this patch: there is still a "main" label in mplayer.c. This is patch to get rid of it.
The code first check if there is a video context (is it the correct name for sh_video?). If there is, the video chain is reset. After that, if neither sh_video nor sh_audio are set, it goes to the next file.
Then, two things are done, but only if there was a video context at first, so I just moved this in the "main" code where sh_video is checked. It seems no previous code depends on the spu state and the font load flag.
Btw, mplayer.c really needs at least a reindent; the current state is quiet a mess…
Ping. -- Clément B.
On 25 Dec 2010, at 17:29, Clément Bœsch <ubitux@gmail.com> wrote:
I'd like a feedback on this patch: there is still a "main" label in mplayer.c. This is patch to get rid of it.
If you only want to get rid of it, just use a if(sh_video) block there and leave everything else unchanged. I don't like changing more code than necessary at least not if there seems to be no good reason to do it...
On Sat, Jan 08, 2011 at 05:41:38PM +0100, Reimar Döffinger wrote:
On 25 Dec 2010, at 17:29, Clément Bœsch <ubitux@gmail.com> wrote:
I'd like a feedback on this patch: there is still a "main" label in mplayer.c. This is patch to get rid of it.
If you only want to get rid of it, just use a if(sh_video) block there and leave everything else unchanged. I don't like changing more code than necessary at least not if there seems to be no good reason to do it...
It's not enough since there is two goto main. I would have done it like you said in case of just one. -- Clément B.
On 8 Jan 2011, at 18:50, Clément Bœsch <ubitux@gmail.com> wrote:
On Sat, Jan 08, 2011 at 05:41:38PM +0100, Reimar Döffinger wrote:
On 25 Dec 2010, at 17:29, Clément Bœsch <ubitux@gmail.com> wrote:
I'd like a feedback on this patch: there is still a "main" label in mplayer.c. This is patch to get rid of it.
If you only want to get rid of it, just use a if(sh_video) block there and leave everything else unchanged. I don't like changing more code than necessary at least not if there seems to be no good reason to do it...
It's not enough since there is two goto main. I would have done it like you said in case of just one.
Ok, but that's still no reason to move the code so far around. Also the code is confusing, part of the confusion is due to a completely pointless check ofmthe reinit result, the only thing that does is making a crash possible. I think the code should be If (video) reinit... If (video) previous code Else if (!audio) goto next
On Sat, Jan 08, 2011 at 10:40:30PM +0100, Reimar Döffinger wrote:
On 8 Jan 2011, at 18:50, Clément Bœsch <ubitux@gmail.com> wrote:
On Sat, Jan 08, 2011 at 05:41:38PM +0100, Reimar Döffinger wrote:
On 25 Dec 2010, at 17:29, Clément Bœsch <ubitux@gmail.com> wrote:
I'd like a feedback on this patch: there is still a "main" label in mplayer.c. This is patch to get rid of it.
If you only want to get rid of it, just use a if(sh_video) block there and leave everything else unchanged. I don't like changing more code than necessary at least not if there seems to be no good reason to do it...
It's not enough since there is two goto main. I would have done it like you said in case of just one.
Ok, but that's still no reason to move the code so far around. Also the code is confusing, part of the confusion is due to a completely pointless check ofmthe reinit result, the only thing that does is making a crash possible. I think the code should be If (video) reinit... If (video) previous code Else if (!audio) goto next
Mmh ok I see :) Well I'll commit this version in a few days then. Thanks. Just a question though: is the crash you were refering to would be a case where reinit returns false but video context is still set? -- Clément B.
On Wed, Jan 12, 2011 at 10:39:29PM +0100, Clément Bœsch wrote:
On Sat, Jan 08, 2011 at 10:40:30PM +0100, Reimar Döffinger wrote:
On 8 Jan 2011, at 18:50, Clément Bœsch <ubitux@gmail.com> wrote:
On Sat, Jan 08, 2011 at 05:41:38PM +0100, Reimar Döffinger wrote:
On 25 Dec 2010, at 17:29, Clément Bœsch <ubitux@gmail.com> wrote:
I'd like a feedback on this patch: there is still a "main" label in mplayer.c. This is patch to get rid of it.
If you only want to get rid of it, just use a if(sh_video) block there and leave everything else unchanged. I don't like changing more code than necessary at least not if there seems to be no good reason to do it...
It's not enough since there is two goto main. I would have done it like you said in case of just one.
Ok, but that's still no reason to move the code so far around. Also the code is confusing, part of the confusion is due to a completely pointless check ofmthe reinit result, the only thing that does is making a crash possible. I think the code should be If (video) reinit... If (video) previous code Else if (!audio) goto next
Mmh ok I see :)
Well I'll commit this version in a few days then. Thanks. Just a question though: is the crash you were refering to would be a case where reinit returns false but video context is still set?
No, I mean the only case where not checking reinit result makes any difference in behaviour is if it would return true but still set sh_video to NULL, which would crash with the current code. So that means that we definitely will not lose anything by changing it.
-if(!mpctx->sh_video) goto main; // audio-only - -if(!reinit_video_chain()) { - if(!mpctx->sh_video){ - if(!mpctx->sh_audio) goto goto_next_file; - goto main; // exit_player(MSGTR_Exit_error); - } -} - - if(vo_flags & 0x08 && vo_spudec) - spudec_set_hw_spu(vo_spudec,mpctx->video_out); + if (mpctx->sh_video) + reinit_video_chain();
+ if (mpctx->sh_video) { + if (vo_flags & 0x08 && vo_spudec) + spudec_set_hw_spu(vo_spudec, mpctx->video_out); #ifdef CONFIG_FREETYPE - force_load_font = 1; + force_load_font = 1; #endif + } else if (!mpctx->sh_audio) + goto goto_next_file;
While it's small enough to be readable/reviewable okish anyway, I'd prefer it if you applied reindenting the unchanged parts separately. Otherwise ok.
On Thu, Jan 13, 2011 at 07:55:21AM +0100, Reimar Döffinger wrote:
[...] -if(!mpctx->sh_video) goto main; // audio-only - -if(!reinit_video_chain()) { - if(!mpctx->sh_video){ - if(!mpctx->sh_audio) goto goto_next_file; - goto main; // exit_player(MSGTR_Exit_error); - } -} - - if(vo_flags & 0x08 && vo_spudec) - spudec_set_hw_spu(vo_spudec,mpctx->video_out); + if (mpctx->sh_video) + reinit_video_chain();
+ if (mpctx->sh_video) { + if (vo_flags & 0x08 && vo_spudec) + spudec_set_hw_spu(vo_spudec, mpctx->video_out); #ifdef CONFIG_FREETYPE - force_load_font = 1; + force_load_font = 1; #endif + } else if (!mpctx->sh_audio) + goto goto_next_file;
While it's small enough to be readable/reviewable okish anyway, I'd prefer it if you applied reindenting the unchanged parts separately. Otherwise ok.
Applied without reindent, thank you. -- Clément B.
participants (2)
-
Clément Bœsch -
Reimar Döffinger