793229 - Some build configs broken by shadowed constant in OmxPlugin.cpp

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Our friends reported this.  I guess they build with more anal -W than we do :).

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Remove shadowed constant

Does this do the trick?

Comment 2

[Chris Peterson [:cpeterson]]

cjones, what build config are they using?

This change is related to bug 792988, a fix for my Otoro build break. OmxPlugin.cpp's OmxDecoder::ToVideoFrame() still has a local constant that is shadowing the name OMX_QCOM_COLOR_FormatYVU420PackedSemiPlanar32m4ka. AFAIK, the Otoro build fix only needs the local constant OMX_QCOM_COLOR_FormatYVU420SemiPlanar.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Chris Peterson (:cpeterson) from comment #2)
> cjones, what build config are they using?
> 

No clue.

> This change is related to bug 792988, a fix for my Otoro build break.
> OmxPlugin.cpp's OmxDecoder::ToVideoFrame() still has a local constant that
> is shadowing the name OMX_QCOM_COLOR_FormatYVU420PackedSemiPlanar32m4ka.
> AFAIK, the Otoro build fix only needs the local constant
> OMX_QCOM_COLOR_FormatYVU420SemiPlanar.

Yeah, my patch didn't deal with your change correctly.

Comment 4

[Diego Wilson [:diego]]

Comment on attachment 663464 [details] [diff] [review]
Remove shadowed constant

Review of attachment 663464 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/omx-plugin/OmxPlugin.cpp
@@ -24,5 @@
>  
>  using namespace MPAPI;
>  
> -const int OMX_QCOM_COLOR_FormatYVU420PackedSemiPlanar32m4ka = 0x7FA30C01;
> -

Yep, this does the trick

Comment 5

[Chris Peterson [:cpeterson]]

(In reply to Diego Wilson from comment #4)
> Comment on attachment 663464 [details] [diff] [review]
> Remove shadowed constant
> 
> Review of attachment 663464 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/omx-plugin/OmxPlugin.cpp
> @@ -24,5 @@
> >  
> >  using namespace MPAPI;
> >  
> > -const int OMX_QCOM_COLOR_FormatYVU420PackedSemiPlanar32m4ka = 0x7FA30C01;
> > -
> 
> Yep, this does the trick

Diego, what is your build config (platform, toolchain)? I'd like to avoid breaking other people's code in the future, but our Tinderbox builds only test a few configurations.

Comment 6

[Chris Peterson [:cpeterson]]

Comment on attachment 663464 [details] [diff] [review]
Remove shadowed constant

Review of attachment 663464 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.

I will post a separate patch later to see if the local definition of OMX_QCOM_COLOR_FormatYVU420PackedSemiPlanar32m4ka inside OmxDecoder::ToVideoFrame() is still necessary.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e96de9481915

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e96de9481915

Comment 9

[Diego Wilson [:diego]]

I'm using ARM EABI GCC 4.4.3 on an Ubuntu 10.04 box