1313556 - Implement Crypto() for mp4 rust parser

Status: RESOLVED FIXED

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

Comment 1

[Alfredo Yang (:alfredo)]

Attachment: Bug 1313556 - Implement Crypto() for mp4 rust parser.

Review commit: https://reviewboard.mozilla.org/r/91362/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91362/

Comment 2

[Alfredo Yang (:alfredo)]

https://github.com/mozilla/mp4parse-rust/pull/43

Comment 3

[Matthew Gregan [:kinetik]]

Comment on attachment 8808546 [details]
Bug 1313556 - Implement Crypto() for mp4 rust parser.

https://reviewboard.mozilla.org/r/91362/#review91450

::: media/libstagefright/binding/MP4Metadata.cpp:348
(Diff revision 1)
> +  if (mRustTestMode) {
> +    MOZ_DIAGNOSTIC_ASSERT(rustCrypto.pssh == crypto.pssh);
> +  }
> +#endif
> +
> +  if (mPreferRust && rustCrypto.pssh == crypto.pssh) {

The Rust bits need to be protected by the MOZ_RUST_MP4PARSE define.

I wonder if we should always use rustCrypto if mPreferRust; i.e. remove the equality test here?  We'll catch mismatches via the assert above.

::: media/libstagefright/binding/MP4Metadata.cpp:679
(Diff revision 1)
> +    return;
> +  }
> +
> +  nsTArray<uint8_t> pssh;
> +  pssh.AppendElements(info.data.data, info.data.length, fallible);
> +  mCrypto.Update(pssh.Elements(), pssh.Length());

Can't we pass info.data.data and info.data.length into mCrypto.Update directly, avoiding the copy into the local pssh array?

::: media/libstagefright/binding/include/mp4_demuxer/DecoderData.h:46
(Diff revision 1)
> +
> +  bool operator==(const PsshInfo& aOther) const {
> +    if (uuid != aOther.uuid || data != aOther.data) {
> +      return false;
> +    }
> +    return true;

This could just be

    return uuid == aOther.uuid && data == aOther.data;

Comment 4

[Matthew Gregan [:kinetik]]

Comment on attachment 8808546 [details]
Bug 1313556 - Implement Crypto() for mp4 rust parser.

https://reviewboard.mozilla.org/r/91362/#review91450

r+ with a few minor changes required

Comment 5

[Alfredo Yang (:alfredo)]

(In reply to Matthew Gregan [:kinetik] from comment #4)
> Comment on attachment 8808546 [details]
> Bug 1313556 - Implement Crypto() for mp4 rust parser.
> 
> https://reviewboard.mozilla.org/r/91362/#review91450
> 
> r+ with a few minor changes required

Thanks for review!

Comment 6

[Alfredo Yang (:alfredo)]

Comment on attachment 8808546 [details]
Bug 1313556 - Implement Crypto() for mp4 rust parser.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/91362/diff/1-2/

Comment 7

[Alfredo Yang (:alfredo)]

Attachment: Bug 1313556 - update rust parser for pssh parsing.

Review commit: https://reviewboard.mozilla.org/r/91868/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/91868/

Comment 8

[Ralph Giles (:rillian)]

Comment on attachment 8809286 [details]
Bug 1313556 - update rust parser for pssh parsing.

https://reviewboard.mozilla.org/r/91868/#review92252

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ee624382460
Implement Crypto() for mp4 rust parser. r=kinetik
https://hg.mozilla.org/integration/autoland/rev/88e9a1a777f4
update rust parser for pssh parsing. r=rillian

Comment 10

[Alfredo Yang (:alfredo)]

We also need video extra_data and codec_specific_config to play video. I'll fix it in another bug.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3ee624382460
https://hg.mozilla.org/mozilla-central/rev/88e9a1a777f4