gui: subtitle menu is not populated when playing from mpg file
please consider the following fix for mplayer -gui... mpctx_get_global_sub_size(guiInfo.mpcontext); is not returning the number of subtitles, because void update_global_sub_size(MPContext *mpctx) was not called to update it. ps: the indentation is not optimal to keep the patch size minimal Index: gui/dialog/menu.c =================================================================== --- gui/dialog/menu.c (revision 37244) +++ gui/dialog/menu.c (working copy) @@ -733,9 +733,17 @@ } /* cheap subtitle switching for non-DVD streams */ - if ( global_sub_size && guiInfo.StreamType != STREAMTYPE_DVD ) + if ( guiInfo.StreamType != STREAMTYPE_DVD ) { - int pos, i, j, subs0 = guiInfo.mpcontext->sub_counts[SUB_SOURCE_SUBS], subs1 = guiInfo.mpcontext->sub_counts[SUB_SOURCE_VOBSUB]; + int pos, i, j, subs0, subs1, subs2; + subs0 = guiInfo.mpcontext->sub_counts[SUB_SOURCE_SUBS]; + subs1 = guiInfo.mpcontext->sub_counts[SUB_SOURCE_VOBSUB]; + subs2 = 0; //guiInfo.mpcontext->sub_counts[SUB_SOURCE_DEMUX]; // may be out-of-date! + for (i = 0; i < MAX_S_STREAMS; i++) + if (guiInfo.mpcontext->demuxer->s_streams[i]) + subs2++; + global_sub_size = subs0 + subs1 + subs2; + if ( global_sub_size) { pos = mpctx_get_global_sub_pos(guiInfo.mpcontext); SubMenu=AddSubMenu( window1, (const char*)subtitle_xpm, Menu, MSGTR_GUI_Subtitles ); AddMenuCheckItem( window1, (const char*)empty1px_xpm, SubMenu, MSGTR_GUI__none_, pos == -1, (-1 << 16) + ivSetSubtitle ); @@ -784,6 +792,7 @@ else snprintf( tmp, sizeof(tmp), MSGTR_GUI_TrackN, i ); AddMenuCheckItem( window1,(const char*)empty1px_xpm,SubMenu,tmp,pos == i,( i << 16 ) + ivSetSubtitle ); } + } } AddSeparator( Menu );
Lode Leroy wrote on Fri, 1 Aug 2014 16:30:25 +0200:
please consider the following fix for mplayer -gui...
mpctx_get_global_sub_size(guiInfo.mpcontext); is not returning the number of subtitles, because void update_global_sub_size(MPContext *mpctx) was not called to update it.
Could you please point to a (small) sample? Ingo
On Thu, 07 Aug 2014 18:11:08 +0200 Ingo Brückl <ib@wupperonline.de> wrote:
Could you please point to a (small) sample?
i approved his message from the queue. it was moderated because he wasnt subscribed, you might want to cc him if you havent already. -compn
Lode Leroy wrote on Fri, 1 Aug 2014 16:30:25 +0200:
please consider the following fix for mplayer -gui...
mpctx_get_global_sub_size(guiInfo.mpcontext); is not returning the number of subtitles, because void update_global_sub_size(MPContext *mpctx) was not called to update it.
Well, this is because of the mpg demuxer (lavf works fine by the way). Apparently the mpg demuxer can detect subtitles only while playing, but MPlayer has already finished updating subtitle information then. In order to really fix the cause, update_global_sub_size() should be called prior to returning MPContext global_sub_size information. Unfortunately, update_global_sub_size() cannot be static any longer then. I consider this a better solution that adding some update_global_sub_size() code to the GUI. Opinions? Ingo
On Tue, Aug 12, 2014 at 04:03:58PM +0200, Ingo Brückl wrote:
Lode Leroy wrote on Fri, 1 Aug 2014 16:30:25 +0200:
please consider the following fix for mplayer -gui...
mpctx_get_global_sub_size(guiInfo.mpcontext); is not returning the number of subtitles, because void update_global_sub_size(MPContext *mpctx) was not called to update it.
Well, this is because of the mpg demuxer (lavf works fine by the way).
Apparently the mpg demuxer can detect subtitles only while playing, but MPlayer has already finished updating subtitle information then.
In order to really fix the cause, update_global_sub_size() should be called prior to returning MPContext global_sub_size information. Unfortunately, update_global_sub_size() cannot be static any longer then.
I consider this a better solution that adding some update_global_sub_size() code to the GUI.
Opinions?
I would think that the global sub size variable ideally should be kept up-to-date automatically. I guess that is one way to make that happen, I was just more wondering why it doesn't happen automatically once we got a new subtitle stream, instead of on every read of that variable.
Reimar Döffinger wrote Tue, 12 Aug 2014 20:15:05 +0200:
I would think that the global sub size variable ideally should be kept up-to-date automatically. I guess that is one way to make that happen, I was just more wondering why it doesn't happen automatically once we got a new subtitle stream, instead of on every read of that variable.
On getting a new subtitle stream we only have access to the demuxer (variable), the global sub stuff is stored whithin the mpctx. I don't know whether this is necessary or a design flaw or just something that had been added later. I could try moving the global sub information to the demuxer, but I will only learn during implementation whether this is possible at all. I cannot overview at the moment. Ingo
I wrote on Tue, 12 Aug 2014 21:04:21 +0200:
Reimar Döffinger wrote Tue, 12 Aug 2014 20:15:05 +0200:
I was just more wondering why it doesn't happen automatically once we got a new subtitle stream, instead of on every read of that variable.
On getting a new subtitle stream we only have access to the demuxer (variable), the global sub stuff is stored whithin the mpctx. I could try moving the global sub information to the demuxer, [...]
Although mpctx's global_sub_size is globally accessible, it's basically local to command.c, i.e. it's being updated only there (in update_global_sub_size()). Moreover, since it counts all subtitles we won't achieve much (but new problems) on relocating it to the demuxer just in order to be able to count the demuxer subtitles. There are file and vob subtitles as well. The following patch for the global_sub_size issue is bigger and seems more complicated than the last one, but it is better. The two mpctx_get_global_sub_*() functions (currently only used by the GUI) merge into one. The pos is retrieved by a mp_property_do("sub") command which performs the necessary update of the global_sub-* variables. As a result, update_global_sub_size() can remain static. Ingo
On Wed, Aug 13, 2014 at 07:00:09PM +0200, Ingo Brückl wrote:
Index: mplayer.c =================================================================== --- mplayer.c (revision 37251) +++ mplayer.c (working copy) @@ -360,14 +360,13 @@ return &mpctx->mixer; }
-int mpctx_get_global_sub_size(MPContext *mpctx) +void mpctx_get_global_sub_info(MPContext *mpctx, int *size, int *pos) { - return mpctx->global_sub_size; -} + mp_property_do("sub", M_PROPERTY_GET, pos, mpctx);
-int mpctx_get_global_sub_pos(MPContext *mpctx) -{ - return mpctx->global_sub_pos; + if (size) *size = mpctx->global_sub_size; + + return;
Not completely convinced it's better, but I'm happy to let you decide. However that pointless return should go. Or better yet, return on of these via the return value. My first instinct would have been the pos, but size would make the code smaller. I also think mp_property_do can fail (?), if yes it would be nice to ensure something reasonable and detectable happens in that case.
Reimar Döffinger wrote on Wed, 13 Aug 2014 21:42:24 +0200:
Not completely convinced it's better, but I'm happy to let you decide.
Without merging we'd have to ensure up-to-dateness of global_sub_pos prior to accessing it as well. This will kill two birds with one stone and leave update_global_sub_size() static.
However that pointless return should go.
Oops! That's a leftover from a version returning a success code. (See below why I decided against it.)
Or better yet, return on of these via the return value. My first instinct would have been the pos, but size would make the code smaller.
Exactly that was my quandary. Which value should be returned and which one should be in the argument list? Both seem of equal value, so put them both into the argument list making mpctx_get_global_sub_info() look less obscure.
I also think mp_property_do can fail (?), if yes it would be nice to ensure something reasonable and detectable happens in that case.
The only "failure" with the sub command occurs when passing NULL for arg/pos (resulting in not getting the information). But since that seems to be ok for calling mpctx_get_global_sub_info() and since I'm allowing NULL for size, no real failure can occur. In both cases we have nothing but simple variable assignments. Thus no reason for a return code. Ingo
On Tue, Aug 12, 2014 at 09:04:21PM +0200, Ingo Brückl wrote:
Reimar Döffinger wrote Tue, 12 Aug 2014 20:15:05 +0200:
I would think that the global sub size variable ideally should be kept up-to-date automatically. I guess that is one way to make that happen, I was just more wondering why it doesn't happen automatically once we got a new subtitle stream, instead of on every read of that variable.
On getting a new subtitle stream we only have access to the demuxer (variable), the global sub stuff is stored whithin the mpctx. I don't know whether this is necessary or a design flaw or just something that had been added later.
I could try moving the global sub information to the demuxer, but I will only learn during implementation whether this is possible at all. I cannot overview at the moment.
No, that is not possible in a reasonable way I think. However it is possible to either detect or explicitly signal a change in the number of streams. Note that I am not convinced that this will be better than your suggestion in the end, so feel free to go ahead with that. I just wanted to point out that it doesn't feel ideal.
Lode Leroy wrote on Fri, 1 Aug 2014 16:30:25 +0200:
please consider the following fix for mplayer -gui...
mpctx_get_global_sub_size(guiInfo.mpcontext); is not returning the number of subtitles, because void update_global_sub_size(MPContext *mpctx) was not called to update it.
Fixed in a different way in r37255. Ingo
participants (4)
-
compn -
Ingo Brückl -
Lode Leroy -
Reimar Döffinger