Closed Bug 944977 Opened 6 years ago Closed 6 years ago

Build libvorbis in unified mode

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: cpeterson, Assigned: cpeterson)

References

Details

(Whiteboard: [qa-])

Attachments

(2 files, 1 obsolete file)

Attached patch unify-libvorbis-libvpx.patch (obsolete) — Splinter Review
Move of libvorbis and libvpx could be built in unified mode, but some source files contain duplicate functions. Since these are third-party libraries and I didn't want to change the code in our tree, I just compiled those individual files in non-unified mode.
Attachment #8340743 - Flags: review?(chris.double)
In this try build, the ambiguously named changeset "Bug NUMBER - Part NUMBER: DESCRIPTION" is this unify-libvorbis-libvpx.patch. <:)

https://tbpl.mozilla.org/?tree=Try&rev=7b53eba8bf2e
Please don't touch libvpx until bug 763495 lands.
Patch v2: Build media/libvorbis in unified mode.

As per glandium's comment 2, I removed libvpx from this patch so it only builds libvorbis in unified mode.
Attachment #8340743 - Attachment is obsolete: true
Attachment #8340743 - Flags: review?(chris.double)
Attachment #8340791 - Flags: review?(chris.double)
Comment on attachment 8340791 [details] [diff] [review]
unify-libvorbis-v2.patch

I think the maintainers of the libraries probably know more than me about what traps are involved in making these unified so they should probably review. Maybe Tim Terriberry or Ralph Giles.
Attachment #8340791 - Flags: review?(chris.double)
Summary: Build libvorbis and libvpx in unified mode → Build libvorbis in unified mode
Attachment #8340791 - Flags: review?(tterribe)
Comment on attachment 8340791 [details] [diff] [review]
unify-libvorbis-v2.patch

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

r=me with a few nits.

::: media/libvorbis/moz.build
@@ +29,5 @@
> +]
> +
> +# These files can't be unified because of function redefinitions.
> +SOURCES += [
> +    'lib/vorbis_codebook.c',

You should kick out vorbis_sharedbook.c instead, since that only contains setup/teardown code, while vorbis_codebook.c contains functions that are called many times per packet, and should be optimized together with the bulk of the codec.

@@ +37,1 @@
>      'lib/vorbis_window.c',

There's no actual conflict here, AFAICT. The problem is that the function in the .c file does not match that in the .h file.

I fixed this upstream in r19028. It's okay if you don't want to backport that fix, but you should leave a comment, so we'll remember to move this file back the next time we pull an update of libvorbis.
Attachment #8340791 - Flags: review?(tterribe) → review+
I updated media/libvorbis/update.sh to apply a patch for libvorvis r19028.

btw, upstream r19028 will introduce some clang -Wincompatible-pointer-types-discards-qualifiers warnings. My bug944977-r19028.patch file also fixes those warnings locally.


media/libvorbis/lib/vorbis_block.c:774:18: warning: initializing 'float *' with an expression of type 'const float *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]
Making all in include
          float *w=_vorbis_window_get(b->window[1]-hs);
                 ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

media/libvorbis/lib/vorbis_block.c:781:18: warning: initializing 'float *' with an expression of type 'const float *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]
          float *w=_vorbis_window_get(b->window[0]-hs);
                 ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

media/libvorbis/lib/vorbis_block.c:790:18: warning: initializing 'float *' with an expression of type 'const float *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]
          float *w=_vorbis_window_get(b->window[0]-hs);
                 ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

media/libvorbis/lib/vorbis_block.c:799:18: warning: initializing 'float *' with an expression of type 'const float *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]
          float *w=_vorbis_window_get(b->window[0]-hs);
                 ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

media/libvorbis/lib/vorbis_block.c:1045:10: warning: returning 'const float *' from a function with result type 'float *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]
  return _vorbis_window_get(b->window[W]-hs);
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Attachment #8341504 - Flags: review?(tterribe)
Comment on attachment 8341504 [details] [diff] [review]
part-1-backport-libvorbis-r19028.patch

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

Good call. I fixed these (and various other warnings) upstream in r19031.
Attachment #8341504 - Flags: review?(tterribe) → review+
https://hg.mozilla.org/mozilla-central/rev/727a945e6ddc
https://hg.mozilla.org/mozilla-central/rev/bbdfac4aa444
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.