1208935 - Deinterlacer class definitions are not linked in when skia is disabled

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Steve Singer (:stevensn)]

The class definitions for Deinterlacer are part of Downscaler.cpp but Downscaler.cpp isn't linked in when skia is disabled.  This causes non-skia builds to fail with a link error

Comment 1

[Steve Singer (:stevensn)]

Attachment: split_out_deinterlacer.diff

Comment 2

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8666565 [details] [diff] [review]
split_out_deinterlacer.diff

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

If Deinterlacer needs to be in its own .cpp file, it should get its own header file as well.

Please move Deinterlacer to a new Deinterlacer.h header file. Only nsGIFDecoder2.h should need to include it, if I recall correctly.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Also, can we write a test so this doesn't keep happening? It would probably require more build system skills than I have, unfortunately.

Comment 4

[Steve Singer (:stevensn)]

Attachment: split_out_deinterlacer.diff

Patch updated to have Deinterlacer.h as a standalone file

I have no idea how to detect this type of thing with a test when none of the try platforms build with --disable-skia

Comment 5

[Steve Singer (:stevensn)]

Attachment: split_out_deinterlacer.diff

Comment 6

[Steve Singer (:stevensn)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a18b191e0113

Comment 7

[Seth Fowler [:seth] [:s2h]]

(In reply to Steve Singer (:stevensn) from comment #4)
> I have no idea how to detect this type of thing with a test when none of the
> try platforms build with --disable-skia

Yeah, me neither. We'd need to somehow generate a --disable-skia object file even on platforms that normally have Skia enabled.

Comment 8

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8667085 [details] [diff] [review]
split_out_deinterlacer.diff

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

Looks good; thanks very much for fixing this, Steve!

Comment 9

[Pulsebot]

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

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 11

[Steve Singer (:stevensn)]

Attachment: split_out_deinterlacer.diff - Aurora

version of patch rebased for aurora

Comment 12

[Steve Singer (:stevensn)]

Comment on attachment 8669359 [details] [diff] [review]
split_out_deinterlacer.diff - Aurora

Approval Request Comment
[Feature/regressing bug #]:1194058
[User impact if declined]: non-skia platforms (tier-3) won't build
[Describe test coverage new/current, TreeHerder]: tested on m-c
[Risks and why]: Low, patch moves existing code to a new file
[String/UUID change made/needed]: none

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8669359 [details] [diff] [review]
split_out_deinterlacer.diff - Aurora

Approved for uplift to aurora, should fix a build issue

Comment 14

[Carsten Book [:Tomcat]]

Hi, this failed to apply:

applying split_out_deinterlacer.diff
patching file image/Downscaler.h
Hunk #1 FAILED at 147
1 out of 1 hunks FAILED -- saving rejects to file image/Downscaler.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and refresh split_out_deinterlacer.diff

Comment 15

[Steve Singer (:stevensn)]

Attachment: split_out_deinterlacer.diff - Aurora

This version have the patched has been refreshed against aurora

Comment 16

[Carsten Book [:Tomcat]]

https://treeherder.mozilla.org/#/jobs?repo=mozilla-aurora&revision=f944f0a51702

thanks steve!