Closed Bug 1341483 Opened 7 years ago Closed 7 years ago

Report Rust demuxer issues as errors (or warnings if appropriate)

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Version:
53 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mozbugz, Assigned: mozbugz)

References

Details

Attachments

(5 files, 1 obsolete file)

Bug 1341483 - MP4Metadata::Metadata() now also returns a success/error code -
59 bytes, text/x-review-board-request
kinetik
: review+
Details
Bug 1341483 - MP4Metadata::GetNumberTracks() now also returns a success/error code -
59 bytes, text/x-review-board-request
kinetik
: review+
Details
Bug 1341483 - MP4Metadata::GetTrackInfo() now also returns a success/error code -
59 bytes, text/x-review-board-request
kinetik
: review+
Details
Bug 1341483 - MP4Metadata::Crypto() now also returns a success/error code -
59 bytes, text/x-review-board-request
kinetik
: review+
Details
Bug 1341483 - MP4Metadata::GetTrackIndice() now also returns a success/error code -
59 bytes, text/x-review-board-request
kinetik
: review+
Details 
Some Rust demuxer issues may not be correctly reported, especially cases where the output is different from the C++ demuxer's output.

These should be reported so that they make the DecoderDoctor drop-down appear and users may report them as site issues.
This should help our developers learn about issues, more easily reproduce them thanks to the extra information, including the resource URL.
Please don't start reviewing yet, something landed in m-c that will require me to do an extensive rebase! Please wait for the next push.
Attachment #8847071 - Flags: review?(kinetik)
Attachment #8847072 - Flags: review?(kinetik)
Attachment #8847073 - Flags: review?(kinetik)
Attachment #8847074 - Flags: review?(kinetik)
Attachment #8847075 - Flags: review?(kinetik)
Attachment #8847075 - Attachment is obsolete: true
Comment on attachment 8847071 [details]
Bug 1341483 - MP4Metadata::Metadata() now also returns a success/error code -

https://reviewboard.mozilla.org/r/120108/#review123290
Attachment #8847071 - Flags: review?(kinetik) → review+
Comment on attachment 8847072 [details]
Bug 1341483 - MP4Metadata::GetNumberTracks() now also returns a success/error code -

https://reviewboard.mozilla.org/r/120110/#review123292
Attachment #8847072 - Flags: review?(kinetik) → review+
Comment on attachment 8847073 [details]
Bug 1341483 - MP4Metadata::GetTrackInfo() now also returns a success/error code -

https://reviewboard.mozilla.org/r/120112/#review123296

::: media/libstagefright/binding/MP4Metadata.cpp:419
(Diff revision 2)
> +    const VideoInfo *videoRust = infoRust.GetAsVideoInfo();
> +    const VideoInfo *video = info.GetAsVideoInfo();
> +    if (videoRust->mDisplay == video->mDisplay) { return "Display"; }
> +    if (videoRust->mImage == video->mImage) { return "Image"; }
> +    if (*videoRust->mExtraData == *video->mExtraData) { return "ExtraData"; }
> +    // mCodecSpecificConfig is for video/mp4-es, not video/avc. Since video/mp4-es

Is there a bug on file to do this?

We just had a bug in the Rust/MP4Metadata integration where we copied into mCodecSpecificConfig when we should've used mExtraData.
Attachment #8847073 - Flags: review?(kinetik) → review+
Comment on attachment 8847074 [details]
Bug 1341483 - MP4Metadata::Crypto() now also returns a success/error code -

https://reviewboard.mozilla.org/r/120114/#review123306

::: media/libstagefright/binding/include/mp4_demuxer/MP4Metadata.h:77
(Diff revision 2)
>    ResultAndTrackInfo GetTrackInfo(mozilla::TrackInfo::TrackType aType,
>                                    size_t aTrackNumber) const;
>  
>    bool CanSeek() const;
>  
> -  const CryptoFile& Crypto() const;
> +  using ResultAndCryptoFile = ResultAndType<const CryptoFile*>;

Is there a reason this has to be a pointer instead of a reference?
Attachment #8847074 - Flags: review?(kinetik) → review+
Comment on attachment 8848059 [details]
Bug 1341483 - MP4Metadata::GetTrackIndice() now also returns a success/error code -

https://reviewboard.mozilla.org/r/121030/#review123308
Attachment #8848059 - Flags: review?(kinetik) → review+
Comment on attachment 8847074 [details]
Bug 1341483 - MP4Metadata::Crypto() now also returns a success/error code -

https://reviewboard.mozilla.org/r/120114/#review123306

> Is there a reason this has to be a pointer instead of a reference?

So that we can return a nullptr as a definite error, separate from the MediaResult part, which is just an error/warning message.
Comment on attachment 8847073 [details]
Bug 1341483 - MP4Metadata::GetTrackInfo() now also returns a success/error code -

https://reviewboard.mozilla.org/r/120112/#review123296

> Is there a bug on file to do this?
> 
> We just had a bug in the Rust/MP4Metadata integration where we copied into mCodecSpecificConfig when we should've used mExtraData.

Following your comment, Alfredo opened bug 1348212.
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e229e30bf1a3
MP4Metadata::Metadata() now also returns a success/error code - r=kinetik
https://hg.mozilla.org/integration/autoland/rev/8e992d6e36e4
MP4Metadata::GetNumberTracks() now also returns a success/error code - r=kinetik
https://hg.mozilla.org/integration/autoland/rev/9106e5740bdd
MP4Metadata::GetTrackInfo() now also returns a success/error code - r=kinetik
https://hg.mozilla.org/integration/autoland/rev/361f7a093694
MP4Metadata::Crypto() now also returns a success/error code - r=kinetik
https://hg.mozilla.org/integration/autoland/rev/b39beeadbc92
MP4Metadata::GetTrackIndice() now also returns a success/error code - r=kinetik
Someone blog posted(http://blog.goo.ne.jp/atlanto/e/2cdc3c75132f7353c89e9eebc4f00a73):

the patch  attachment 8847073 [details] seems logically wrong.

https://dxr.mozilla.org/mozilla-central/source/media/libstagefright/binding/MP4Metadata.cpp#394

GetDifferentField(const mozilla::TrackInfo& info,
                  const mozilla::TrackInfo& infoRust)
{
  if (infoRust.mId != info.mId) { return "Id"; }
  if (infoRust.mKind == info.mKind) { return "Kind"; }
  ...


All most of "=="  seems typo, they should be "!=".
So, a lot of "Report Site Issue" notification pop up like a storm.

What do you think?
Flags: needinfo?(gsquelart)
Oh dear, that's right :-(
Thank you for notifying me.

I'll open a bug and fix it ASAP...
Flags: needinfo?(gsquelart)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: