1324183 - Remove Windows Vista and 7 version checks in dom/media

Status: RESOLVED FIXED

(Core :: Audio/Video: Playback, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: part-1-remove-Windows-version-checks.patch

Now that Firefox 53 no longer supports Windows XP or Vista, we can remove unnecessary Windows version checks for Vista and 7.

Comment 1

[Chris Peterson [:cpeterson]]

Attachment: part-2-remove-MediaUnsupportedBeforeWindowsVista.patch

Part 2: Remove MediaUnsupportedBeforeWindowsVista console message.

Comment 2

[Chris Peterson [:cpeterson]]

I have some more IsVistaOrLater() removals from dom/media/ coming in another patch.

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8819516 [details] [diff] [review]
part-1-remove-Windows-version-checks.patch

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

r+

You should try mozreview&autoland!

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8819517 [details] [diff] [review]
part-2-remove-MediaUnsupportedBeforeWindowsVista.patch

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

r+ if you also remove the code that report 'sMediaUnsupportedBeforeWindowsVista' (lines 548,552-557 in the original version; search "vista" to find them)*.

You may also want to remove similar XP&Vista code in browser/base/content/browser-media.js and browser/locales/en-US/chrome/browser/browser.properties, but please r?gijs for it. It could be done in a separate bug if you prefer.

* See, if you had used mozreview, it would have made it easier for me to check this, and even to auto-quote the relevant part and flag the issue. ;-)

Comment 5

[mirh]

Just for the records, I read bug 1146362 for the original reporting of the 4K videos Vista breakage, and it seems more of a driver/gpu shortcoming. 

Shouldn't you blacklist the adapters without the required hardware decoding if anything?
For example, on a fully updated Windows 7 with a Radeon 7750 and latest driver, even 1440p is already unusable (and oddly CPU isn't maxed out).

Comment 6

[Chris Peterson [:cpeterson]]

(In reply to Gerald Squelart [:gerald] from comment #4)
> Review of attachment 8819517 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ if you also remove the code that report
> 'sMediaUnsupportedBeforeWindowsVista' (lines 548,552-557 in the original
> version; search "vista" to find them)*.

Sorry. I forgot to hg qrefresh some local changes.

> * See, if you had used mozreview, it would have made it easier for me to
> check this, and even to auto-quote the relevant part and flag the issue. ;-)

Sorry again and thanks for the push. I will finally move to mozreview! <:)

Comment 7

[Chris Peterson [:cpeterson]]

(In reply to mirh from comment #5)
> Just for the records, I read bug 1146362 for the original reporting of the
> 4K videos Vista breakage, and it seems more of a driver/gpu shortcoming. 
> 
> Shouldn't you blacklist the adapters without the required hardware decoding
> if anything?
> For example, on a fully updated Windows 7 with a Radeon 7750 and latest
> driver, even 1440p is already unusable (and oddly CPU isn't maxed out).

Matt, you disabled hardware decoding on XP and Vista in bug 1154536. mirh wonders whether we should be blocklisting the relevant drivers instead of Windows versions.

Comment 8

[mirh]

(In reply to Chris Peterson [:cpeterson] from comment #7)
> Matt, you disabled hardware decoding on XP and Vista in bug 1154536. mirh
> wonders whether we should be blocklisting the relevant drivers instead of
> Windows versions.

I was specifically referring to lines 38-44 of your first patch (which thus would now be fixed). 

But in a more general sense, yes I was wondering why that choice in the first place: it looks pretty hurried and tentative, and perhaps the same mindset might have led to some other unexpected effect too. 
See also bug 1130266, comment 19

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a768dc05d7e
Part 1: Remove some Windows Vista and 7 version checks in dom/media/. r=gerald
https://hg.mozilla.org/integration/mozilla-inbound/rev/86c08096f8c4
Part 2: Remove MediaUnsupportedBeforeWindowsVista console message. r=gerald

Comment 10

[Matt Woodrow (:mattwoodrow)]

(In reply to Chris Peterson [:cpeterson] from comment #7)
> (In reply to mirh from comment #5)
> > Just for the records, I read bug 1146362 for the original reporting of the
> > 4K videos Vista breakage, and it seems more of a driver/gpu shortcoming. 
> > 
> > Shouldn't you blacklist the adapters without the required hardware decoding
> > if anything?
> > For example, on a fully updated Windows 7 with a Radeon 7750 and latest
> > driver, even 1440p is already unusable (and oddly CPU isn't maxed out).
> 
> Matt, you disabled hardware decoding on XP and Vista in bug 1154536. mirh
> wonders whether we should be blocklisting the relevant drivers instead of
> Windows versions.

I didn't disable hardware decoding on XP/Vista, I stopped us reporting that we could play 4k videos at all (hardware or otherwise).

Neither XP nor Vista support 4k video decoding in hardware (on any hardware), so these videos would always be software.

The vista/XP check here is being used a proxy for processor speed (albeit not a very good one), so just removing it is fine.

Comment 11

[mirh]

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> I didn't disable hardware decoding on XP/Vista, I stopped us reporting that
> we could play 4k videos at all (hardware or otherwise).

I didn't mention general hardware decoding myself to be honest (Chris did)
Anyway, I was just wondering why there was that awkward check for 4k, other factors are definitively at stake. 
Checking for DXVA reported bits for example seemed pretty more comprehensive. 

> Neither XP nor Vista support 4k video decoding in hardware (on any
> hardware), so these videos would always be software.

AMD supports it starting with UVD 5 (GCN 1.2 cards). Which indeed have no Vista driver, true.
Then you have Intel, that for as much included a decoder all the way back to Ivy Bridge (which have also drivers for XP) possibly doesn't count that much given there's no way to "retro-fit" integrated graphics

But then you have nvidia, that only dropped those OS months ago (PureVideo HD5 starting from Kepler and a couple of Fermi chips btw). 

> The vista/XP check here is being used a proxy for processor speed (albeit
> not a very good one), so just removing it is fine.

Oh right, software decoding is a thing too, I was forgetting lol. 
I'm not sure on the rationale behind that for 4K though. 

Yes I mean.. Perhaps with some beefy (at least 4 core) desktop CPU that should be possible, but putting aside it still seems a huge energy waste, I really wouldn't know how one would be able to discern between an Atom with hyper-threading and a Xeon.

Comment 12

[Masatoshi Kimura [:emk]]

The discussion is going off-topic. Perhaps we should file a new bug to investigate a better way to determine whether we offer 4K.

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9a768dc05d7e
https://hg.mozilla.org/mozilla-central/rev/86c08096f8c4

Comment 14

[mirh]

(In reply to Masatoshi Kimura [:emk] from comment #12)
> The discussion is going off-topic. Perhaps we should file a new bug to
> investigate a better way to determine whether we offer 4K.

Sure, sorry. 
Decide yourself if it would be worth then.

Comment 15

[Vangelis]

Staying on topic (I think), I am still testing mozilla-central (firefox 53.0a1) on Windows Vista SP2 32bit here, and I can confirm that this bug 1324183 TOTALLY BROKE h264/aac playback on that OS (through 
WMF, with Platform Update Supplement for WinVistaSP2 (KB2117917) installed).

Last good build: 
buildID=20161218030213
https://hg.mozilla.org/mozilla-central/rev/a3ce1fce4f15616f66ac328e4a562d0117c93a0d

Proof: http://i.imgur.com/6u68iku.jpg

First bad build:
buildID=20161219030207
https://hg.mozilla.org/mozilla-central/rev/863c2b61bd27bb6099104933134d3be7c052551a

Proof: http://i.imgur.com/0HGmlaO.jpg

The net effect of this bug on a fully updated Vista OS is the same as toggling pref 
"media.wmf.enabled;true" on a pre20161219 build.
But this pref is now irrelevant after bug 1324183 landed on the Dec 19th 2016 nightly build...

Comment 16

[Chris Peterson [:cpeterson]]

Vangelis, Firefox 53 and later no longer support Windows XP or Vista (bug 1130266). XP and Vista users running Firefox Beta or Release versions will be switched to the Firefox 52 ESR version, which will continue to support XP and Vista until 2018 Q1.

btw, how did you install those Nightly 53 builds? XP and Vista users running Nightly or Dev Edition should have stopped receiving nightly updates a few weeks ago (bug 1317780) and the Nightly and Dev Edition installers should prevent XP and Vista users from installing (bug 1305453).

Comment 17

[mirh]

(In reply to Chris Peterson [:cpeterson] from comment #16)
> btw, how did you install those Nightly 53 builds? XP and Vista users running
> Nightly or Dev Edition should have stopped receiving nightly updates a few
> weeks ago (bug 1317780) and the Nightly and Dev Edition installers should
> prevent XP and Vista users from installing (bug 1305453).

Cause you haven't only installers for nightlies https://ftp.mozilla.org/pub/firefox/nightly/2016/12/2016-12-19-03-02-07-mozilla-central/ :p

Though.. I thought you didn't only rely on WMF for H264 (bug 1210231)?

Comment 18

[Nathan Froyd [:froydnj]]

(In reply to mirh from comment #17)
> (In reply to Chris Peterson [:cpeterson] from comment #16)
> > btw, how did you install those Nightly 53 builds? XP and Vista users running
> > Nightly or Dev Edition should have stopped receiving nightly updates a few
> > weeks ago (bug 1317780) and the Nightly and Dev Edition installers should
> > prevent XP and Vista users from installing (bug 1305453).
> 
> Cause you haven't only installers for nightlies
> https://ftp.mozilla.org/pub/firefox/nightly/2016/12/2016-12-19-03-02-07-
> mozilla-central/ :p

Heh.  Thanks for the pointer; I filed bug 1324790 to give an error message for these kinds of cases.  Not out of spite, mind you, but because we're going to start using APIs elsewhere in the browser that don't exist on XP/Vista, and it'd be nice to give a reasonable error message up front rather than running into inscrutable errors later on.

Comment 19

[mirh]

(In reply to Nathan Froyd [:froydnj] from comment #18)
> Heh.  Thanks for the pointer; I filed bug 1324790 to give an error message
> for these kinds of cases.  Not out of spite, mind you, but because we're
> going to start using APIs elsewhere in the browser that don't exist on
> XP/Vista, and it'd be nice to give a reasonable error message up front
> rather than running into inscrutable errors later on.

Well, shouldn't the OS already display such an error message in that case?

Also, as I said in bug 1130266, comment 19, if we are talking about Vista: that's *already* the case. 
AFAIK basically everything you could touch without also affecting W7 is already broken. 

Unless you are going to refine its support this last year in the ESR branch, I don't even see how FF52 and FF53 could even differ on this OS.

Comment 20

[Nathan Froyd [:froydnj]]

(In reply to mirh from comment #19)
> (In reply to Nathan Froyd [:froydnj] from comment #18)
> > Heh.  Thanks for the pointer; I filed bug 1324790 to give an error message
> > for these kinds of cases.  Not out of spite, mind you, but because we're
> > going to start using APIs elsewhere in the browser that don't exist on
> > XP/Vista, and it'd be nice to give a reasonable error message up front
> > rather than running into inscrutable errors later on.
> 
> Well, shouldn't the OS already display such an error message in that case?

Sure.  But it'd be better for the error to be "You can't run Firefox on this version of Windows" or somesuch rather than "cannot find library symbol FrobAllTheThings" or similar.

> Also, as I said in bug 1130266, comment 19, if we are talking about Vista:
> that's *already* the case. 
> AFAIK basically everything you could touch without also affecting W7 is
> already broken. 

I have no opinion here, as I know very little about what APIs are supported where on Windows.  But bug 1130266 comment 10's investigation suggests that there might be some sandboxing APIs that are only supported on Windows 7 and above?  Limited googling of the functions mentioned suggests that many of the APIs were added in XP, though... *shrug*

> Unless you are going to refine its support this last year in the ESR branch,
> I don't even see how FF52 and FF53 could even differ on this OS.

ESR is for security and regression fixes, not for any refinement of functionality or support.

Comment 21

[mirh]

(In reply to Nathan Froyd [:froydnj] from comment #20)
> Sure.  But it'd be better for the error to be "You can't run Firefox on this
> version of Windows" or somesuch rather than "cannot find library symbol
> FrobAllTheThings" or similar.

Mhh, makes sense.. insofar as people downloading firefox from ftp server doesn't *really* seem the kind that would need such a reminder, if I can explain. 

> I have no opinion here, as I know very little about what APIs are supported
> where on Windows.  But bug 1130266 comment 10's investigation suggests that
> there might be some sandboxing APIs that are only supported on Windows 7 and
> above?  Limited googling of the functions mentioned suggests that many of
> the APIs were added in XP, though... *shrug* 

https://codereview.chromium.org/810083002#msg5
After quite a lot of digging, it seems they just decided to forbear Vista altogether, in any way (likely for the same venial reasons I mentioned thence)
As for XP, it truly didn't support the thing https://msdn.microsoft.com/en-us/library/windows/desktop/aa965848.aspx

Regardless, afaik Mozilla decided to drop XP and Vista for the burden (of guaranteeing a satisfactory experience) compared to the small user base. 
But for as much some users begged on continued XP support on the basis that it's still very popular (ignoring the fact its oldness easily beat that), I feel like on the other hand developers are ignoring Vista is already in its worst possible state. 

> ESR is for security and regression fixes, not for any refinement of
> functionality or support.

And following the aforementioned wondering, I hardly see a sense for this indeed.