Data sample fed potentially fed twice to the decoder

RESOLVED FIXED in Firefox 52

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: JamesCheng, Assigned: JamesCheng)

Tracking

({regression})

Trunk
mozilla52
regression
Points:
---

Firefox Tracking Flags

(firefox50 unaffected, firefox51 unaffected, firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
If we recreate decoder and init the decoder in [1], we should make sure the decoder have been initialized(promise resolved) then feed the sample to decoder input.

It may be a defect in current code implementation[2], could have an elegant way to deal with this.




[1]
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/media/platforms/wrappers/H264Converter.cpp#281

[2]
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/dom/media/platforms/wrappers/H264Converter.cpp#113
(Assignee)

Comment 1

2 years ago
I would like to work on this as a beginner of media playback rookie.
Assignee: nobody → jacheng
(Assignee)

Comment 2

2 years ago
By adding some logs,

I found that when we met below scenario,

[1] Queue the incoming sample and wait for decoder initialized
http://searchfox.org/mozilla-central/rev/008fdd93290c83325de6629a1ae48dc2afaed868/dom/media/platforms/wrappers/H264Converter.cpp#225

[2] Input sample which may be converted into Annex B format to an initializing not finished decoder.
http://searchfox.org/mozilla-central/rev/008fdd93290c83325de6629a1ae48dc2afaed868/dom/media/platforms/wrappers/H264Converter.cpp#113

[3] Input the same sample[1](but not converted to Annex B) twice to the initialized decoder.
http://searchfox.org/mozilla-central/rev/008fdd93290c83325de6629a1ae48dc2afaed868/dom/media/platforms/wrappers/H264Converter.cpp#248

Is the scenario dealing with the input sample correctly(input twice for the AnnexB and non-AnnexB sample is OK?)?

If it is OK, I think it makes me feel confused and hard to realize.

So I modified the code as the attached patch.

Hi jya, could you please guide me if I misunderstand anything or it is really a potential bug?

Thank you.
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8809271 - Flags: review?(jyavenard)

Comment 4

2 years ago
mozreview-review
Comment on attachment 8809271 [details]
Bug 1314863 - Correct data sample being fed twice to the decoder

https://reviewboard.mozilla.org/r/91866/#review91822

You now have two different places where you initialise the decoder and report and handle an error. that's duplicated code.

But I think you took a too complicated approach.

Let's look at the problem at hand:
- The decoder will be fed a sample before the initialisation has completed.

So what we want to do, is not feed the decoder until initalisation has completed right?

We already have a mechanism in place for a value returned by CreateDecoderAndInit to indicate we need to postpone any further action: NS_ERROR_NOT_INITIALIZED.
CheckForSPSChange returns the value returned by CreateDecoderAndInit.

As such, there's only two things to do:
1- Have CreateDecoderAndInit return NS_ERROR_NOT_INITIALIZED after calling mDecoder->Init().
2- Handle the returned value if its NS_ERROR_NOT_INITIALIZED like the earlier line:

    if (rv == NS_ERROR_NOT_INITIALIZED) {
      // Decoder is not yet initialized
      mCallback->InputExhausted();
      return;
    }


That way the sample, which has been queued to mMediaRawSamples will be handled when the initialisation complete.

problem solved :)
Attachment #8809271 - Flags: review?(jyavenard) → review-

Comment 5

2 years ago
mozreview-review-reply
Comment on attachment 8809271 [details]
Bug 1314863 - Correct data sample being fed twice to the decoder

https://reviewboard.mozilla.org/r/91866/#review91822

BTW, while there is a bug as you've noticed. In practice it doesn't matter as all the current MediaDataDecoder::Init() call are actually synchronous. So in effect, we are actually feeding the decoder only after it's been initialised. That the data is fed twice for annexB however is a problem and is a regression introduced by bug 1297311. That should be fixed
Blocks: 1297311
Keywords: regression
Actually, it is also not a problem, because the H264Converter is always destroyed whenever a new init segment has been seen by the MediaFormatReader.

Still a coding regression
Summary: Make sure feeding the sample to the decoder after the decoder have been initialized. → Data sample fed potentially fed twice to the decoder
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 years ago
mozreview-review
Comment on attachment 8809271 [details]
Bug 1314863 - Correct data sample being fed twice to the decoder

https://reviewboard.mozilla.org/r/91866/#review91840

Thanks for your guidance. 

I just found 
http://searchfox.org/mozilla-central/rev/008fdd93290c83325de6629a1ae48dc2afaed868/dom/media/platforms/agnostic/gmp/GMPVideoDecoder.cpp#302

Does this "Init" function a real async call and be used in current product?

::: dom/media/platforms/wrappers/H264Converter.cpp:257
(Diff revision 2)
>      }
> +    if (!mNeedAVCC &&
> +        !mp4_demuxer::AnnexB::ConvertSampleToAnnexB(sample, mNeedKeyframe)) {
> +      mCallback->Error(MediaResult(NS_ERROR_OUT_OF_MEMORY,
> +                                   RESULT_DETAIL("ConvertSampleToAnnexB")));
> +    }

Do you think we should do the conversion here?  Or it is not neccessary.

Comment 9

2 years ago
mozreview-review
Comment on attachment 8809271 [details]
Bug 1314863 - Correct data sample being fed twice to the decoder

https://reviewboard.mozilla.org/r/91866/#review91848

::: dom/media/platforms/wrappers/H264Converter.cpp:257
(Diff revision 2)
>      }
> +    if (!mNeedAVCC &&
> +        !mp4_demuxer::AnnexB::ConvertSampleToAnnexB(sample, mNeedKeyframe)) {
> +      mCallback->Error(MediaResult(NS_ERROR_OUT_OF_MEMORY,
> +                                   RESULT_DETAIL("ConvertSampleToAnnexB")));
> +    }

YES!

good call!

Don't forget to return early though. we have an OOM error
Attachment #8809271 - Flags: review?(jyavenard) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 11

2 years ago
mozreview-review
Comment on attachment 8809271 [details]
Bug 1314863 - Correct data sample being fed twice to the decoder

https://reviewboard.mozilla.org/r/91866/#review91866

::: dom/media/platforms/wrappers/H264Converter.cpp:236
(Diff revision 3)
>  
>      mInitPromiseRequest.Begin(mDecoder->Init()
>        ->Then(AbstractThread::GetCurrent()->AsTaskQueue(), __func__, this,
>               &H264Converter::OnDecoderInitDone,
>               &H264Converter::OnDecoderInitFailed));
>    }

the return should have been here !
otherwise error will always be skipped now

Comment 12

2 years ago
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d32e0a4ff68c
Data sample fed potentially fed twice to the decoder. r=jya
Keywords: checkin-needed

Comment 13

2 years ago
mozreview-review
Comment on attachment 8809271 [details]
Bug 1314863 - Correct data sample being fed twice to the decoder

https://reviewboard.mozilla.org/r/91866/#review91868

also the commit message needs to be fixed

"Correct data sample being fed twice to the decoder"
Comment hidden (mozreview-request)
(Assignee)

Comment 15

2 years ago
Comment on attachment 8809271 [details]
Bug 1314863 - Correct data sample being fed twice to the decoder

I press the "reopen for review" but nothing happened....

Need to use bugzilla to r? again.

Thanks.
Attachment #8809271 - Flags: review+ → review?(jyavenard)
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1316550

Comment 17

2 years ago
mozreview-review
Comment on attachment 8809271 [details]
Bug 1314863 - Correct data sample being fed twice to the decoder

https://reviewboard.mozilla.org/r/91866/#review91872

::: dom/media/platforms/wrappers/H264Converter.cpp:90
(Diff revision 4)
>        return;
>      }
>    } else {
>      rv = CheckForSPSChange(aSample);
> +    if (rv == NS_ERROR_NOT_INITIALIZED) {
> +      // Decoder has not yet initialized.

"Decoder has not yet been initialized"

though seeing that the return value is super explict, we may get rid of the comment or put something with more details, like

The decoder is pending initialization etc...
Attachment #8809271 - Flags: review?(jyavenard) → review+

Comment 18

2 years ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/324fdf1165e8
Backed out changeset d32e0a4ff68c on request from jya
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 20

2 years ago
mozreview-review
Comment on attachment 8809271 [details]
Bug 1314863 - Correct data sample being fed twice to the decoder

https://reviewboard.mozilla.org/r/91866/#review91888

::: dom/media/platforms/wrappers/H264Converter.cpp:258
(Diff revision 5)
>      }
> +    if (!mNeedAVCC &&
> +        !mp4_demuxer::AnnexB::ConvertSampleToAnnexB(sample, mNeedKeyframe)) {
> +      mCallback->Error(MediaResult(NS_ERROR_OUT_OF_MEMORY,
> +                                   RESULT_DETAIL("ConvertSampleToAnnexB")));
> +      return;

while it likely doesn't matter.

We'll probably want to clean the queued sample there for cleanliness.
Keywords: checkin-needed
(Assignee)

Comment 21

2 years ago
Addressed comment 20.
Keywords: checkin-needed
(In reply to James Cheng[:JamesCheng] from comment #21)
> Addressed comment 20.

where?
Keywords: checkin-needed
(Assignee)

Comment 23

2 years ago
Sorry , I don't know why my mozreview push is pushed but shows it is a draft in reviewboard and I've never met this before. And I accidently press the "discard " button by my cellphone... I will push the patch again when I'm back home.
Comment hidden (mozreview-request)
(Assignee)

Comment 25

2 years ago
ensuring I addressed comment 20.
Keywords: checkin-needed

Comment 26

2 years ago
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/361dfd9071b6
Correct data sample being fed twice to the decoder r=jya
Keywords: checkin-needed

Comment 27

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/361dfd9071b6
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
status-firefox50: --- → unaffected
status-firefox51: --- → unaffected
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.