1248154 - HTMLVideoElement.videoWidth and videoHeight don't pick up on changes.

Status: RESOLVED FIXED

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

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Using the patches and fiddle in Bug 1244913 comment 5, I observe that without a 1 second delay, I get the wrong results from videoWidth and videoHeight.

I.e. I have to do this:

  var dimensions = v => v.videoWidth +"x"+ v.videoHeight;

  var set = (v, vinfo, stream) => Promise.resolve(v.srcObject = stream)
    .then(() => new Promise(r => v.onloadedmetadata = r))
    .then(() => wait(1000)) // Bug!!
    .then(() => setInterval(() => update(vinfo, dimensions(v), 1000)))
    .catch(log);

The reason the video changes has to do with delays in the WebRTC stack wrt scaling, which are perhaps unfortunate, but the fact that these values don't reflect changes correctly aren't helping, and should probably be fixed regardless of that.

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Note that I'm using setInterval, which means that repeated prodding keeps producing the wrong value after the change. I.e. the problem does not self-correct, which is unfortunate for diagnostics.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

jya - any ideas?  I'm pretty sure this used to work

Comment 3

[Jean-Yves Avenard [:jya]]

Could it be bug 1237809? that's the only changes I can think off that changed the handling of dimensions.
But it should be the opposite of what is seeing and was rather specific to plain media playback.

I don't know how webrtc handles changes of dimensions, the whole thing in HTMLMediaElement when it comes to media stream has always looked highly suspicious to me.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

I tend to suspect changes to when/how onloadedmetadata fires instead of that.

Pehrsons - Could it be related to bug 1240478?

Comment 5

[Andreas Pehrson [:pehrsons] (PTO; back in Aug)]

(In reply to Randell Jesup [:jesup] from comment #4)
> I tend to suspect changes to when/how onloadedmetadata fires instead of that.
> 
> Pehrsons - Could it be related to bug 1240478?

Maybe.

I thought the size would be updated by the ImageContainer independent of what source we are using. Is that not happening?

Comment 6

[Andreas Pehrson [:pehrsons] (PTO; back in Aug)]

We can easily write a better test for resize and videoWidth/Height now that we can get a stream from canvas.

Comment 7

[Andreas Pehrson [:pehrsons] (PTO; back in Aug)]

It works fine here: https://jsfiddle.net/pehrsons/k1cnz9rw/

jib, can you provide a more complete example?

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment out the wait(1000) line in this fiddle: https://jsfiddle.net/kv14z7r6/

Comment 9

[Andreas Pehrson [:pehrsons] (PTO; back in Aug)]

I forked that fiddle and added some console logs (e.g., onresize so we see what's really going on): https://jsfiddle.net/pehrsons/7L9k7zLq/

Also your setInterval call was broken - the resolution now gets fixed rather quickly.

It seems however like a receiving MediaPipeline creates a 640x480 frame initially. We should figure out why.

Comment 10

[Andreas Pehrson [:pehrsons] (PTO; back in Aug)]

I traced the 640x480 back to version 31: https://hg.mozilla.org/mozilla-central/rev/8733191b0d10

Not sure if the bug is exposed there. The way they get through now is by MediaPipelineReceiveVideo::Init calling WebrtcVideoConduit::AttachRenderer which calls back to the new renderer (forwarded to the pipeline) using the method FrameSizeChanged() with the default args 640x480, since no real frames had been received and decoded yet.

If the default is 0x0 instead, I think it'll work as intended. Also seems more semantically correct...

Comment 11

[Andreas Pehrson [:pehrsons] (PTO; back in Aug)]

I can also note btw, that we do pass a couple of null images through as frames with size 640x480. This because the MediaPipelineReceiveVideo::PipelineListener (a MediaStreamListener) is pull based.

Comment 12

[Andreas Pehrson [:pehrsons] (PTO; back in Aug)]

Attachment: Default to 0x0 frame size before receiving any frames

Works for me. jib, want to double check?

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #9)
> Also your setInterval call was broken

Doh!

> Works for me. jib, want to double check?

Wfm too, even without the setInterval.

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Looks good to go. Anything else needed for this to land?

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5213de555ac0

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5213de555ac0