Bugzilla Bug 241110

After cleaning up use flags for arches, per bug 240347, mplayer should be fine
to rekeyword for arches.  I just didn't want to do it myself, so if you guys
can verify that the use flags are correct, please rekeyword ~ for:

=media-video/mplayer-1.0_rc2_p27458
=media-video/mplayer-1.0_rc2_p27725-r1

Target keywords: ~alpha ~x86-bsd ~ia64 ~ppc ~sparc

Thanks

Created an attachment (id=168112) [details]
mplayer-libmpeg2-libavcodec-sparc.patch

The use flags/conditions seem to be fine, but the libmpeg2 and libavcodec they
ship are broken for sparc:

1.0_rc2_p27458
  libmpeg2 is fine
  libavcodec has an undefined reference to ff_simple_idct_vis
1.0_rc2_p27725-r1
   libmpeg2 has undefined references to mpeg2_idct_copy_mvi, mpeg2_idct_add_mvi
and mpeg2_idct_alpha_init
  libavcodec has an undefined reference to ff_simple_idct_vis

I attached a unified patch but it should probably be split into two patches to
make it more flexible. Do you want to do that or do you want me to do it?
It also affects alpha but that shouldn't be a problem since alpha also needs to
re-keyword 1.0_rc2_p27725-r1.

bsd done

We need =media-video/mplayer-1.0_rc2_p27725-r1 keyworded and stabled for
security reasons. Please consider this bug a security issue.
Stabling for this version is handled in bug 239130.

(In reply to comment #1)
> 1.0_rc2_p27725-r1
>    libmpeg2 has undefined references to mpeg2_idct_copy_mvi, mpeg2_idct_add_mvi
> and mpeg2_idct_alpha_init

-#ifdef HAVE_VIS
+#ifdef ARCH_ALPHA
     if (accel & MPEG2_ACCEL_ALPHA_MVI) {
        mpeg2_idct_copy = mpeg2_idct_copy_mvi;
        mpeg2_idct_add = mpeg2_idct_add_mvi;
        mpeg2_idct_alpha_init ();
     } else


this should probably be HAVE_MVI and be sent upstream for discussion

>   libavcodec has an undefined reference to ff_simple_idct_vis

+extern void ff_simple_idct_vis(DCTELEM *data);
 inline void ff_simple_idct_vis(DCTELEM *data) {

weird; does ffmpeg-0.4.9_p20081014 exhibits the same problem ? I don't
understand why you need to define twice the function but to me this doesn't
look correct. Code hasn't changed here for a while.

> this should probably be HAVE_MVI and be sent upstream for discussion

It was ARCH_ALPHA before they changed it to HAVE_VIS, so I just changed it
back. But yes, HAVE_MVI would probably be even better. Do you want me to ping
upstream or do it yourself?

http://svn.mplayerhq.hu/mplayer/trunk/libmpeg2/idct.c?r1=26657&r2=27554

> weird; does ffmpeg-0.4.9_p20081014 exhibits the same problem ? I don't
> understand why you need to define twice the function but to me this doesn't
> look correct. Code hasn't changed here for a while.

Yes it does.
There are basicly three ways to define a function as inline:
  - static inline void function()
  - extern inline void function()
and
  - extern void function(); inline void function()
They do already have the "extern void ff_simple_idct_vis(DCTELEM *data);" but
it's in dsputil_vis.c and it also needs to be in simple_idct_vis.c(or in a
header file)
GCC 4.1 is able to compile this correctly - that's why we didn't detect it
until now - but 4.3 is not. Since our current stable is 4.1 we could ignore it
for now, but we have to fix it eventually ;)

http://www.greenend.org.uk/rjk/2003/03/inline.html

ppc is done as per #239130

So whats the status wrt sparc?
It fails for me on gcc-4.1...

I've stabilised 1.0_rc2_p27725-r1 already (sec bug 239130 )

Is there a point in keywording the older version (1.0_rc2_p27458)?

(In reply to comment #8)
> I've stabilised 1.0_rc2_p27725-r1 already (sec bug 239130 )
> 
> Is there a point in keywording the older version (1.0_rc2_p27458)?
> 

That version doesn't exist anymore. ~ia64 done

(In reply to comment #7)
> So whats the status wrt sparc?
> It fails for me on gcc-4.1...

beh, seems I hadn't seen Friedrich's reply :(

(In reply to comment #5)
> > this should probably be HAVE_MVI and be sent upstream for discussion
> 
> It was ARCH_ALPHA before they changed it to HAVE_VIS, so I just changed it
> back. But yes, HAVE_MVI would probably be even better. Do you want me to ping
> upstream or do it yourself?

They include a bundled version of libmpeg2 and wrongly patch it... nice :)
Please ping upstream, as this is sparc specific they may want more information
which i wouldn't be able to provide (though as it seems to be a small typo, I
really doubt it).

> > weird; does ffmpeg-0.4.9_p20081014 exhibits the same problem ? I don't
> > understand why you need to define twice the function but to me this doesn't
> > look correct. Code hasn't changed here for a while.
> 
> Yes it does.
> There are basicly three ways to define a function as inline:
>   - static inline void function()
>   - extern inline void function()
> and
>   - extern void function(); inline void function()
> They do already have the "extern void ff_simple_idct_vis(DCTELEM *data);" but
> it's in dsputil_vis.c and it also needs to be in simple_idct_vis.c(or in a
> header file)
> GCC 4.1 is able to compile this correctly - that's why we didn't detect it
> until now - but 4.3 is not. Since our current stable is 4.1 we could ignore it
> for now, but we have to fix it eventually ;)
> 
> http://www.greenend.org.uk/rjk/2003/03/inline.html

Ok, or maybe just drop the inline... I'm not sure there: ff_simple_idct_mmx
isn't inline as far as i can see.
Anyway, please briefly read http://ffmpeg.mplayerhq.hu/general.html#SEC28 and
send that patch to ffmpeg-devel mailing list
(https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel/). It'll quickly get
reviewed and applied/rejected; I'd really prefer to have reviewed patches as
this is a stable candidate version. Once a patch reaches ffmpeg svn, feel free
to apply it to ffmpeg & mplayer ebuilds I'd say.

(In reply to comment #10)
> (In reply to comment #7)
> > So whats the status wrt sparc?
> > It fails for me on gcc-4.1...
> 
> beh, seems I hadn't seen Friedrich's reply :(
> 
> (In reply to comment #5)
> > > this should probably be HAVE_MVI and be sent upstream for discussion
> > 
> > It was ARCH_ALPHA before they changed it to HAVE_VIS, so I just changed it
> > back. But yes, HAVE_MVI would probably be even better. Do you want me to ping
> > upstream or do it yourself?
> 
> They include a bundled version of libmpeg2 and wrongly patch it... nice :)
> Please ping upstream, as this is sparc specific they may want more information
> which i wouldn't be able to provide (though as it seems to be a small typo, I
> really doubt it).
> 
> > > weird; does ffmpeg-0.4.9_p20081014 exhibits the same problem ? I don't
> > > understand why you need to define twice the function but to me this doesn't
> > > look correct. Code hasn't changed here for a while.
> > 
> > Yes it does.
> > There are basicly three ways to define a function as inline:
> >   - static inline void function()
> >   - extern inline void function()
> > and
> >   - extern void function(); inline void function()
> > They do already have the "extern void ff_simple_idct_vis(DCTELEM *data);" but
> > it's in dsputil_vis.c and it also needs to be in simple_idct_vis.c(or in a
> > header file)
> > GCC 4.1 is able to compile this correctly - that's why we didn't detect it
> > until now - but 4.3 is not. Since our current stable is 4.1 we could ignore it
> > for now, but we have to fix it eventually ;)
> > 
> > http://www.greenend.org.uk/rjk/2003/03/inline.html
> 
> Ok, or maybe just drop the inline... I'm not sure there: ff_simple_idct_mmx
> isn't inline as far as i can see.
> Anyway, please briefly read http://ffmpeg.mplayerhq.hu/general.html#SEC28 and
> send that patch to ffmpeg-devel mailing list
> (https://lists.mplayerhq.hu/mailman/listinfo/ffmpeg-devel/). It'll quickly get
> reviewed and applied/rejected; I'd really prefer to have reviewed patches as
> this is a stable candidate version. Once a patch reaches ffmpeg svn, feel free
> to apply it to ffmpeg & mplayer ebuilds I'd say.
> 

aballier, would you mind pinging them yourself since you know what you're
talking about? :)  I'd have a hard time explaining it if any objections came
up.

(In reply to comment #11)
> aballier, would you mind pinging them yourself since you know what you're
> talking about? :)  I'd have a hard time explaining it if any objections came
> up.

yes i could too, but that's the same for me: i'd have hard time
explaining/doing further tests if some objections come up; i'd need a machine
to test it which i don't have :/

(In reply to comment #12)
> (In reply to comment #11)
> > aballier, would you mind pinging them yourself since you know what you're
> > talking about? :)  I'd have a hard time explaining it if any objections came
> > up.
> 
> yes i could too, but that's the same for me: i'd have hard time
> explaining/doing further tests if some objections come up; i'd need a machine
> to test it which i don't have :/
> 

We can provide access to it. Ping me on irc if you want to

Thanks aballier,

patch accepted upstream:

http://lists.mplayerhq.hu/pipermail/mplayer-cvslog/2008-November/035850.html

I'm going to apply the patch to the same ebuild (1.0_rc2_p27725-r1).  Every
arch is stable but sparc, can you please test /stable it 

Sparc stable for 1.0_rc2_p27725-r1, closing since we're last :)