Don't use pthread for libvpx in mingw builds.

RESOLVED FIXED in Firefox 40

Status

()

Core
Audio/Video
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Jacek Caban, Assigned: Jacek Caban)

Tracking

Trunk
mozilla41
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox39 unaffected, firefox40 fixed, firefox41 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Created attachment 8625317 [details] [diff] [review]
patch

mingw can optionally have pthread support, but even if it does, it doesn't make sense to use it on Windows when support for native Windows threading is provided anyway. Currently, mingw build without pthread fails.
Attachment #8625317 - Flags: review?(j)

Comment 1

3 years ago
Comment on attachment 8625317 [details] [diff] [review]
patch

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

Patch looks ok but needs to be added to update.py so its not lost with the next update.
Attachment #8625317 - Flags: review?(j) → review-

Comment 2

3 years ago
Created attachment 8625425 [details] [diff] [review]
0001-Bug-1176730-Don-t-use-pthread-for-libvpx-in-mingw-bu.patch
Attachment #8625317 - Attachment is obsolete: true
Attachment #8625425 - Flags: review?(giles)
Comment on attachment 8625425 [details] [diff] [review]
0001-Bug-1176730-Don-t-use-pthread-for-libvpx-in-mingw-bu.patch

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

::: media/libvpx/vpx_config_x86-win32-gcc.h
@@ +35,4 @@
>  #define HAVE_VPX_PORTS 1
>  #define HAVE_STDINT_H 1
>  #define HAVE_ALT_TREE_LAYOUT 0
> +#undef HAVE_PTHREAD_H

Why is the undef necessary?
(Assignee)

Comment 4

3 years ago
(In reply to Ralph Giles (:rillian) from comment #3)
> Comment on attachment 8625425 [details] [diff] [review]
> 0001-Bug-1176730-Don-t-use-pthread-for-libvpx-in-mingw-bu.patch
> 
> Review of attachment 8625425 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/libvpx/vpx_config_x86-win32-gcc.h
> @@ +35,4 @@
> >  #define HAVE_VPX_PORTS 1
> >  #define HAVE_STDINT_H 1
> >  #define HAVE_ALT_TREE_LAYOUT 0
> > +#undef HAVE_PTHREAD_H
> 
> Why is the undef necessary?

We have a check for pthread.h in configure script. If it's found then HAVE_PTHREAD_H is defined in mozilla-config.h and we have a warning here.
Comment on attachment 8625425 [details] [diff] [review]
0001-Bug-1176730-Don-t-use-pthread-for-libvpx-in-mingw-bu.patch

Ok, looks fine to me.
Attachment #8625425 - Flags: review?(giles) → review+

Comment 6

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/30f68d9c65a5
https://hg.mozilla.org/mozilla-central/rev/30f68d9c65a5
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox41: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment on attachment 8625425 [details] [diff] [review]
0001-Bug-1176730-Don-t-use-pthread-for-libvpx-in-mingw-bu.patch

Approval Request Comment
[Feature/regressing bug #]: Bug 1151175
[User impact if declined]: This is needed for uplift of newer libvpx changes.
[Describe test coverage new/current, TreeHerder]: Landed on m-c some time ago.
[Risks and why]: Risk is minimal. This doesn't affect officially supported builds.
[String/UUID change made/needed]: None.
Attachment #8625425 - Flags: approval-mozilla-beta?
Blocks: 1178215
status-firefox39: --- → unaffected
status-firefox40: --- → affected
Flags: needinfo?(abillings)
Comment on attachment 8625425 [details] [diff] [review]
0001-Bug-1176730-Don-t-use-pthread-for-libvpx-in-mingw-bu.patch

As Ralph stated, this is a no-op for officially supported builds. Beta+
Attachment #8625425 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Don't we need this on Aurora as well?
Flags: needinfo?(abillings)
https://hg.mozilla.org/releases/mozilla-beta/rev/b92f62aa6643

(In reply to Al Billings [:abillings] from comment #10)
> Don't we need this on Aurora as well?

This landed prior to the uplift.
status-firefox40: affected → fixed
You need to log in before you can comment on or make changes to this bug.