If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Remove MediaDataDecoder::ConfigurationChanged API

RESOLVED FIXED in Firefox 54

Status

()

Core
Audio/Video: Playback
P1
normal
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: jya, Assigned: jolin)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox54 fixed, firefox55 fixed)

Details

MozReview Requests

()

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

Attachments

(5 attachments, 4 obsolete attachments)

59 bytes, text/x-review-board-request
jya
: review+
Details | Review
59 bytes, text/x-review-board-request
jya
: review+
Details | Review
59 bytes, text/x-review-board-request
jya
: review+
Details | Review
59 bytes, text/x-review-board-request
jya
: review+
Details | Review
59 bytes, text/x-review-board-request
jya
: review+
Details | Review
(Reporter)

Description

8 months ago
As discussed with :jolin

Rather than having an extra API that calls a particular entry (ConfigurationChanged) whenever a change of resolution is detected.

We should instead simply add to the MediaRawData the related VideoInfo/AudioInfo in a TrackInfoSharedPtr (or SharedTrackInfo in 53 and earlier).

This will make the change easier to adapt in order to uplift bug 1336431
(Assignee)

Updated

8 months ago
Assignee: nobody → jolin
Blocks: 1343847
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 3

8 months ago
mozreview-review
Comment on attachment 8843901 [details]
Bug 1344649 - part 1: let VideoData::CreateFromImage() accept only neccessary parameters.

https://reviewboard.mozilla.org/r/117520/#review119146

you can't commit this change as-is.. it will fail the mochitests.
Attachment #8843901 - Flags: review?(jyavenard) → review+
(Reporter)

Comment 4

8 months ago
mozreview-review
Comment on attachment 8843902 [details]
Bug 1344649 - part 2: rename DurationMap and turn it into a generic class.

https://reviewboard.mozilla.org/r/117522/#review119148

I don't think this should be fully reverted.

all we need instead is change the MFR to stop calling ConfigurationChanged() and the H264Converter to simply create a new SharedTrackInfo whenever a change of resolution is detected.

I'm expecting to see most if this back in a follow up patch.

The commit title is also incomplete as there's more revert that Bug 1336431 P5... there's bug 1342217 and some of bug 1339748

::: dom/media/MediaFormatReader.cpp:1963
(Diff revision 1)
>    while (decoder.mQueuedSamples.Length()) {
>      RefPtr<MediaRawData> sample = decoder.mQueuedSamples[0];
>      RefPtr<TrackInfoSharedPtr> info = sample->mTrackInfo;
>  
>      if (info && decoder.mLastStreamSourceID != info->GetID()) {
> -      if (decoder.mNextStreamSourceID.isNothing()
> +      bool supportRecycling = MediaPrefs::MediaDecoderCheckRecycling()

I think this should stay as-is, it doesn't need to be reverted

::: dom/media/MediaFormatReader.cpp
(Diff revision 1)
>          nsTArray<RefPtr<MediaRawData>> samples{ Move(decoder.mQueuedSamples) };
>          ShutdownDecoder(aTrack);
>          if (sample->mKeyframe) {
>            decoder.mQueuedSamples.AppendElements(Move(samples));
>          }
> -      } else if (decoder.mInfo && *decoder.mInfo != *info) {

only that part needs to be removed

::: dom/media/platforms/wrappers/H264Converter.cpp:291
(Diff revision 1)
>        MediaResult(NS_ERROR_OUT_OF_MEMORY,
>                    RESULT_DETAIL("ConvertSampleToAnnexB")),
>        __func__);
>      return;
>    }
> -  if (CanRecycleDecoder()) {
> +

that should be replaced by creating a new TrackInfoSharedPtr that would be added to the MediaRawData.

::: dom/media/platforms/wrappers/H264Converter.cpp
(Diff revision 1)
> -
>    if (CanRecycleDecoder()) {
>      // Do not recreate the decoder, reuse it.
>      UpdateConfigFromExtraData(extra_data);
> -    // Ideally we would want to drain the decoder instead of flushing it.
> -    // However the draining operation requires calling Drain and looping several

this should stay too...
Attachment #8843902 - Flags: review?(jyavenard) → review+
Priority: -- → P1
(Assignee)

Comment 5

7 months ago
mozreview-review-reply
Comment on attachment 8843902 [details]
Bug 1344649 - part 2: rename DurationMap and turn it into a generic class.

https://reviewboard.mozilla.org/r/117522/#review119148

> I think this should stay as-is, it doesn't need to be reverted

Okay. Will bring in back in next version.

> that should be replaced by creating a new TrackInfoSharedPtr that would be added to the MediaRawData.

Yes, I was going to do this in another bug/patch, but you're right that it makes more sense being here.

> this should stay too...

Sorry but I don't quite understand why draining/flushing is still needed here after we attach updated video info copy in `mPendingSample`. I thought that is for dropping outputs before configuration change so they won't incorrectly use the new dimensions. Could you please explain it? Thanks.
(Assignee)

Updated

7 months ago
Flags: needinfo?(jyavenard)
(Reporter)

Comment 6

7 months ago
mozreview-review-reply
Comment on attachment 8843902 [details]
Bug 1344649 - part 2: rename DurationMap and turn it into a generic class.

https://reviewboard.mozilla.org/r/117522/#review119148

> Sorry but I don't quite understand why draining/flushing is still needed here after we attach updated video info copy in `mPendingSample`. I thought that is for dropping outputs before configuration change so they won't incorrectly use the new dimensions. Could you please explain it? Thanks.

Because I see two parts of this bug.

The reversal of ConfigurationChange is one thing, and that's to be replaced by instead setting the TrackInfoSharedPtr to the current configuration.

And then you have what's causing the problem of bug 1343847: that's the drain/flush. For this part, we need to figure out why the draining cause the problem (it shouldn't really) and either fix that, or get around the problem and not drain.

Separating the two tasks is better, and it doesn't make incorrectly blame the problem to using the ConfigurationChanged API, they are two separate issues.

It sounds much nicer to me to do it that way.
(Reporter)

Updated

7 months ago
Flags: needinfo?(jyavenard)
(Assignee)

Comment 7

7 months ago
Good point. Thanks!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 12

7 months ago
I did a complete rewrite to address comments. Hope this version make more sense now.
(Assignee)

Updated

7 months ago
Attachment #8843901 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Attachment #8843902 - Attachment is obsolete: true
(Assignee)

Updated

7 months ago
Attachment #8844756 - Attachment is obsolete: true
Attachment #8844756 - Flags: review?(jyavenard)
(Assignee)

Updated

7 months ago
Attachment #8844757 - Attachment is obsolete: true
Attachment #8844757 - Flags: review?(jyavenard)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 17

7 months ago
mozreview-review
Comment on attachment 8844759 [details]
Bug 1344649 - part 2: let VideoData::CreateFromImage() accept only neccessary parameters.

https://reviewboard.mozilla.org/r/118074/#review120104

Personally, I think it would be easier to continue passing a VideoInfo argument, it's less changes and makes it easier to understand (I think)

::: commit-message-2cce2:3
(Diff revision 1)
> +Bug 1344649 - part 1: let VideoData::CreateFromImage() accept only neccessary parameters. r?jya
> +
> +VideoData doesn't care what's in aInfo but display size, and aPicture is unused.

display size and aPicture are unused

::: dom/media/platforms/android/RemoteDataDecoder.cpp:185
(Diff revision 1)
>  
>        if (size > 0) {
>          MutexAutoLock lock(mDecoder->mMutex);
>  
>          RefPtr<layers::Image> img = new SurfaceTextureImage(
> -          mDecoder->mSurfaceTexture.get(), mDecoder->mConfig.mDisplay,
> +          mDecoder->mSurfaceTexture.get(), mDecoder->mConfig.mImage,

that can't be good.

This should be done in another patch as it's out of scope with the rest of the changes.

::: dom/media/platforms/apple/AppleVTDecoder.cpp:355
(Diff revision 1)
>    // Where our resulting image will end up.
>    RefPtr<MediaData> data;
>    // Bounds.
>    VideoInfo info;
>    info.mDisplay = nsIntSize(mDisplayWidth, mDisplayHeight);
>    gfx::IntRect visible = gfx::IntRect(0,

visible scope should be moved from inside if (mUseSoftwareImages) { as it's no longer used otherwise
Attachment #8844759 - Flags: review?(jyavenard) → review+
(Reporter)

Comment 18

7 months ago
mozreview-review
Comment on attachment 8844760 [details]
Bug 1344649 - part 3: rename DurationMap and turn it into a generic class.

https://reviewboard.mozilla.org/r/118076/#review120110

not fan of the name but hey .... SimpleMap?
Attachment #8844760 - Flags: review?(jyavenard) → review+
(Reporter)

Comment 19

7 months ago
mozreview-review
Comment on attachment 8844761 [details]
Bug 1344649 - part 4: store frame sizes in queue rather than relying on ConfigurationChanged().

https://reviewboard.mozilla.org/r/118078/#review120116

::: dom/media/platforms/android/RemoteDataDecoder.cpp:140
(Diff revision 1)
>      java::CodecProxy::GlobalRef mCodec;
>      java::Sample::GlobalRef mSample;
>    };
>  
> +
> +  class InputInfo {

bracket on next line:
class InputInfo
{

::: dom/media/platforms/android/RemoteDataDecoder.cpp:142
(Diff revision 1)
>    };
>  
> +
> +  class InputInfo {
> +  public:
> +    InputInfo()

InputInfo() { } 

(note the space between the brackets)
Attachment #8844761 - Flags: review?(jyavenard) → review+
(Reporter)

Comment 20

7 months ago
mozreview-review
Comment on attachment 8844762 [details]
Bug 1344649 - part 5: deprecate ConfigurationChanged() once again.

https://reviewboard.mozilla.org/r/118080/#review120132

::: dom/media/MediaFormatReader.cpp
(Diff revision 1)
>          nsTArray<RefPtr<MediaRawData>> samples{ Move(decoder.mQueuedSamples) };
>          ShutdownDecoder(aTrack);
>          if (sample->mKeyframe) {
>            decoder.mQueuedSamples.AppendElements(Move(samples));
>          }
> -      } else if (decoder.mInfo && *decoder.mInfo != *info) {

You need to set the MediaRawData with the info...
Attachment #8844762 - Flags: review?(jyavenard) → review-
(Reporter)

Comment 21

7 months ago
mozreview-review
Comment on attachment 8844761 [details]
Bug 1344649 - part 4: store frame sizes in queue rather than relying on ConfigurationChanged().

https://reviewboard.mozilla.org/r/118078/#review120134

::: dom/media/platforms/wrappers/H264Converter.cpp:326
(Diff revision 1)
>    RefPtr<MediaRawData> sample = aSample;
>  
>    if (CanRecycleDecoder()) {
>      // Do not recreate the decoder, reuse it.
>      UpdateConfigFromExtraData(extra_data);
> +    sample->mTrackInfo = new TrackInfoSharedPtr(mCurrentConfig, 0);

This should be done in part 4...

Additionally, you should check if a sample->mTrackInfo is already set or not, as if it is, it means you're using MSE and the MediaRawData already contains the current value.
(Reporter)

Comment 22

7 months ago
mozreview-review
Comment on attachment 8844762 [details]
Bug 1344649 - part 5: deprecate ConfigurationChanged() once again.

https://reviewboard.mozilla.org/r/118080/#review120166
Attachment #8844762 - Flags: review- → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 28

7 months ago
mozreview-review
Comment on attachment 8845261 [details]
Bug 1344649 - part 1: use picture instead of display size to construct Image.

https://reviewboard.mozilla.org/r/118432/#review120374
Attachment #8845261 - Flags: review?(jyavenard) → review+
(Reporter)

Comment 29

7 months ago
mozreview-review
Comment on attachment 8844762 [details]
Bug 1344649 - part 5: deprecate ConfigurationChanged() once again.

https://reviewboard.mozilla.org/r/118080/#review120376

::: dom/media/platforms/wrappers/H264Converter.h
(Diff revision 2)
>    }
>  
>    void DecodeFirstSample(MediaRawData* aSample);
>  
>    RefPtr<PlatformDecoderModule> mPDM;
> -  const VideoInfo mOriginalConfig;

I think all related to mOriginalConfig should stay, the logic was wrong before..

::: dom/media/platforms/wrappers/H264Converter.cpp:206
(Diff revision 2)
>      mLastError = NS_ERROR_FAILURE;
>      return NS_ERROR_FAILURE;
>    }
>  
>    mDecoder = mPDM->CreateVideoDecoder({
> -    mUseOriginalConfig ? mOriginalConfig : mCurrentConfig,
> +    mCurrentConfig,

dont revert that code, otherwise the VideoInfo used to create the decoder will be chsnged in a non threadsafe fashion
(Reporter)

Comment 30

7 months ago
mozreview-review-reply
Comment on attachment 8844762 [details]
Bug 1344649 - part 5: deprecate ConfigurationChanged() once again.

https://reviewboard.mozilla.org/r/118080/#review120132

> You need to set the MediaRawData with the info...

doh...
sorry for that.
(Reporter)

Updated

7 months ago
See Also: → bug 1345768
(Reporter)

Updated

7 months ago
No longer blocks: 1343847
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 37

7 months ago
Pushed by jolin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2bf3ae75d54
part 1: use picture instead of display size to construct Image. r=jya
https://hg.mozilla.org/integration/autoland/rev/5eb2d62d3c61
part 2: let VideoData::CreateFromImage() accept only neccessary parameters. r=jya
https://hg.mozilla.org/integration/autoland/rev/71954a8ab450
part 3: rename DurationMap and turn it into a generic class. r=jya
https://hg.mozilla.org/integration/autoland/rev/ba81bdbba7e4
part 4: store frame sizes in queue rather than relying on ConfigurationChanged(). r=jya
https://hg.mozilla.org/integration/autoland/rev/fcd6c84fa5b0
part 5: deprecate ConfigurationChanged() once again. r=jya

Comment 38

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b2bf3ae75d54
https://hg.mozilla.org/mozilla-central/rev/5eb2d62d3c61
https://hg.mozilla.org/mozilla-central/rev/71954a8ab450
https://hg.mozilla.org/mozilla-central/rev/ba81bdbba7e4
https://hg.mozilla.org/mozilla-central/rev/fcd6c84fa5b0
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 39

7 months ago
Comment on attachment 8845261 [details]
Bug 1344649 - part 1: use picture instead of display size to construct Image.

Please approve uplifting all patches to aurora. Thanks a lot. 

Approval Request Comment
[Feature/Bug causing the regression]: bug 1257777
[User impact if declined]: wrong video size/aspect ratio
[Is this code covered by automated tests?]:no
[Has the fix been verified in Nightly?]:yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]:low risk
[Why is the change risky/not risky?]: it changes only video frames size.
[String changes made/needed]:none
Attachment #8845261 - Flags: approval-mozilla-aurora?
Fails to merge:
    Merge failed for commit fcd6c84fa5b0 (parent fd8bdabb4813)

    grafting 386116:fcd6c84fa5b0 "Bug 1344649 - part 5: deprecate ConfigurationChanged() once again. r=jya"
    merging dom/media/MediaFormatReader.cpp
    merging dom/media/platforms/PlatformDecoderModule.h
    merging dom/media/platforms/wrappers/H264Converter.cpp
    merging dom/media/platforms/wrappers/H264Converter.h
    warning: conflicts while merging dom/media/platforms/wrappers/H264Converter.cpp! (edit, then use 'hg resolve --mark')
    abort: unresolved conflicts, can't continue
    (use 'hg resolve' and 'hg graft --continue')

    Merge failed for commit ba81bdbba7e4 (parent fd8bdabb4813)

    grafting 386115:ba81bdbba7e4 "Bug 1344649 - part 4: store frame sizes in queue rather than relying on ConfigurationChanged(). r=jya"
    merging dom/media/platforms/android/RemoteDataDecoder.cpp
    warning: conflicts while merging dom/media/platforms/android/RemoteDataDecoder.cpp! (edit, then use 'hg resolve --mark')
    abort: unresolved conflicts, can't continue
    (use 'hg resolve' and 'hg graft --continue')
Flags: needinfo?(jolin)
(Assignee)

Comment 41

7 months ago
I'm terribly sorry. These patches are dependent on bug 1344661 and bug 1345545 and I forgot to list them as other uplifts needed. Please help uplift them too. Thanks a lot and sorry again.
Flags: needinfo?(jolin)
Hi John,
Are there any ways that QA can help verify this? These patches are huge to me and your risk adjustment doesn't seem correct to me. I will hold this until we have better verification steps for QA to test.
Flags: needinfo?(jolin)
(Assignee)

Comment 43

7 months ago
Hi Gary,
 These patches are actually not as huge as it looks like. This bug rewrites bug 1336431 to make it easier to uplift to beta(53) and a large part of the changeset is reverting that. There are only 3 real logic changes which sets (correct) video frame sizes.
 
 As for QA test, I'm afraid that I haven't met the symptom in the field in person and don't have a SOP to reproduce it. But the old logic, as Jean-Yves said in bug 1336431, is just plain wrong.
 
 Also, the work of bug 1345599 was developed on top of patches here so it will be more difficult to uplift that without this.

 Please let me know if you have more concerns. Thanks a lot.
Flags: needinfo?(jolin) → needinfo?(gchang)
(Assignee)

Comment 44

7 months ago
Oops! Just found that I misspell your name. Terribly sorry about that, Gerry.
Comment on attachment 8845261 [details]
Bug 1344649 - part 1: use picture instead of display size to construct Image.

This is much more clear to me. Thanks for the explanation. Aurora54+.
Flags: needinfo?(gchang)
Attachment #8845261 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox54: --- → affected

Comment 46

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/da006962bafd
https://hg.mozilla.org/releases/mozilla-aurora/rev/2ce9976ffff3
https://hg.mozilla.org/releases/mozilla-aurora/rev/28d1b6af43b7
https://hg.mozilla.org/releases/mozilla-aurora/rev/3facbb51731c
https://hg.mozilla.org/releases/mozilla-aurora/rev/534c39cf5079
status-firefox54: affected → fixed
You need to log in before you can comment on or make changes to this bug.