Closed Bug 1413076 Opened 2 years ago Closed 2 years ago

Return other error in H264Converter

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: ayang, Assigned: ayang)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Suggested by Jya that it should return other error in addition to OOM.
Comment on attachment 8923661 [details]
Bug 1413076 - return other kind of error in addition to OOM.

https://reviewboard.mozilla.org/r/194780/#review199914

::: dom/media/platforms/wrappers/H264Converter.cpp:123
(Diff revision 1)
>  
>    if (mNeedKeyframe && !aSample->mKeyframe) {
>      return DecodePromise::CreateAndResolve(DecodedData(), __func__);
>    }
>  
> -  if (!*mNeedAVCC &&
> +  auto res = !*mNeedAVCC

and don't use auto here, it makes it very hard to know what argument type is returned

Could create a new typedef for that and re-use it everywhere.

Would be something like typedef Result<bool, MediaResult> MediaReturn; or something.

then you can use that type thorough the code to ensure future consistency

::: dom/media/platforms/wrappers/H264Converter.cpp:126
(Diff revision 1)
>    }
>  
> -  if (!*mNeedAVCC &&
> -      mp4_demuxer::AnnexB::ConvertSampleToAnnexB(aSample, mNeedKeyframe).isErr()) {
> +  auto res = !*mNeedAVCC
> +             ? mp4_demuxer::AnnexB::ConvertSampleToAnnexB(aSample, mNeedKeyframe)
> +             : Ok();
> +  if (res.isErr()) {

and don't use auto here, it makes it very hard to know what argument type is returned

::: dom/media/platforms/wrappers/H264Converter.cpp:126
(Diff revision 1)
>    }
>  
> -  if (!*mNeedAVCC &&
> -      mp4_demuxer::AnnexB::ConvertSampleToAnnexB(aSample, mNeedKeyframe).isErr()) {
> +  auto res = !*mNeedAVCC
> +             ? mp4_demuxer::AnnexB::ConvertSampleToAnnexB(aSample, mNeedKeyframe)
> +             : Ok();
> +  if (res.isErr()) {

Please have a follow up patch where ConvertSampleToAnnexB returns a Result<bool, MediaResult> and reject the DecodePromise with the error returned there.

::: dom/media/platforms/wrappers/H264Converter.cpp:378
(Diff revision 1)
>      mDecodePromise.Resolve(mPendingFrames, __func__);
>      mPendingFrames.Clear();
>      return;
>    }
>  
> -  if (!*mNeedAVCC &&
> +  auto res = !*mNeedAVCC

same, no auto
Attachment #8923661 - Flags: review?(jyavenard) → review+
(In reply to Jean-Yves Avenard [:jya] from comment #2)
> Comment on attachment 8923661 [details]
> Bug 1413076 - return other kind of error in addition to OOM.
> 
> https://reviewboard.mozilla.org/r/194780/#review199914
> 
> ::: dom/media/platforms/wrappers/H264Converter.cpp:123
> (Diff revision 1)
> >  
> >    if (mNeedKeyframe && !aSample->mKeyframe) {
> >      return DecodePromise::CreateAndResolve(DecodedData(), __func__);
> >    }
> >  
> > -  if (!*mNeedAVCC &&
> > +  auto res = !*mNeedAVCC
> 
> and don't use auto here, it makes it very hard to know what argument type is
> returned
> 
> Could create a new typedef for that and re-use it everywhere.
> 
> Would be something like typedef Result<bool, MediaResult> MediaReturn; or
> something.
> 
> then you can use that type thorough the code to ensure future consistency

There are other places have the same patterns. I'll fix it in another bug.

> 
> ::: dom/media/platforms/wrappers/H264Converter.cpp:126
> (Diff revision 1)
> >    }
> >  
> > -  if (!*mNeedAVCC &&
> > -      mp4_demuxer::AnnexB::ConvertSampleToAnnexB(aSample, mNeedKeyframe).isErr()) {
> > +  auto res = !*mNeedAVCC
> > +             ? mp4_demuxer::AnnexB::ConvertSampleToAnnexB(aSample, mNeedKeyframe)
> > +             : Ok();
> > +  if (res.isErr()) {
> 
> Please have a follow up patch where ConvertSampleToAnnexB returns a
> Result<bool, MediaResult> and reject the DecodePromise with the error
> returned there.

I'll fix it in another bug.
Pushed by ayang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/75a973619e42
return other kind of error in addition to OOM. r=jya
https://hg.mozilla.org/mozilla-central/rev/75a973619e42
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.