Closed Bug 1370175 Opened 7 years ago Closed 7 years ago

Failed to play video in twitter.com

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: ayang, Assigned: ayang)

References

Details

(Keywords: compat)

Attachments

(1 file, 1 obsolete file)

Bug 1370175 - enable rust mp4 parser on other platforms and fallback to stagefright if rust parser fails.
59 bytes, text/x-review-board-request
kinetik
: review+
Details 
twitter.com - video doesn't play #7233

https://github.com/webcompat/web-bugs/issues/7233
Assignee: nobody → ayang
++DOMWINDOW == 3 (0x1134d0000) [pid = 15966] [serial = 4] [outer = 0x1135e0000]
W/MPEG4Extractor(15966): Error -1007 parsing chunck at offset 641 looking for first track
FileTypeBox { major_brand: mp42, minor_version: 0, compatible_brands: [mp42, mp41, iso4] }

Parsing fails at stagefright but it's ok at rust parser. So I won't dig into it because rust parser will enable on other patforms soon.
See Also: → 1341221
Sweet! Maybe we should set it depend on bug 1341221.
Priority: -- → P1
The issue is due to the ctts (Composition Time to Sample Box) being of version 1.

stagefright only handle ctts with a version of 0.

version 0 and version 1 are very similar except that version 0 is using unsigned int for the sample offset  and version 1 is using signed int.

I see in the mp4parse-rust code the exact same code and comments as in the mp4box.js project, it treats all integer as signed entry, even if version 0... 

I could add proper support to version 1 in stagefright, but maybe we should just enable mp4parse. Surely, it should be ready by now...

So when will the switch happen?
Flags: needinfo?(ajones)
If stagefright fails then we should be falling back to mp4parse-rust. We should only fix stagefright if it isn't reporting a failure correctly. This would mean we neither need to wait, nor need to fix stagefright.
Flags: needinfo?(ajones)
well, that fallback obviously doesn't work then.. because as it is, https://video.twimg.com/amplify_video/869584511708708864/vid/720x720/ep8MAWBWralrpj6F.mp4 doesn't play here (tested on ubuntu 17.04 with firefox nightly)
(In reply to Jean-Yves Avenard [:jya] from comment #5)
> well, that fallback obviously doesn't work then.. because as it is,
> https://video.twimg.com/amplify_video/869584511708708864/vid/720x720/
> ep8MAWBWralrpj6F.mp4 doesn't play here (tested on ubuntu 17.04 with firefox
> nightly)

hmmm... it's ok on my ubuntu 14.04 with nightly 55.0a1 (2017-06-06). Do you turn off "media.rust.mp4parser" in preference?
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #4)
> If stagefright fails then we should be falling back to mp4parse-rust. We
> should only fix stagefright if it isn't reporting a failure correctly. This
> would mean we neither need to wait, nor need to fix stagefright.

Will do.
(In reply to Alfredo Yang (:alfredo) from comment #6)
> (In reply to Jean-Yves Avenard [:jya] from comment #5)
> > well, that fallback obviously doesn't work then.. because as it is,
> > https://video.twimg.com/amplify_video/869584511708708864/vid/720x720/
> > ep8MAWBWralrpj6F.mp4 doesn't play here (tested on ubuntu 17.04 with firefox
> > nightly)
> 
> hmmm... it's ok on my ubuntu 14.04 with nightly 55.0a1 (2017-06-06). Do you
> turn off "media.rust.mp4parser" in preference?

i did !

my bad...
(In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #4)
> If stagefright fails then we should be falling back to mp4parse-rust. We
> should only fix stagefright if it isn't reporting a failure correctly. This
> would mean we neither need to wait, nor need to fix stagefright.

I believe we should do the opposite.

Make mp4parse-rust the default, and if it fails attempt to use stagefright.
(In reply to Jean-Yves Avenard [:jya] from comment #9)
> (In reply to Anthony Jones (:kentuckyfriedtakahe, :k17e) from comment #4)
> > If stagefright fails then we should be falling back to mp4parse-rust. We
> > should only fix stagefright if it isn't reporting a failure correctly. This
> > would mean we neither need to wait, nor need to fix stagefright.
> 
> I believe we should do the opposite.
> 
> Make mp4parse-rust the default, and if it fails attempt to use stagefright.
Agree with jya. 
Since we have not seen any problems on Linux (rust demuxer is enabled on Linux first) and the above mechanism should make demuxer more robust, IMO, we should consider enabling it on other platforms on 55 to get more feedbacks. If we find any problems on 55, we could consider disabling it accordingly. 

Alfredo,
What do you think?
Flags: needinfo?(ayang)
Attachment #8877425 - Attachment is obsolete: true
Attachment #8877425 - Flags: review?(kinetik)
Comment on attachment 8877490 [details]
Bug 1370175 - enable rust mp4 parser on other platforms and fallback to stagefright if rust parser fails.

https://reviewboard.mozilla.org/r/148936/#review153698

r- since I'm not sure about the telemetry bit.

::: dom/media/MediaPrefs.h:189
(Diff revision 1)
>    DECL_MEDIA_PREF("media.flac.enabled",                       FlacEnabled, bool, true);
>  
>    // Hls
>    DECL_MEDIA_PREF("media.hls.enabled",                        HLSEnabled, bool, false);
>  
> -#if !defined(RELEASE_OR_BETA)
> +  // Both rust/stagefright will be enable when this is true regardless of 'media.rust.mp4parser'.

Typo: enabled

::: media/libstagefright/binding/MP4Metadata.cpp:284
(Diff revision 1)
>                                   numTracks.Ref(),
>                                   numTracksRust.Result().Description().get(),
>                                   numTracksRust.Ref()));
>  
> +
> +  if (mRustTestMode) {

Rust test mode defaults to off, so this is effectively disabling telemetry for the Rust parser, right?  Is that really what we want?

::: media/libstagefright/binding/MP4Metadata.cpp:826
(Diff revision 1)
>    }
>  
> +  aParserDisable = !MediaPrefs::EnableRustMP4Parser() && !MediaPrefs::RustTestMode();
> +  if (aParserDisable) {
> +    return;
> +  }

It seems like it'd be better to handle this in the caller, passing a bool& in to mutate and changing the initialization of this seems harder to understand when casually reading the code.

It'd also be better if we could avoid calling mp4parse_new and enabling mp4parse logging if we're disabling the parser since it's just wasted resources.

::: media/libstagefright/binding/MP4Metadata.cpp:834
(Diff revision 1)
>    MOZ_LOG(sLog, LogLevel::Debug, ("rust parser returned %d\n", rv));
>    Telemetry::Accumulate(Telemetry::MEDIA_RUST_MP4PARSE_SUCCESS,
>                          rv == mp4parse_status_OK);
>    if (rv != mp4parse_status_OK) {
> +    MOZ_LOG(sLog, LogLevel::Info, ("Rust mp4 parser fails to parse this stream."));
> +    aParserDisable = true;

wrt to the earlier comment, I guess it's harder to handle this case since you'd need to split initialization into the ctor and a separate Init() that can fail...
Attachment #8877490 - Flags: review?(kinetik) → review-
Comment on attachment 8877490 [details]
Bug 1370175 - enable rust mp4 parser on other platforms and fallback to stagefright if rust parser fails.

https://reviewboard.mozilla.org/r/148936/#review153698

> Rust test mode defaults to off, so this is effectively disabling telemetry for the Rust parser, right?  Is that really what we want?

I'll fix that.
Another thought is: do we need test mode? It seems overlay with MediaPrefs::MediaWarningsAsErrorsStageFrightVsRust() to me. Maybe it make codes clear to keep MediaPrefs::MediaWarningsAsErrorsStageFrightVsRust() only?
(In reply to Alfredo Yang (:alfredo) from comment #16)
> Another thought is: do we need test mode? It seems overlay with
> MediaPrefs::MediaWarningsAsErrorsStageFrightVsRust() to me. Maybe it make
> codes clear to keep MediaPrefs::MediaWarningsAsErrorsStageFrightVsRust()
> only?

Good point; it's obsolete now we have the proper error reporting stuff so we can just rip it out and simplify the code a bit.
Comment on attachment 8877490 [details]
Bug 1370175 - enable rust mp4 parser on other platforms and fallback to stagefright if rust parser fails.

https://reviewboard.mozilla.org/r/148936/#review153698

> wrt to the earlier comment, I guess it's harder to handle this case since you'd need to split initialization into the ctor and a separate Init() that can fail...

hmm... I don't have idea except for adding a Init(). Any suggestion?
Comment on attachment 8877490 [details]
Bug 1370175 - enable rust mp4 parser on other platforms and fallback to stagefright if rust parser fails.

https://reviewboard.mozilla.org/r/148936/#review154352
Attachment #8877490 - Flags: review?(kinetik) → review+
Can we completely remove the mRustTestMode bool and associated pref now?
(In reply to Matthew Gregan [:kinetik] from comment #21)
> Can we completely remove the mRustTestMode bool and associated pref now?

Ya, will do. Thanks for review.
Pushed by ayang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2664a53d4c18
enable rust mp4 parser on other platforms and fallback to stagefright if rust parser fails. r=kinetik
https://hg.mozilla.org/mozilla-central/rev/2664a53d4c18
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Blocks: 1374562
See Also: → 1650718
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: