Closed Bug 1398102 Opened 2 years ago Closed 2 years ago

canPlayType() suggests "probably" even if codecs parameter is empty

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: annevk, Assigned: JamesCheng)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

I will take a look of this bug first.
Assignee: nobody → jacheng
Priority: -- → P3
Hi Chris,

It's weird that I run

"./mach test testing/web-platform/tests/html/semantics/embedded-content/media-elements/mime-types/canPlayType.html" without my patch, the result is "PASS".

When running by "./mach run" then navigate to http://www.w3c-test.org/html/semantics/embedded-content/media-elements/mime-types/canPlayType.html

The result is "Fail" as expected.

Do you know what makes the difference?



Thank you!
Flags: needinfo?(cpearce)
Attachment #8906531 - Flags: review?(cpearce)
Attachment #8906531 - Flags: review?(cpearce)
Sorry for the noise

The latest WPT test have tested these three cases [1]

t(type + ';', 'maybe');
t(type + ';codecs', 'maybe');
t(type + ';codecs=', 'maybe');

We only tested [2]
t(type, 'maybe');

So the code still needs to be reviewed.

[1]
https://github.com/w3c/web-platform-tests/pull/7294/commits/69c80784a43645899a531ef4caf5453e2d6c22e5#diff-2f0122405e19db390c20f960e276c86eR57

[2]
http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/testing/web-platform/tests/html/semantics/embedded-content/media-elements/mime-types/canPlayType.html#54
Flags: needinfo?(cpearce)
(*containerType).ExtendedType().Codecs().IsEmpty() will return true (I used it in this patch)

but

(*containerType).ExtendedType().HaveCodecs() will return false in these three cases

t(type + ';', 'maybe');
t(type + ';codecs', 'maybe');
t(type + ';codecs=', 'maybe');

will file another bug to fix this.

ni gerald to let him know I will do some change according to this.
As comment 6 said.  thanks.
Flags: needinfo?(gsquelart)
Thank you for letting me know.

I see that mHaveCodecs is computed there:
http://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/dom/media/MediaMIMETypes.cpp#194-195
It's based on what nsContentTypeParser gives us when requesting 'codecs', so it may not be trivial to fix that... Good luck!

If you get it done, please add appropriate tests in TestMediaMIMETypes.cpp as well.
Flags: needinfo?(gsquelart)
Thinking about it further, if I understand the issue correctly, you shouldn't need to modify HaveCodecs: Isn't the fact that it returns false (in your comment 6 examples) enough to return 'maybe' for types where codecs are expected?

(But I may just be confused about the whole thing -- happy to discuss later if you'd like)
I'll let gerald handle this.
Attachment #8906531 - Flags: review?(cpearce) → review?(gsquelart)
Comment on attachment 8906531 [details]
Bug 1398102 - [Part1] canPlayType should return 'maybe' if the codec parameter is empty.

https://reviewboard.mozilla.org/r/178280/#review183608

r+ with following suggestion (or similar) for the comment; and please open a new bug for that TODO (you could even open the new bug first, and put its number in the TODO comment here).

::: dom/html/HTMLMediaElement.cpp:4650
(Diff revision 2)
> +  if (status == CANPLAY_YES &&
> +      (*containerType).ExtendedType().Codecs().IsEmpty()) {
> +    // TODO: Fix this per the spec on bug 1398102.

That TODO is a bit confusing to me, as this is the patch for bug 1398102!
How about:
    // Per spec: 'Generally, a user agent should never return "probably" for a
    // type that allows the `codecs` parameter if that parameter is not present.'
    // As all our currently-supported types allow for `codecs`, we can do this
    // check here.
    // TODO: Instead, missing `codecs` should be checked in each decoder's
    // `IsSupportedType` call from `CanHandleCodecsType()`.
Attachment #8906531 - Flags: review?(gsquelart) → review+
Blocks: 1399023
(In reply to Gerald Squelart [:gerald] from comment #9)
> Thinking about it further, if I understand the issue correctly, you
> shouldn't need to modify HaveCodecs: Isn't the fact that it returns false
> (in your comment 6 examples) enough to return 'maybe' for types where codecs
> are expected?
> 
> (But I may just be confused about the whole thing -- happy to discuss later
> if you'd like)

Thank you gerald!

I got typo and I need to clarify it again.

The only case that HaveCodecs reports the wrong value( is

t(type + ';codecs=', 'maybe');
These two case are OK.
t(type + ';', 'maybe');
t(type + ';codecs', 'maybe');

But it is more reasonable to do it in your suggested TODO in Bug 1399023 .

Thanks.
(In reply to James Cheng[:JamesCheng] from comment #12)
> The only case that HaveCodecs reports the wrong value is
> t(type + ';codecs=', 'maybe');

I see, it makes more sense.
Then I would argue that HaveCodecs is correct, in that there *is* a provided 'codecs' parameter, it just happens to have an empty value. ;-)

So in the end I think your existing test using Codecs().Empty() should be enough, don't you think?
(And it can be moved to the decoders later on.)
Pushed by jacheng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/939ecf1b80cb
canPlayType should return 'maybe' if the codec parameter is empty. r=gerald
(In reply to Gerald Squelart [:gerald] from comment #13)
> (In reply to James Cheng[:JamesCheng] from comment #12)
> > The only case that HaveCodecs reports the wrong value is
> > t(type + ';codecs=', 'maybe');
> 
> I see, it makes more sense.
> Then I would argue that HaveCodecs is correct, in that there *is* a provided
> 'codecs' parameter, it just happens to have an empty value. ;-)
> 
> So in the end I think your existing test using Codecs().Empty() should be
> enough, don't you think?
> (And it can be moved to the decoders later on.)

I totally agree.

Thank you!
If I make that `type + ';codecs=,'` we still consider it empty I hope? Or with a space of sorts.
Backed out for failing mda's dom/media/test/test_can_play_type_wave.html:

https://hg.mozilla.org/integration/autoland/rev/1c376c20595a1468a84fbfc4ef23558a4577571a

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=939ecf1b80cbd72a1a908a68578c92811b3a5ead&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=130295298&repo=autoland

[task 2017-09-12T08:30:24.370755Z] 08:30:24     INFO - TEST-START | dom/media/test/test_can_play_type_wave.html
[task 2017-09-12T08:30:24.495559Z] 08:30:24     INFO - GECKO(1123) | --DOCSHELL 0x7f38a7b41000 == 2 [pid = 1178] [id = {fd27e707-c74c-4b80-9cd3-2967693d5a0d}]
[task 2017-09-12T08:30:24.495713Z] 08:30:24     INFO - GECKO(1123) | [Child 1178] WARNING: stylo: ServoStyleSets cannot respond to document state changes yet (only matters for chrome documents). See bug 1290285.: file /builds/worker/workspace/build/src/layout/base/PresShell.cpp, line 4295
[task 2017-09-12T08:30:24.542566Z] 08:30:24     INFO - GECKO(1123) | ++DOMWINDOW == 8 (0x7f38a7b4b800) [pid = 1178] [serial = 64] [outer = 0x7f38a7b0e000]
[task 2017-09-12T08:30:25.249233Z] 08:30:25     INFO - GECKO(1123) | [Child 1178] WARNING: stylo: Web Components not supported yet: file /builds/worker/workspace/build/src/dom/base/nsDocument.cpp, line 6413
[task 2017-09-12T08:30:25.250259Z] 08:30:25     INFO - GECKO(1123) | [Child 1178] WARNING: stylo: Web Components not supported yet: file /builds/worker/workspace/build/src/dom/base/nsDocument.cpp, line 6413
[task 2017-09-12T08:30:25.390449Z] 08:30:25     INFO - GECKO(1123) | ++DOCSHELL 0x7f38a83c3000 == 3 [pid = 1178] [id = {c4ab53f0-6ee4-49bb-9435-a58b5879ee4f}]
[task 2017-09-12T08:30:25.391353Z] 08:30:25     INFO - GECKO(1123) | ++DOMWINDOW == 9 (0x7f38a83c3800) [pid = 1178] [serial = 65] [outer = (nil)]
[task 2017-09-12T08:30:25.413378Z] 08:30:25     INFO - GECKO(1123) | ++DOMWINDOW == 10 (0x7f38a83c4800) [pid = 1178] [serial = 66] [outer = 0x7f38a83c3800]
[task 2017-09-12T08:30:25.489961Z] 08:30:25     INFO - TEST-INFO | started process screentopng
[task 2017-09-12T08:30:25.923151Z] 08:30:25     INFO - TEST-INFO | screentopng: exit 0
[task 2017-09-12T08:30:25.923415Z] 08:30:25     INFO - Buffered messages logged at 08:30:25
[task 2017-09-12T08:30:25.925317Z] 08:30:25     INFO - TEST-PASS | dom/media/test/test_can_play_type_wave.html | A valid string reason is expected 
[task 2017-09-12T08:30:25.926414Z] 08:30:25     INFO - TEST-PASS | dom/media/test/test_can_play_type_wave.html | Reason cannot be empty 
[task 2017-09-12T08:30:25.927382Z] 08:30:25     INFO - TEST-PASS | dom/media/test/test_can_play_type_wave.html | audio/wave 
[task 2017-09-12T08:30:25.928338Z] 08:30:25     INFO - TEST-PASS | dom/media/test/test_can_play_type_wave.html | audio/wav 
[task 2017-09-12T08:30:25.929929Z] 08:30:25     INFO - TEST-PASS | dom/media/test/test_can_play_type_wave.html | audio/x-wav 
[task 2017-09-12T08:30:25.931837Z] 08:30:25     INFO - TEST-PASS | dom/media/test/test_can_play_type_wave.html | audio/x-pn-wav 
[task 2017-09-12T08:30:25.933733Z] 08:30:25     INFO - TEST-PASS | dom/media/test/test_can_play_type_wave.html | audio/wave; codecs=1 
[task 2017-09-12T08:30:25.935819Z] 08:30:25     INFO - TEST-PASS | dom/media/test/test_can_play_type_wave.html | audio/wave; codecs=6 
[task 2017-09-12T08:30:25.937192Z] 08:30:25     INFO - TEST-PASS | dom/media/test/test_can_play_type_wave.html | audio/wave; codecs=7 
[task 2017-09-12T08:30:25.939241Z] 08:30:25     INFO - Buffered messages finished
[task 2017-09-12T08:30:25.940954Z] 08:30:25     INFO - TEST-UNEXPECTED-FAIL | dom/media/test/test_can_play_type_wave.html | audio/wave; codecs= - got "maybe", expected "probably"
[task 2017-09-12T08:30:25.942510Z] 08:30:25     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:312:5
[task 2017-09-12T08:30:25.943881Z] 08:30:25     INFO -     check@dom/media/test/can_play_type_wave.js:3:5
[task 2017-09-12T08:30:25.945207Z] 08:30:25     INFO -     check_wave@dom/media/test/can_play_type_wave.js:17:3
[task 2017-09-12T08:30:25.946656Z] 08:30:25     INFO -     @dom/media/test/test_can_play_type_wave.html:24:1
[task 2017-09-12T08:30:25.947537Z] 08:30:25     INFO - Not taking screenshot here: see the one that was previously logged
[task 2017-09-12T08:30:25.948467Z] 08:30:25     INFO - TEST-UNEXPECTED-FAIL | dom/media/test/test_can_play_type_wave.html | audio/wave; codecs="" - got "maybe", expected "probably"
[task 2017-09-12T08:30:25.949345Z] 08:30:25     INFO -     SimpleTest.is@SimpleTest/SimpleTest.js:312:5
[task 2017-09-12T08:30:25.950095Z] 08:30:25     INFO -     check@dom/media/test/can_play_type_wave.js:3:5
[task 2017-09-12T08:30:25.950959Z] 08:30:25     INFO -     check_wave@dom/media/test/can_play_type_wave.js:18:3
[task 2017-09-12T08:30:25.951632Z] 08:30:25     INFO -     @dom/media/test/test_can_play_type_wave.html:24:1
Flags: needinfo?(jacheng)
I've run local mochitest and it passed.

Push try to ensure no side effect again.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7876a592dc3f234472bcd25422df529675a9da85
Flags: needinfo?(jacheng)
Comment on attachment 8906964 [details]
Bug 1398102 - [Part2] Fix test fail due to wrong assumption.

https://reviewboard.mozilla.org/r/178712/#review184054
Attachment #8906964 - Flags: review?(gsquelart) → review+
(In reply to Anne (:annevk) from comment #18)
> If I make that `type + ';codecs=,'` we still consider it empty I hope? Or
> with a space of sorts.

These will probably not be considered empty, as the IsEmpty() test is purely string-based.

However I don't think it's a problem in the larger context of canPlayType tests:
Decoders' IsSupportedType functions will be given these non-empty list of codecs, and will actually perform deeper checks with them, in which case most decoders will respond that they cannot play these specified (albeit empty) codecs!

Anyway, if that's a real-world problem (who would actually use such a thing?), I'd say it's less urgent and could be dealt with in another bug, possibly bug 1399023? I'll add a note there.
try looks good. re-push it again.
Pushed by jacheng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c9589184e49f
[Part1] canPlayType should return 'maybe' if the codec parameter is empty. r=gerald
https://hg.mozilla.org/integration/autoland/rev/3dbe64e08fd0
[Part2] Fix test fail due to wrong assumption. r=gerald
It's not a big problem as far as I know (until someone relies on the difference between browsers). Just wondering if our parser make sense. Found this while trying to find out how MIME types are parsed across various APIs.
https://hg.mozilla.org/mozilla-central/rev/c9589184e49f
https://hg.mozilla.org/mozilla-central/rev/3dbe64e08fd0
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.